Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
jimblandy opened this issue May 28, 2021 · 11 comments · May be fixed by #6188
Open

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

jimblandy opened this issue May 28, 2021 · 11 comments · May be fixed by #6188
Assignees
@jimblandy
Labels
area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator type: enhancement New feature or request
Milestone
WebGPU Specificat...

Comments

@jimblandy
Copy link
Member

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.
@kvark kvark added area: validation Issues related to validation, diagnostics, and error handling kind: feature lang: WGSL WebGPU Shading Language labels May 28, 2021
@JCapucho
Copy link
Collaborator

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

@jimblandy
Copy link
Member Author

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

@jimblandy jimblandy reopened this Aug 30, 2022
@jimblandy jimblandy mentioned this issue Aug 30, 2022
Closed
@jimblandy jimblandy changed the title Support dynamic indexing of by-value matrices, per WGSL Support dynamic indexing of by-value matrices and arrays, per WGSL Apr 4, 2023
@jimblandy jimblandy mentioned this issue Apr 4, 2023
Open
davidar referenced this issue in compute-toys/wgpu-compute-toy Apr 10, 2023
@davidar
shaders broken by gfx-rs/naga#920
aeee0f1
@teoxoy teoxoy mentioned this issue Aug 8, 2023
Open
@cwfitzgerald cwfitzgerald added the naga Shader Translator label Oct 25, 2023
@cwfitzgerald cwfitzgerald transferred this issue from gfx-rs/naga Oct 25, 2023
@teoxoy teoxoy added this to the WebGPU Specification V1 milestone Nov 3, 2023
@teoxoy teoxoy removed the lang: WGSL WebGPU Shading Language label Dec 4, 2023
@cshenton
Copy link

cshenton commented Dec 4, 2023

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.

@cwfitzgerald
Copy link
Member

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.

This was referenced Dec 14, 2023
Open
Open
@nical
Copy link
Contributor

nical commented Jan 3, 2024

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

This was referenced Jan 3, 2024
Closed
Open
Open
@jimblandy jimblandy self-assigned this May 20, 2024
@ErichDonGubler
Copy link
Member

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

@teoxoy teoxoy added type: enhancement New feature or request and removed kind: feature labels Jul 3, 2024
@jimblandy
Copy link
Member Author

I've increased this issue's stack rank.

We'll want to crib from Dawn for this.

@ErichDonGubler
Copy link
Member

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

@sagudev sagudev linked a pull request Aug 31, 2024 that will close this issue
Open
6 tasks
@ErichDonGubler
Copy link
Member

@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? 😀

@sagudev
Copy link
Contributor

sagudev commented Sep 4, 2024 •
edited
Loading

@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

wgpu/naga/src/back/spv/mod.rs

Lines 234 to 238 in 960555a

/// This is the variant of [`LookupType`] used to represent types that might not
/// be available in the arena. Variants are present here for one of two reasons:
///
/// - They represent types synthesized during code generation, as explained
/// in the documentation for [`LookupType`].

@jimblandy
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jimblandy jimblandy

Labels
area: validation Issues related to validation, diagnostics, and error handling naga Shader Translator type: enhancement New feature or request
Projects
WebGPU for Firefox
Status: Todo
Milestone
WebGPU Specification V1
Development

Successfully merging a pull request may close this issue.

[naga] Allow dynamic indexing of array sagudev/wgpu
9 participants
@kvark @nical @ErichDonGubler @jimblandy @cwfitzgerald @cshenton @sagudev @teoxoy @JCapucho