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

[naga] Allow dynamic indexing of array #6188

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Aug 31, 2024

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 sagudev changed the title [naga] Allow dynamic indexing of array and matrix [naga] Allow dynamic indexing of array Sep 1, 2024
@sagudev
Copy link
Contributor Author

sagudev commented Sep 2, 2024

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
Copy link
Contributor Author

sagudev commented Sep 2, 2024

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
Copy link
Member

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
Copy link
Contributor Author

sagudev commented Sep 4, 2024

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).

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

naga/tests/out/wgsl/access.wgsl Show resolved Hide resolved
@jimblandy
Copy link
Member

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

@jimblandy
Copy link
Member

Note that this PR must be squashed when merged.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

naga/src/back/spv/writer.rs Outdated Show resolved Hide resolved
naga/src/back/spv/writer.rs Show resolved Hide resolved
@sagudev
Copy link
Contributor Author

sagudev commented Oct 2, 2024

What blocking this? Is there something I need to do?

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Oct 2, 2024

@sagudev: I don't see anything that's blocking; I see no objection to this PR merging, concrete or abstract, and there's an approving review (from me). We definitely dropped the ball on this one, sorry about that!

@gfx-rs/naga, unless you object in the next 24ish hours, I'm gonna merge this.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

Bring gfx-rs/naga#723 back from the dead.

Signed-off-by: sagudev <[email protected]>
Co-authored-by: Dzmitry Malyshau <[email protected]>
Co-authored-by: Jim Blandy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: WGSL WebGPU Shading Language naga Shader Translator
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

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