[naga wgsl-in] Allow global const declarations to have abstract types #7055
Conversation
jamienicol
force-pushed
the
abstract-consts
branch
from
22d7f60 to
5f74a4e
Compare
jamienicol
changed the title
naga/src/compact/functions.rs
Outdated
| let mut expr_ty = typifier.get(value, self.types); | ||
| while let crate::TypeInner::Array { base, .. } = *expr_ty { | ||
| expr_ty = &self.types[base].inner; | ||
| } |
There was a problem hiding this comment.
question: Here and in other places, we're recursing into types that can have abstract numeric components, and making sure those aren't abstract. This sounds correct. However, we're only checking for Arrays' components. Don't we also need to check for vector and matrix components?
There was a problem hiding this comment.
Face-to-face discussion with @jamienicol: This is because the scalar() helper below handles the cases I'm thinking of, but not Array. A separate helper here sounds interesting.
ErichDonGubler
added
type: enhancement
naga/src/compact/functions.rs
Outdated
| pub types: &'a crate::UniqueArena<crate::Type>, | ||
| pub resolve_context: crate::proc::ResolveContext<'a>, |
There was a problem hiding this comment.
I don't think this is the right approach. We talked in person about how this works, and I see that it does accomplish the goal. But as much as possible, I would really like to keep the compactor a dumb structural algorithm: you give it this DAG-gy module thing, and it shakes out all the disconnected pieces without looking too hard at their semantics, and that's it. Running the typifier in compaction introduces subtleties that are not the direction that I would like to see things going.
Instead, as discussed, let's try having the WGSL front end simply not add abstract-typed constants to the function's named expressions table at all. The hope is that this would make compaction just remove them naturally, since constant evaluation should have left them all unused.
There was a problem hiding this comment.
The avoid adding them to named expressions trick seemed to work. Patch is updated. It felt trivial enough just to keep it in the same PR
cwfitzgerald
dismissed
ErichDonGubler’s stale review
Code was indeed fine, some structural changes are needed per discussion with jimb
jamienicol
force-pushed
the
abstract-consts
branch
3 times, most recently
from
bbfb99c to
8563e3a
Compare
When lowering a return statement, call expression_for_abstract() rather than expression() to avoid concretizing the return value. Then, if the function has a return type, call try_automatic_conversions() to attempt to convert our return value to the correct type. This has the unfortunate side effect that some errors that would have been caught by the validator are instead encountered as conversion errors by the parser. This may result in a slightly less descriptive error message in some cases. (See the change to the invalid_functions() test, for example.)
jamienicol
force-pushed
the
abstract-consts
branch
from
8563e3a to
93f7101
Compare
…t expressions It must call ConstantEvaluator::check_and_get() to possibly retrieve the constant expression from a separate arena, like is currently done when evaluating `As` expressions for non-array casts.
… abstract types Instead allow the const to be converted and 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. A consequence of this is that abstract types may now find their way in to the IR, which we don't want. We therefore additionally now ensure that the compact pass removes global constants of abstract types. This will have no *functional* effect on shaders generated by the backends, as the expressions belonging to the abstract consts in the IR will not actually be used, as any usage in the input shader will have been const-evaluated away. Certain unused const declarations will now be removed, however, as can be seen by the effect on the snapshot outputs.
jamienicol
force-pushed
the
abstract-consts
branch
from
93f7101 to
38e2eeb
Compare
|
Updated this PR to only handle global const declarations per @jimblandy's request. As a consequence, noticed an issue with the first patch in the series (the one to handle casting constant arrays.) The problem was that However, by splitting out the local and global consts from eachother, I noticed we run in to the same problem from a different callsight - when concretizing a local const which is set to a global const. The |
jamienicol
changed the title
Connections
Fixes #6232
Depends on #7035 as it builds upon testcases added there. But could be made independent if we choose not to land that for whatever reason. The first commit of the 4 in this PR is actually from there. If there's a better way to handle dependent PRs in github please let me know!
Description
Currently global const declarations are concretized when declared, meaning if you declared an abstract float or abstract int they will be given an effective value type of f32 or i32 respectively, and can only be subsequently used as those types. The spec, however, allows const declarations to have an abstract type, which then allows, for example, an abstract int const to later be used as either an f32 or an i32 (or both) depending on the context. These patches implement that behaviour by removing the call to
concretize()when lowering a global const declaration, instead appending the Constant to the IR with its abstract type.We must, however, ensure that abstract types do not end up in the final IR that will be validated or passed to the backends. We therefore ensure that the compact pass removes any constants of abstract types. Any other non-constant expressions in the IR which referenced that abstract constant will have had to have been concretized, meaning they no longer will reference the constant. Meaning this does not remove any expressions that were actually used. It does, however, remove some unused expressions, which does have some effect on our snapshot tests.
Testing
Inspected snapshot changes and ensured validation still passes. Checked that some
webgpu:shader,validation,const_assert,const_assert:constant_expression_assert:*tests in the CTS now pass. (Some still fail in that group for other reasons.)Added some additional tests in the final patch in series. For reviewability I kept them separate from the snapshot changes caused by the fix itself.
Checklist
cargo fmt.cargo clippy. If applicable, add:cargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.