Allow local const declarations to have abstract types #7222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jamienicol
force-pushed
the
abstract-local-consts
branch
from
da8d10e to
f615461
Compare
|
Gonna proactively assign to @jimblandy, since he requested that this review be split out from #7055. |
sagudev
reviewed
Feb 28, 2025
Comment on lines
+1589
to
+1595
| // Only add constants of non-abstract types to the named expressions | ||
| // to prevent abstract types ending up in the IR. | ||
| let is_abstract = ctx.module.types[ty].inner.is_abstract(&ctx.module.types); | ||
| if !is_abstract { | ||
| ctx.named_expressions | ||
| .insert(init, (c.name.name.to_string(), c.name.span)); | ||
| } |
There was a problem hiding this comment.
I think this is another reason why we might not want const expr in named expr at all. #6207 (comment)
There was a problem hiding this comment.
That doesn't seem unreasonable to me.
…abstract types Instead allow the const to be converted each time it is const evaluated as part of another expression. This allows an abstract const to be used as a different type depending on the context. As a result, abstract types may now find their way in to the IR, which we don't want. This occurs because the compact pass treats named expressions as used, mostly so that our snapshot tests are more useful, and therefore does not remove them. To prevent this, we avoid adding abstract-typed local consts to the named expressions list. This will have no functional effect on any shaders produced by the backends, but some unused local const declarations will no longer be present.
jimblandy
force-pushed
the
abstract-local-consts
branch
from
f615461 to
cbb9f22
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Second half of #6232 (and therefore follow up from #7055) - for local consts as opposed to global consts
Description
Just like global consts, local const declarations are allowed to have abstract types. This means we must avoid concretizing them when they are declared and instead can convert them to applicable type for each expression they are used in, during constant evaluation . This ensures no used expressions of abstract types will remain after constant evaluation, and we can then rely on the compact pass to remove the unused expressions and types.
The only hitch with that is that we always treat named expressions as used, for the purposes of compaction. This is mostly so that our snapshots tests are actually useful, as otherwise the majority of expressions would be stripped! We therefore avoid adding local constants of abstract types to the named expressions list in the first place. This ensures we don't end up with abstract types in the IR during validation. But it does mean that abstract local const declarations will not appear in the output - something to bear in mind when writing snasphot tests.
Testing
Added additinal cases to snapshot tests
Squash or Rebase?
Single commit - don't mind
Checklist
cargo fmt.- [ ] Runtaplo format.cargo clippy. If applicable, add:- [ ]--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.