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] Support pipeline overrides in the WGSL backend #6310

Closed
wants to merge 4 commits into from

Conversation

cbbowen
Copy link

@cbbowen cbbowen commented Sep 22, 2024

Connections
#4484

Description
The WGSL backend currently does not support pipeline overrides (i.e. override foo: f32). These are included in the latest WGSL spec, this PR adds support for them by:

  1. Generating appropriate override declarations in wgsl::Writer.
  2. Handling Override expressions in wgsl::Writer.
  3. Handling Unary and Binary constant expressions in wgsl::Writer.

This last step was necessary because the existing "overrides" snapshot test exercises the functionality, but it isn't currently implemented for the WGSL backend.

Testing
The first commit in this PR enables the existing "overrides" snapshot tests for the WGSL target which I verified fails. The following commits update the backend such that it passes and generates semantically equivalent WGSL to the input.

Ran:

cargo test --all-features
cargo xtask validate wgsl

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.

@cbbowen cbbowen requested a review from a team as a code owner September 22, 2024 22:44
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Just to help the naga reviewers: the testing looks good on this PR

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.

I feel confident that the behavior intended to be allowed here is correct, but I don't feel confident that we're rejecting improper programs. I'm not very familiar with overrides to begin with. @jimblandy, could we maybe pair on this, so I can get your thoughts?

@ErichDonGubler ErichDonGubler added naga Shader Translator lang: WGSL WebGPU Shading Language area: naga back-end Outputs of naga shader conversion labels Sep 27, 2024
@ErichDonGubler
Copy link
Member

@jimblandy: I intend to merge this in the next 24 hours, unless you have an objection.

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.

Rather than adding limited support for some operations to back::wgsl::writer, you should just make write_output_wgsl in naga/tests/snapshots.rs call naga::back::pipeline_constants::process_overrides, the way the other write_output_foo functions do.

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.

Going a bit further than my previous comment:

I don't think this PR is the right approach. Naga backends should assume that overrides' values have been provided, and the module has been rewritten so that all overrides appear as constants instead. So rather than the approach taken here, I think the WGSL backend should handle overrides the same way the other backends do:

        if !module.overrides.is_empty() {
            return Err(Error::Override);
        }

As explained in the prior review, it should be the caller's responsibility to call pipeline_constants::process_overrides and eliminate all overrides from the module before passing it to the backend.

@cbbowen
Copy link
Author

cbbowen commented Oct 3, 2024

To verify I understand, the intent is that modules with pipeline overrides should not be translated from one language to another? They must instead be substituted with concrete values. That precludes some use cases, including mine, but I can understand not wanting to take on that maintenance burden. If that understanding is correct, I can close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: naga back-end Outputs of naga shader conversion lang: WGSL WebGPU Shading Language naga Shader Translator
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants