[naga] Allow dynamic indexing of array #6188

sagudev · 2024-08-31T08:31:17Z

Connections
For arrays, at least, this fixes #4337, second commit is just gfx-rs/naga#723 ported to trunk.

Description
See commits and linked PR.

Testing
There are some new tests and I did CTS run in servo.

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-09-02T05:08:36Z

Servo's CTS run: https://github.com/sagudev/servo/actions/runs/10656885185/job/29537501039 (click view raw log, then find string Stable unexpected results, from there results start):
A lot of passes (do note that servo hit flakyl CRASH webgpu:shader,execution,expression,* due to servo/servo#31397) so I will only extract bad unexpected results:

TIMEOUT /_webgpu/webgpu/cts.https.html?q=webgpu:shader,execution,expression,call,builtin,textureSample:sampled_2d_coords:*
[2024-09-01T20:57:19Z ERROR wgpu_core::device::global] Device::create_shader_module error:
           Shader validation error:
              ┌─ :34:23
              │
         34 │           data[ndx] = textureLoad(tex, global_invocation_id.xy, mipLevel);
              │                                ^^^^^^^^^^^ naga::Expression [22]
       
 
    TIMEOUT [expected PASS] subtest: :format="bc1-rgba-unorm";sample_points="texel-centre"     
    NOTRUN [expected PASS] subtest: :format="bc1-rgba-unorm";sample_points="spiral"
    ...
TIMEOUT /_webgpu/webgpu/cts.https.html?q=webgpu:shader,execution,expression,call,builtin,textureSample:sampled_3d_coords:*
[2024-09-01T20:44:34Z ERROR wgpu_core::device::global] Device::create_shader_module error:
           Shader validation error:
              ┌─ :35:11
              │
         35 │           textureLoad(
              │           ^^^^^^^^^^^ naga::Expression [23]
    PASS [expected FAIL] subtest: :format="r8unorm";viewDimension="3d";sample_points="texel-centre"
    ...
    TIMEOUT [expected FAIL] subtest: :format="bc1-rgba-unorm";viewDimension="cube";sample_points="cube-edges" 
    NOTRUN [expected FAIL] subtest: :format="bc1-rgba-unorm";viewDimension="cube";sample_points="texel-centre"
    ...

sagudev · 2024-09-02T14:43:38Z

First CTS failures is from https://github.com/gpuweb/cts/blob/9b30f7a02d172ce36d138c02614d0a5a1edbfa72/src/webgpu/shader/execution/expression/call/builtin/texture_utils.ts#L3318, because servo does not support GPUExternalTexture, second is just timeout because tests would need to be spillted more (or servo should be faster).

TL;DR all ok

ErichDonGubler · 2024-09-04T16:05:36Z

Planning on reviewing this, and I heard @teoxoy also wanted to take a look. I'll consider this a learning experience for myself, since I'm still getting familiar with Naga, while @teoxoy would be the authoritative "LGTM". 🙂

sagudev · 2024-09-04T16:08:43Z

I'll consider this a learning experience for myself, since I'm still getting familiar with Naga, while @teoxoy would be the authoritative "LGTM". 🙂

I am relatively new to naga too (my first commit to naga is 34bb9e4, which landed 5 days ago).

ErichDonGubler

LGTM; let's see what @teoxoy has to say now. 😀

naga/tests/out/wgsl/access.wgsl

jimblandy · 2024-09-05T23:23:00Z

I think the sentence "Fixes #4337 for arrays" in the description of #6188 is confusing GitHub.

jimblandy · 2024-09-05T23:42:25Z

Note that this PR must be squashed when merged.

Signed-off-by: sagudev <[email protected]>

gfx-rs/naga#723 back from dead Co-authored-by: Dzmitry Malyshau <[email protected]> Signed-off-by: sagudev <[email protected]>

Signed-off-by: sagudev <[email protected]>

jimblandy · 2024-09-05T23:57:09Z

naga/src/back/spv/block.rs

+                        self.function.internal_variables.push(variable);
+                        id
+                    }
+                    // TODO: support `crate::TypeInner::Matrix`


Please don't add TODO comments. Issues are easier to track, prioritize, and assign. Adding a comment that mentions an issue number is fine, if you like. (I know there are lots of other extant TODOs; the confusion is our fault.)

I'll fix this one myself.

jimblandy

Overall, this looks really reasonable. Not approving yet because:

I have some more tweaks I want to push
I haven't actually looked over the test changes yet.
I should be able to finish those tomorrow morning.

jimblandy · 2024-09-06T00:01:54Z

naga/src/back/spv/writer.rs

+        let pointer_type_id = self.id_gen.next();
+        Instruction::type_pointer(
+            pointer_type_id,
+            spirv::StorageClass::Function,
+            container_type_id,
+        )
+        .to_words(&mut self.logical_layout.declarations);


It turns out that, if the thing being indexed is a TypeInner::Array, that will never be a TypeResolution::Local, there will always be a Handle. So if we change this to expect a Handle<Type> instead of container_resolution, we can use get_pointer_id here as well.

I have a change that does this (and documents TypeResolution::Local a bit better) which I'll push to the branch tomorrow.

jimblandy · 2024-09-06T00:42:44Z

naga/src/back/spv/writer.rs

+        block.body.push(Instruction::load(
+            result_type_id,
+            id,
+            element_pointer_id,
+            None,
+        ));


I think doing things this way means that, if we have something like:

fn (aaa: array<array<array<f32, 3>, 3> 3>, i: i32, j: i32, k: i32) -> f32 { return aaa[i][j][k]; }

then we'll end up spilling each intermediate array into its own SPIR-V variable, even though we could handle the access normally once we've spilled the outermost thing.

I don't have the wits at the moment to fix this. It's probably fine to address it in a follow-up PR.

+1 for iterating! 😀

sagudev force-pushed the non_const_idx branch from 895c756 to f7cbb7c Compare September 1, 2024 14:14

sagudev changed the title ~~[naga] Allow dynamic indexing of array and matrix~~ [naga] Allow dynamic indexing of array Sep 1, 2024

sagudev force-pushed the non_const_idx branch from f7cbb7c to bdcd77b Compare September 2, 2024 17:30

sagudev marked this pull request as ready for review September 2, 2024 17:31

sagudev requested a review from a team as a code owner September 2, 2024 17:31

ErichDonGubler force-pushed the non_const_idx branch from bdcd77b to fc694a8 Compare September 4, 2024 15:24

ErichDonGubler assigned ErichDonGubler and sagudev Sep 4, 2024

ErichDonGubler self-requested a review September 4, 2024 16:01

ErichDonGubler added naga Shader Translator lang: WGSL WebGPU Shading Language labels Sep 4, 2024

ErichDonGubler requested a review from teoxoy September 4, 2024 16:04

ErichDonGubler assigned teoxoy Sep 4, 2024

ErichDonGubler approved these changes Sep 4, 2024

View reviewed changes

naga/tests/out/wgsl/access.wgsl Show resolved Hide resolved

ErichDonGubler mentioned this pull request Sep 4, 2024

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

Open

jimblandy assigned jimblandy and unassigned teoxoy Sep 5, 2024

ErichDonGubler requested review from jimblandy and removed request for teoxoy September 5, 2024 20:34

sagudev and others added 5 commits September 5, 2024 16:45

[wgsl-in] Allow dynamic indexing of array

fbcd5f8

Signed-off-by: sagudev <[email protected]>

[spv-out] implement array value indexing

f9f1d69

gfx-rs/naga#723 back from dead Co-authored-by: Dzmitry Malyshau <[email protected]> Signed-off-by: sagudev <[email protected]>

Update expectations

57d0833

Signed-off-by: sagudev <[email protected]>

Further updates to documentation.

1adbc7d

Minor cleanup to validator: move single true case to top of match.

6c78051

jimblandy force-pushed the non_const_idx branch from fc694a8 to 6c78051 Compare September 5, 2024 23:46

jimblandy reviewed Sep 5, 2024

View reviewed changes

jimblandy reviewed Sep 6, 2024

View reviewed changes

jimblandy added 2 commits September 5, 2024 17:47

Prefer issue links to TODO comments.

dba8dfc

Manage to use Writer::get_pointer_id, to avoid OpTypePointer dups.

83db494

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga] Allow dynamic indexing of array #6188

