wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

Support dynamic indexing of by-value matrices and arrays, per WGSL

Open jimblandy opened this issue 4 years ago • 8 comments

WGSL permits dynamically indexing matrices that are not stored in variables:

  • A function parameter may be a matrix.
  • A formal parameter expression does not evaluate to a pointer, but rather to the passed value.
  • A matrix access expression does not require a pointer to the matrix.

At present, Naga does not permit this: typifier::ResolveContext::resolve only permits indexing matrices behind pointers. This is in part because the only SPIR-V instructions that can take dynamic indices operate on pointers, not values. (The WGSL spec description of matrix access expressions mentions the SPIR-V OpCompositeAccess, which isn't sufficient for the job; filed as gpuweb/gpuweb#1782.)

We solved the analogous problem for arrays in gfx-rs/naga#723 by moving the value to a temporary variable, obtaining a pointer to that, and then indexing the pointer. It seems like the same tactic would work here.

jimblandy avatar May 28 '21 18:05 jimblandy

https://github.com/gpuweb/gpuweb/pull/1801 removed this, so all dynamic accesses from wgsl must happen from a reference now which means that no local will be needed, closing.

JCapucho avatar Sep 18 '21 21:09 JCapucho

The committee reversed its position, and the ability to index matrices and arrays dynamically was re-added in gpuweb/gpuweb#2427.

jimblandy avatar Aug 30 '22 21:08 jimblandy

Is there a time frame on fixing this? It is an enormous performance killer. Without this, we have to mark dynamically indexed arrays as var, which pulls them into per-thread registers, causing register pressure which kills SM occupancy.

cshenton avatar Dec 04 '23 21:12 cshenton

Are you seeing different codegen from this? I wouldn't have expected the keyword you use to affect anything once the underlying compiler got to it.

cwfitzgerald avatar Dec 04 '23 22:12 cwfitzgerald

This blocks a fair amount of the shaders at https://webgpufundamentals.org/ Bugzilla entry: https://bugzilla.mozilla.org/show_bug.cgi?id=1830763

nical avatar Jan 03 '24 10:01 nical

Another bug blocked by this on the Firefox side: https://bugzilla.mozilla.org/show_bug.cgi?id=1878323

ErichDonGubler avatar Jun 14 '24 16:06 ErichDonGubler

I've increased this issue's stack rank.

We'll want to crib from Dawn for this.

jimblandy avatar Jul 10 '24 15:07 jimblandy

Enough things are getting reported as blocked on the Firefox side that I made a bug entry to track it there, too: https://bugzilla.mozilla.org/show_bug.cgi?id=1913424

ErichDonGubler avatar Aug 16 '24 08:08 ErichDonGubler

@sagudev: Since #6188 only handles the array case, I wanted to ask: Are you interested in tackling the matrix portion of the scope in this issue after it lands? 😀

ErichDonGubler avatar Sep 04 '24 18:09 ErichDonGubler

@sagudev: Since #6188 only handles the array case, I wanted to ask: Are you interested in tackling the matrix portion of the scope in this issue after it lands? 😀

Initial I tried to do both, but hit a roadblock that I couldn't resolve fast so I minimized the scope of that PR.

The problem is that in promote_access_expression_to_variable we need element_ty: Handle<crate::Type> (for get_pointer_id), but we do not have inner type in matrix as we have for vector and we cannot insert new type because self.ir_module.types is immutable. Also relevant read https://github.com/gfx-rs/wgpu/blob/960555a426b0aae48ed36889b8c7feb48de19a6d/naga/src/back/spv/mod.rs#L234-L238

sagudev avatar Sep 04 '24 19:09 sagudev

I think there is a way to generalize get_pointer_id, but I agree this is tricky.

jimblandy avatar Sep 13 '24 23:09 jimblandy

Still need to handle the matrix portion of scope for this issue.

ErichDonGubler avatar Oct 03 '24 01:10 ErichDonGubler