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(msl): Clear named expressions after writing each function #4594

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Naga(msl): Clear named expressions after writing each function #4594

merged 3 commits into from
Nov 2, 2023

Conversation

DJMcNab
Copy link
Contributor

@DJMcNab DJMcNab commented Oct 27, 2023

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
This bug started in gfx-rs/naga#2309

Found in linebender/vello#398

Description
When writing the local variables for a shader, the existance of previous functions could cause expressions to be incorrectly treated as if they were named.

This change appears to match other backends, and fixes the case where expressions which were named in earlier functions are used in local variable declarations

Testing
I made a reproducing wgsl file:

fn func() -> f32 {
    return 1.0;
}

fn blend_mix() {
    func();
}

fn blend_compose(
) {
    var fa = 0.0;
}

This used to have the problem, but doesn't after this PR. Additionally, we will validate have validated that Vello works as expected (once someone with a Mac can check).

I'm not sure that a regression test is very useful here.

This appears to match other backends, and fixes
fix the case where expressions which were named in earlier
functions are used in local variable declarations
@DJMcNab DJMcNab requested review from a team as code owners October 27, 2023 18:57
@raphlinus
Copy link
Contributor

Confirm this works with Vello. Thanks for the fix!

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.

Dismissing erroneous wgpu review request

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.

Makes sense, thanks!

@teoxoy teoxoy enabled auto-merge (squash) November 2, 2023 18:32
@teoxoy teoxoy merged commit 7709010 into gfx-rs:trunk Nov 2, 2023
27 checks passed
@DJMcNab DJMcNab deleted the metal_named_expressions branch November 2, 2023 22:11
@cwfitzgerald cwfitzgerald added the PR: needs back-porting PR with a fix that needs to land on crates label Nov 8, 2023
cwfitzgerald pushed a commit that referenced this pull request Nov 15, 2023
This appears to match other backends, and fixes
fix the case where expressions which were named in earlier
functions are used in local variable declarations
@cwfitzgerald cwfitzgerald removed the PR: needs back-porting PR with a fix that needs to land on crates label Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants