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] constants aren't named expressions #7304

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

sagudev
Copy link
Contributor

@sagudev sagudev commented Mar 9, 2025

Connections
Fix #6207

Description
Do not threat local constants as named expressions, and do not emit constable expr as constant in wgsl-out.

Testing
Existing tests

Squash or Rebase? Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

Copy link
Contributor

@andyleiserson andyleiserson left a comment

Choose a reason for hiding this comment

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

I think this was already clear to everyone else from the discussion in the issue, but I'll note that removing constants from named_expressions has the side effect that they're less likely to appear in the output (see here), and if they do appear, they will appear without their names and not as const.

Otherwise, this looks good to me, normally I would approve, but I think it would be good to have a stamp from somebody more familiar with the code than I am.

It would be nice to merge this soon, because it is going to conflict with the tests I am writing for my fix for #7356.

@jimblandy
Copy link
Member

I think this was already clear to everyone else from the discussion in the issue, but I'll note that removing constants from named_expressions has the side effect that they're less likely to appear in the output (see here), and if they do appear, they will appear without their names and not as const.

What Naga promises is to preserve your shader's behavior. What we aspire to is legible output that has a reasonable relation to what you wrote. So it's not really a blocker if local const expressions get removed.

Note that everything with abstract type will always be removed.

The rationale for omitting local consts is explained in #6207 (comment). This isn't the ideal solution, but it will do for now.

@sagudev
Copy link
Contributor Author

sagudev commented Mar 29, 2025

Rebased!

@sagudev
Copy link
Contributor Author

sagudev commented Apr 1, 2025

Gentle ping @jimblandy

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.

[naga wgsl-out] letdeclarations are wrongly detected as const
3 participants