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

fix(msl-out): use namer for <fun>{Input,Output} structs #6438

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Oct 22, 2024

Connections

Description

Names generated for <fun>{Input,Output} structures in the Metal backend are not escaped using the namer API. This means the naming does not avoid conflicts with shader-provided identifiers that may use the same names.

Fix this by using the namer API.

Testing

  • One can reproduce this issue by using the following shader in a render pipeline:

       struct OurVertexShaderOutput {
         @builtin(position) position: vec4f,
         @location(0) texcoord: vec2f,
       };
     
       @vertex fn vs(
         @location(0) xy: vec2f
       ) -> OurVertexShaderOutput {
         var vsOutput: OurVertexShaderOutput;
         vsOutput.position = vec4f(xy, 0.0, 1.0);
         return vsOutput;
       }
     
       @fragment fn fs() -> @location(0) vec4f {
         return vec4f(1.0, 0.0, 0.0, 1.0);
       }

    When the pipeline is created, this (rather confusing) Metal compiler error is emitted:

     Uncaptured WebGPU error: Internal error in ShaderStages(VERTEX) shader: Metal: program_source:44:12: error: no viable conversion from returned value of type 'OurVertexShaderOutput' to function return type 'vsOutput'
         return vsOutput { _tmp.position, _tmp.texcoord };
                ^~~~~~~~
     program_source:25:8: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'OurVertexShaderOutput' to 'const vsOutput &' for 1st argument
     struct vsOutput {
            ^
     program_source:25:8: note: candidate c
    
  • A regression test has been added using the above case, whose snapshot output is adjusted with this fix.

  • I've consumed this change in Firefox, and validated that the issue noted at https://webgpufundamentals.org/webgpu/lessons/webgpu-fundamentals.html#drawing-triangles-to-textures is fixed.

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • 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.

@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: naga back-end Outputs of naga shader conversion naga Shader Translator lang: Metal Metal Shading Language labels Oct 22, 2024
@ErichDonGubler ErichDonGubler self-assigned this Oct 22, 2024
@ErichDonGubler ErichDonGubler requested a review from a team as a code owner October 22, 2024 13:30
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Nice!

@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Oct 22, 2024

I didn't delve into naga-repo-era history, but it seems the most recent author to touch the code in question is @jimblandy, and his changes were mostly moving the code around. Tagging @gfx-rs/naga for awareness: let's be more suspicious of identifiers that don't go through the namer.

@ErichDonGubler ErichDonGubler merged commit 36fab5c into gfx-rs:trunk Oct 22, 2024
27 checks passed
@ErichDonGubler ErichDonGubler deleted the erichdongubler/push-xzsmnzvtwkxy branch October 22, 2024 14:14
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: Metal Metal Shading Language naga Shader Translator type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

struct <fun>Output conflicts with fn <fun> on Metal
2 participants