[naga] Allow dynamic indexing of array #6188

sagudev commented Aug 31, 2024 •

edited by jimblandy

Loading

sagudev commented Sep 2, 2024

sagudev commented Sep 2, 2024

ErichDonGubler commented Sep 4, 2024

sagudev commented Sep 4, 2024

ErichDonGubler left a comment

jimblandy commented Sep 5, 2024

jimblandy commented Sep 5, 2024

jimblandy Sep 5, 2024

jimblandy left a comment

jimblandy Sep 6, 2024

jimblandy Sep 6, 2024

ErichDonGubler Sep 6, 2024

sagudev Sep 6, 2024

[naga] Allow dynamic indexing of array #6188

Are you sure you want to change the base?

[naga] Allow dynamic indexing of array #6188

Conversation

sagudev commented Aug 31, 2024 • edited by jimblandy Loading

sagudev commented Sep 2, 2024

sagudev commented Sep 2, 2024

ErichDonGubler commented Sep 4, 2024

sagudev commented Sep 4, 2024

ErichDonGubler left a comment

Choose a reason for hiding this comment

jimblandy commented Sep 5, 2024

jimblandy commented Sep 5, 2024

jimblandy Sep 5, 2024

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy Sep 6, 2024

Choose a reason for hiding this comment

jimblandy Sep 6, 2024

Choose a reason for hiding this comment

ErichDonGubler Sep 6, 2024

Choose a reason for hiding this comment

sagudev Sep 6, 2024

Choose a reason for hiding this comment

sagudev commented Aug 31, 2024 •

edited by jimblandy

Loading