[naga] Support dynamic indexing of by-value matrices #6362

sagudev · 2024-10-03T15:30:42Z

Connections
Fix #4337

Description
Add support for dynamic indexing of by-value matrices, by introducing get_local_pointer_id and then follow #6188.

Testing
New tests added and because there was no expectation changes in servo's CTS run I wrote new test: gpuweb/cts#3982 (which passes with this PR).

Checklist

Run cargo fmt.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

sagudev · 2024-10-05T17:31:05Z

naga/src/back/spv/writer.rs

+        let element_pointer_type_id = match element_ty {
+            LookupType::Handle(handle) => {
+                self.get_pointer_id(ir_types, handle, spirv::StorageClass::Function)?
+            }
+            LookupType::Local(local_type) => {
+                self.get_local_pointer_id(local_type, spirv::StorageClass::Function)
+            }
+        };


Should we pack this in one single get_pointer_id method?

BTW get_pointer_id never returns Err.

I think we should update get_pointer_id and address the TODO that's currently in get_local_pointer_id.

address the TODO that's currently in get_local_pointer_id

This is hard as we cannot pack LookupType in pointer (due to infinite struct size), except if we box it. Another alternative is to simply put u32 for local type, but this could get pretty chaotic.

I think boxing solution is the most nice one. What do you think?

What I hand in mind is adding a new ValuePointer variant. Similar to:

wgpu/naga/src/lib.rs

Line 751 in 43cb730

