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.

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

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

Open
jimblandy opened this issue May 28, 2021 · 5 comments
Open

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

jimblandy opened this issue May 28, 2021 · 5 comments
Assignees
Labels
area: validation Issues related to validation, diagnostics, and error handling kind: feature New feature or request naga Shader Translator

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 New feature or request 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 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
davidar referenced this issue in compute-toys/wgpu-compute-toy Apr 10, 2023
@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.

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: validation Issues related to validation, diagnostics, and error handling kind: feature New feature or request naga Shader Translator
Projects
None yet
Development

No branches or pull requests

7 participants