[naga wgsl-in] Allow global const declarations to have abstract types #7055

jamienicol · 2025-02-04T07:31:22Z

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

Run cargo fmt.
Run cargo clippy. If applicable, add:
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

ErichDonGubler · 2025-02-04T17:00:41Z

naga/src/compact/functions.rs

+            let mut expr_ty = typifier.get(value, self.types);
+            while let crate::TypeInner::Array { base, .. } = *expr_ty {
+                expr_ty = &self.types[base].inner;
+            }


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?

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.

naga/src/compact/functions.rs

ErichDonGubler

LGTM!

jimblandy

I'd like to split out global constants and local constants into separate PRs. Also, I think we need to take a different approach for local constants.

jimblandy · 2025-02-05T15:31:56Z

naga/src/compact/functions.rs

+    pub types: &'a crate::UniqueArena<crate::Type>,
+    pub resolve_context: crate::proc::ResolveContext<'a>,


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.

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

Code was indeed fine, some structural changes are needed per discussion with jimb

jamienicol · 2025-02-06T10:36:21Z

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 ConstantEvaluator.cast_array() did not properly handle Constant expressions by calling check_and_get(). My old patch fixed this from one callsight by making it eval a Expression::As instead (which does call check_and_get(). And making evaluating an Expression:As call cast_array instead of just cast.

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 concretize() call here also calls cast_array(). So I think the better solution is just to make cast_array() itself call check_and_get(). Additionally I noticed the comment for cast_array() specifically mentions that Expression::As does not handle arrays, so I think my previous patch was incorrect to make it do so.

jamienicol · 2025-02-10T08:37:51Z

Rebased now #7035 has been merged

jamienicol · 2025-02-12T15:25:32Z

Rebased now #7055 has merged as it conflicted. Ready for re-review

jimblandy · 2025-02-25T01:11:15Z

naga/tests/in/abstract-types-function-calls.wgsl

@@ -14,25 +14,53 @@ fn func_au(a: array<u32, 2>) {}

 fn func_f_i(a: f32, b: i32) {}

+const const_af = 0;


how juvenile

jimblandy

This looks great! Please look over the additional changes I pushed, and if they look good, please merge.

For these tests, I send you a bouquet of flowers: 💐

…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 to the validation stage, 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.

…bal consts

Define `TypeInner::is_abstract`, and let it take a type arena so that it can properly handle abstract arrays. Use this in compaction and in the WGSL front end's conversion code to recognize abstract types.

Introduce the `AbstractRule` enum to control the behavior of `Lowerer::type_and_init`. This is slightly clearer than `allow_abstract: bool`, but more wordy.

jamienicol force-pushed the abstract-consts branch from 22d7f60 to 5f74a4e Compare February 4, 2025 08:54

jamienicol changed the title ~~Allow const declarations to have abstract types~~ [naga wgsl-in] Allow const declarations to have abstract types Feb 4, 2025

ErichDonGubler self-assigned this Feb 4, 2025

ErichDonGubler reviewed Feb 4, 2025

View reviewed changes

naga/src/compact/functions.rs Outdated Show resolved Hide resolved

ErichDonGubler previously approved these changes Feb 4, 2025

View reviewed changes

ErichDonGubler added type: enhancement New feature or request naga Shader Translator area: naga front-end lang: WGSL WebGPU Shading Language type: bug Something isn't working area: ecosystem Help the connected projects grow and prosper area: correctness We're behaving incorrectly labels Feb 4, 2025

jimblandy requested changes Feb 5, 2025

View reviewed changes

cwfitzgerald assigned jimblandy and unassigned ErichDonGubler Feb 5, 2025

jamienicol force-pushed the abstract-consts branch 5 times, most recently from 93f7101 to 38e2eeb Compare February 6, 2025 10:30

jamienicol changed the title ~~[naga wgsl-in] Allow const declarations to have abstract types~~ [naga wgsl-in] Allow global const declarations to have abstract types Feb 6, 2025

jamienicol force-pushed the abstract-consts branch from 38e2eeb to fe76561 Compare February 10, 2025 08:37

jamienicol mentioned this pull request Feb 10, 2025

[naga wgsl-in] Do not attempt automatic type conversion for non-abstract types #7099

Merged

3 tasks

jamienicol force-pushed the abstract-consts branch from fe76561 to d1f4046 Compare February 12, 2025 15:24

jamienicol requested a review from jimblandy February 12, 2025 15:25

jamienicol force-pushed the abstract-consts branch 2 times, most recently from a61968b to c08f218 Compare February 17, 2025 15:25

jimblandy force-pushed the abstract-consts branch 3 times, most recently from f4f0d08 to d85e418 Compare February 25, 2025 00:53

jimblandy reviewed Feb 25, 2025

View reviewed changes

jimblandy approved these changes Feb 25, 2025

View reviewed changes

jamienicol and others added 5 commits February 25, 2025 09:01

[naga wgsl-in] Ensure ConstantEvaluator::cast_array() handles Constan…

7e72e87

…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.

[naga wgsl-in] Add additional test cases regarding abstract-typed glo…

cb9b3d9

…bal consts

[naga] Introduce TypeInner::is_abstract utility function.

216d146

Define `TypeInner::is_abstract`, and let it take a type arena so that it can properly handle abstract arrays. Use this in compaction and in the WGSL front end's conversion code to recognize abstract types.

[naga wgsl-in] Introduce AbstractRule enum.

7bacac4

Introduce the `AbstractRule` enum to control the behavior of `Lowerer::type_and_init`. This is slightly clearer than `allow_abstract: bool`, but more wordy.

jamienicol force-pushed the abstract-consts branch from 6bb7015 to 7bacac4 Compare February 25, 2025 09:11

jamienicol merged commit e0f0185 into gfx-rs:trunk Feb 25, 2025
35 checks passed

jamienicol deleted the abstract-consts branch February 25, 2025 09:25

jamienicol mentioned this pull request Feb 25, 2025

Allow local const declarations to have abstract types #7222

Merged

4 tasks

jimblandy mentioned this pull request Feb 25, 2025

[naga] Compaction is not a great removal utility #7223

Open

jamienicol mentioned this pull request Feb 28, 2025

[naga wgsl-in] Dynamically indexing abstract expression fails #7251

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga wgsl-in] Allow global const declarations to have abstract types #7055

[naga wgsl-in] Allow global const declarations to have abstract types #7055

jamienicol commented Feb 4, 2025 •

edited

Loading

ErichDonGubler Feb 4, 2025

ErichDonGubler Feb 4, 2025

ErichDonGubler left a comment

jimblandy left a comment

jimblandy Feb 5, 2025 •

edited

Loading

jamienicol Feb 5, 2025

jamienicol commented Feb 6, 2025

jamienicol commented Feb 10, 2025

jamienicol commented Feb 12, 2025

jimblandy Feb 25, 2025 •

edited

Loading

jimblandy Feb 25, 2025

jimblandy left a comment

		pub types: &'a crate::UniqueArena<crate::Type>,
		pub resolve_context: crate::proc::ResolveContext<'a>,

		@@ -14,25 +14,53 @@ fn func_au(a: array<u32, 2>) {}

		fn func_f_i(a: f32, b: i32) {}

		const const_af = 0;

[naga wgsl-in] Allow global const declarations to have abstract types #7055

[naga wgsl-in] Allow global const declarations to have abstract types #7055

Conversation

jamienicol commented Feb 4, 2025 • edited Loading

ErichDonGubler Feb 4, 2025

Choose a reason for hiding this comment

ErichDonGubler Feb 4, 2025

Choose a reason for hiding this comment

ErichDonGubler left a comment

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

jimblandy Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

jamienicol Feb 5, 2025

Choose a reason for hiding this comment

jamienicol commented Feb 6, 2025

jamienicol commented Feb 10, 2025

jamienicol commented Feb 12, 2025

jimblandy Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

jimblandy Feb 25, 2025

Choose a reason for hiding this comment

jimblandy left a comment

Choose a reason for hiding this comment

jamienicol commented Feb 4, 2025 •

edited

Loading

jimblandy Feb 5, 2025 •

edited

Loading

jimblandy Feb 25, 2025 •

edited

Loading