ValuePointer {

LocalType has pointers encoded in LocalType::Value field.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>

teoxoy · 2024-10-08T16:25:08Z

naga/src/back/spv/block.rs

+                            TypeResolution::Handle(handle) => handle,
+                            TypeResolution::Value(_) => {
+                                return Err(Error::Validation(
+                                    "Array types should always be in the arena",


Suggested change

"Array types should always be in the arena",

"Matrix types should always be in the arena",

I don't think this is always true:

wgpu/naga/src/proc/typifier.rs

Line 98 in 43cb730

/// - TypeInner::Matrix (generated by matrix multiplication)

This was copy&paste from array, I will check and fix.

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>

jimblandy · 2024-10-08T18:44:57Z

I think actually just fixing #6358 for both matrices and arrays might be simpler than the combination of this and #6188. I'm taking a shot at it now, but if it's hard, then we'll take this PR instead.

sagudev · 2024-10-08T18:49:38Z

I think actually just fixing #6358 for both matrices and arrays might be simpler than the combination of this and #6188. I'm taking a shot at it now, but if it's hard, then we'll take this PR instead.

Sounds good. Ping me on PR, so I will see it (and test it with servo against CTS).

Improve handling of `Access` expressions whose base is an array or matrix (not a pointer to such), and whose index is not known at compile time. SPIR-V does not have instructions that can do this directly, so spill such values to temporary variables, and perform the accesses using `OpAccessChain` instructions applied to the temporaries. Permit dynamic indexing of matrices in validation. Handle matrices and arrays consistently; remove special cases for arrays. When performing chains of accesses like `a[i].x[j]`, do not reify intermediate values; generate a single `OpAccessIndex` for the entire thing. For details, see the comments on the new tracking structures in `naga::back::spv::Function`. Add snapshot test `index-by-value.wgsl`. Fixes gfx-rs#6358. Alternative to gfx-rs#6362.

Improve handling of `Access` expressions whose base is an array or matrix (not a pointer to such), and whose index is not known at compile time. SPIR-V does not have instructions that can do this directly, so spill such values to temporary variables, and perform the accesses using `OpAccessChain` instructions applied to the temporaries. When performing chains of accesses like `a[i].x[j]`, do not reify intermediate values; generate a single `OpAccessIndex` for the entire thing. Remove special cases for arrays; the same code now handles arrays and matrices. Update validation to permit dynamic indexing of matrices. For details, see the comments on the new tracking structures in `naga::back::spv::Function`. Add snapshot test `index-by-value.wgsl`. Fixes gfx-rs#6358. Alternative to gfx-rs#6362.

Improve handling of `Access` expressions whose base is an array or matrix (not a pointer to such), and whose index is not known at compile time. SPIR-V does not have instructions that can do this directly, so spill such values to temporary variables, and perform the accesses using `OpAccessChain` instructions applied to the temporaries. When performing chains of accesses like `a[i].x[j]`, do not reify intermediate values; generate a single `OpAccessChain` for the entire thing. Remove special cases for arrays; the same code now handles arrays and matrices. Update validation to permit dynamic indexing of matrices. For details, see the comments on the new tracking structures in `naga::back::spv::Function`. Add snapshot test `index-by-value.wgsl`. Fixes gfx-rs#6358. Alternative to gfx-rs#6362.

Improve handling of `Access` expressions whose base is an array or matrix (not a pointer to such), and whose index is not known at compile time. SPIR-V does not have instructions that can do this directly, so spill such values to temporary variables, and perform the accesses using `OpAccessChain` instructions applied to the temporaries. When performing chains of accesses like `a[i].x[j]`, do not reify intermediate values; generate a single `OpAccessChain` for the entire thing. Remove special cases for arrays; the same code now handles arrays and matrices. Update validation to permit dynamic indexing of matrices. For details, see the comments on the new tracking structures in `naga::back::spv::Function`. Add snapshot test `index-by-value.wgsl`. Fixes gfx-rs#6358. Fixes gfx-rs#4337. Alternative to gfx-rs#6362.

Improve handling of `Access` expressions whose base is an array or matrix (not a pointer to such), and whose index is not known at compile time. SPIR-V does not have instructions that can do this directly, so spill such values to temporary variables, and perform the accesses using `OpAccessChain` instructions applied to the temporaries. When performing chains of accesses like `a[i].x[j]`, do not reify intermediate values; generate a single `OpAccessChain` for the entire thing. Remove special cases for arrays; the same code now handles arrays and matrices. Update validation to permit dynamic indexing of matrices. For details, see the comments on the new tracking structures in `naga::back::spv::Function`. Add snapshot test `index-by-value.wgsl`. Fixes #6358. Fixes #4337. Alternative to #6362.

jimblandy · 2024-10-11T15:52:26Z

Closing, since #6390 has landed.

sagudev mentioned this pull request Oct 5, 2024

Test indexing of a matrix using non-const index gpuweb/cts#3982

Merged

8 tasks

This comment was marked as duplicate.

Sign in to view

sagudev commented Oct 5, 2024

View reviewed changes

sagudev marked this pull request as ready for review October 5, 2024 17:37

sagudev requested a review from a team as a code owner October 5, 2024 17:37

sagudev changed the title ~~[naga] support dyn indexing of matrix~~ [naga] Support dynamic indexing of by-value matrices Oct 5, 2024

Allow dyn indexing of matrices

a12a9ba

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>

sagudev force-pushed the dyn-idx-matrix branch from a869bd5 to a12a9ba Compare October 6, 2024 07:50

teoxoy requested changes Oct 8, 2024

View reviewed changes

sagudev added 2 commits October 8, 2024 19:57

general get_pointer_id

3bd095e

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>

update expect

c180b3b

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>

jimblandy mentioned this pull request Oct 10, 2024

[naga spv-out] Spill arrays and matrices for runtime indexing. #6390

Merged

3 tasks

jimblandy closed this Oct 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga] Support dynamic indexing of by-value matrices #6362

[naga] Support dynamic indexing of by-value matrices #6362

sagudev commented Oct 3, 2024 •

edited

Loading

This comment was marked as duplicate.

sagudev Oct 5, 2024

teoxoy Oct 8, 2024 •

edited

Loading

sagudev Oct 8, 2024

teoxoy Oct 8, 2024

sagudev Oct 8, 2024

teoxoy Oct 8, 2024

sagudev Oct 8, 2024

jimblandy commented Oct 8, 2024

sagudev commented Oct 8, 2024

jimblandy commented Oct 11, 2024

	"Array types should always be in the arena",
	"Matrix types should always be in the arena",

[naga] Support dynamic indexing of by-value matrices #6362

[naga] Support dynamic indexing of by-value matrices #6362

Conversation

sagudev commented Oct 3, 2024 • edited Loading

This comment was marked as duplicate.

sagudev Oct 5, 2024

Choose a reason for hiding this comment

teoxoy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

sagudev Oct 8, 2024

Choose a reason for hiding this comment

teoxoy Oct 8, 2024

Choose a reason for hiding this comment

sagudev Oct 8, 2024

Choose a reason for hiding this comment

teoxoy Oct 8, 2024

Choose a reason for hiding this comment

sagudev Oct 8, 2024

Choose a reason for hiding this comment

jimblandy commented Oct 8, 2024

sagudev commented Oct 8, 2024

jimblandy commented Oct 11, 2024

sagudev commented Oct 3, 2024 •

edited

Loading

teoxoy Oct 8, 2024 •

edited

Loading