Skip to content

Conversation

@sagudev
Copy link
Collaborator

@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 added 4 commits March 29, 2025 16:59
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
…ession

Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com>
@sagudev
Copy link
Collaborator Author

sagudev commented Mar 29, 2025

Rebased!

@sagudev
Copy link
Collaborator Author

sagudev commented Apr 1, 2025

Gentle ping @jimblandy

@jimblandy jimblandy merged commit f7f0935 into gfx-rs:trunk Apr 2, 2025
37 checks passed
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