[naga] anonymous overrides for array-size expressions #7082

kentslaney · 2025-02-07T18:01:49Z

Connections
Closes #6722

Description
When 2 arrays are sized with different overrides initialized from the same expression, naga panics because the type arena now contains a duplicate. This resolves the issue by creating an anonymous override for each override-sized array.

Testing
This change is tested by adding the test case that prompted it

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

kentslaney · 2025-02-09T18:17:27Z

In the writers, the check for unresolved overrides gets moved from the beginning into the expression processing functions. Unfortunately, unresolved overrides can manifest as non-constant statements being assigned to global variables, so any unresolved expressions get marked as a sign of unresolved overrides:

            _ => {
                return Err(Error::Override);
            }

This is valid since the code is unreachable otherwise, but it still seems like an overgeneralization if you don't know that context going in.

jimblandy · 2025-02-26T00:57:09Z

@kentslaney I'm confused about the relationship between ResolvedSize and IndexableLength. Their definitions are almost identical. Why do we have both types, instead of just using IndexableLength?

jimblandy · 2025-02-26T00:59:16Z

naga/src/valid/type.rs

@@ -533,6 +533,7 @@ impl super::Validator {
                            | TypeFlags::COPY
                            | TypeFlags::HOST_SHAREABLE
                            | TypeFlags::ARGUMENT
+                            | TypeFlags::CONSTRUCTIBLE


I think the spec says that override-sized arrays are not actually "constructible", since they need to have creation-fixed footprint.

I'll push a commit that reverts this addition, but since you went out of your way to add it, I wanted to run it by you to make sure.

Edit: I pushed such a commit, but then I removed it; see subsequent comments.

Okay, I see that CREATION_RESOLVED is what corresponds to WGSL's "constructible". This is unfortunate naming.

Filed #7227 for my confusion.

So the remaining question is: why is this added in this PR? Shouldn't it have been there since #6654?

Yes, it should have been there initially. I don't remember what prompted me noticing it. Other than the naming issue, would you like it changed somehow?

I left a comment in #7227 (comment).

Override-sized arrays are not constructible, so we shouldn't add this flag here.

wgpu-hal/src/metal/command.rs

jimblandy

This looks great. The simplification in naga::back::pipeline_constants was the kind of thing I was hoping for.

As a follow-up, I actually think it would be fine to go even further and not bother changing Override expressions to Constant expressions, since the backends can just use the same code to handle both of them. But I wonder what @teoxoy thinks.

Marking this "changes requested" until the comments get addressed. Also, please look over the commits I pushed to make sure they seem okay.

teoxoy · 2025-02-26T14:56:52Z

This looks great. The simplification in naga::back::pipeline_constants was the kind of thing I was hoping for.

This looks nice indeed!

As a follow-up, I actually think it would be fine to go even further and not bother changing Override expressions to Constant expressions, since the backends can just use the same code to handle both of them.

This would probably be troublesome since the backends expect fully evaluated constant expressions.

jimblandy · 2025-02-26T19:47:10Z

This would probably be troublesome since the backends expect fully evaluated constant expressions.

The idea is that process_overrides would go ahead and rewrite all overrides to have fully-evaluated constant expressions as their initializers, but otherwise leave things alone. So you'd have global expression handles change due to constant evaluation, but no new entries in Module::constants. Backends could handle Expression::Override just by writing out its init expression.

teoxoy · 2025-02-27T08:53:02Z

Oh, yeah - that would work.

kentslaney · 2025-02-28T22:40:16Z

It seems like the process_overrides change belongs in a separate PR. Do the changes address your concerns @jimblandy?

teoxoy

Looks good overall - left comments about validation.

naga/src/valid/mod.rs

naga/src/valid/expression.rs

When the user provides values for a module's overrides, rather than replacing override-sized array types with ordinary array types (which could require adjusting type handles throughout the module), instead edit all overrides to have initializers that are fully-evaluated constant expressions. Then, change all backends to handle override-sized arrays by retrieving their overrides' values. For arrays whose sizes are override expressions, not simple references to a specific override's value, let front ends built array types that refer to anonymous overrides whose initializers are the necessary expression. This means that all arrays whose sizes are override expressions are references to some `Override`. Remove `naga::PendingArraySize`, and let `ArraySize::Pending` hold a `Handle<Override>` in all cases. Expand `tests/gpu-tests/shader/array_size_overrides.rs` to include the test case that motivated this approach.

jimblandy · 2025-03-06T22:13:48Z

I need this change for other work, so I've addressed the review comments, rebased, and squashed.

jimblandy

Comments addressed.

I've implemented the changes Teo requested, and I'd like to merge this.

kentslaney · 2025-03-06T22:16:11Z

wait, what about #7082 (comment)

jimblandy · 2025-03-07T02:54:13Z

wait, what about #7082 (comment)

Oh, good catch - I just didn't see that one. I'm probably not able to fix it today.

kentslaney · 2025-03-16T02:14:47Z

Now that the more immediate problem has been resolved, I'll look into #7082 (review) over the next couple days.

kentslaney requested a review from a team as a code owner February 7, 2025 18:01

kentslaney marked this pull request as draft February 7, 2025 18:02

kentslaney marked this pull request as ready for review February 9, 2025 18:00

cwfitzgerald assigned jimblandy Feb 12, 2025

jimblandy force-pushed the array-size-overrides branch 6 times, most recently from 5153f2d to 7578531 Compare February 25, 2025 21:10

jimblandy mentioned this pull request Feb 25, 2025

[naga] ResolvedSize is redundant with IndexableLength #7224

Closed

jimblandy force-pushed the array-size-overrides branch 3 times, most recently from bbe42d6 to d7b6ffd Compare February 25, 2025 21:25

jimblandy reviewed Feb 26, 2025

View reviewed changes

jimblandy force-pushed the array-size-overrides branch from b2cb1cc to 6c18747 Compare February 26, 2025 01:09

jimblandy reviewed Feb 26, 2025

View reviewed changes

wgpu-hal/src/metal/command.rs Outdated Show resolved Hide resolved

jimblandy requested changes Feb 26, 2025

View reviewed changes

jimblandy force-pushed the array-size-overrides branch from 6c18747 to 784c00c Compare February 26, 2025 19:43

kentslaney mentioned this pull request Feb 28, 2025

msl bitmask fix #7255

Merged

5 tasks

kentslaney requested a review from jimblandy March 3, 2025 17:36

cwfitzgerald assigned teoxoy and unassigned jimblandy Mar 5, 2025

cwfitzgerald removed the request for review from jimblandy March 5, 2025 16:11

cwfitzgerald requested a review from teoxoy March 5, 2025 16:11

teoxoy previously requested changes Mar 6, 2025

View reviewed changes

naga/src/valid/mod.rs Outdated Show resolved Hide resolved

naga/src/valid/expression.rs Show resolved Hide resolved

jimblandy force-pushed the array-size-overrides branch from 5737b5d to 4f4ce43 Compare March 6, 2025 21:48

jimblandy force-pushed the array-size-overrides branch from b14fef8 to e1b62e7 Compare March 6, 2025 22:12

jimblandy approved these changes Mar 6, 2025

View reviewed changes

jimblandy enabled auto-merge (rebase) March 6, 2025 22:14

jimblandy merged commit 2fac7fa into gfx-rs:trunk Mar 6, 2025
34 checks passed

kentslaney mentioned this pull request Mar 7, 2025

[naga] override-sized arrays are flaged as constructible #7296

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga] anonymous overrides for array-size expressions #7082

[naga] anonymous overrides for array-size expressions #7082

kentslaney commented Feb 7, 2025 •

edited

Loading

kentslaney commented Feb 9, 2025

jimblandy commented Feb 26, 2025

jimblandy Feb 26, 2025 •

edited

Loading

jimblandy Feb 26, 2025

jimblandy Feb 26, 2025

jimblandy Feb 26, 2025

kentslaney Feb 28, 2025

teoxoy Mar 6, 2025

jimblandy left a comment •

edited

Loading

teoxoy commented Feb 26, 2025

jimblandy commented Feb 26, 2025

teoxoy commented Feb 27, 2025

kentslaney commented Feb 28, 2025

teoxoy left a comment

jimblandy commented Mar 6, 2025

jimblandy left a comment

kentslaney commented Mar 6, 2025

jimblandy commented Mar 7, 2025

kentslaney commented Mar 16, 2025

[naga] anonymous overrides for array-size expressions #7082

[naga] anonymous overrides for array-size expressions #7082

Conversation

kentslaney commented Feb 7, 2025 • edited Loading

kentslaney commented Feb 9, 2025

jimblandy commented Feb 26, 2025

jimblandy Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

jimblandy Feb 26, 2025

Choose a reason for hiding this comment

jimblandy Feb 26, 2025

Choose a reason for hiding this comment

jimblandy Feb 26, 2025

Choose a reason for hiding this comment

kentslaney Feb 28, 2025

Choose a reason for hiding this comment

teoxoy Mar 6, 2025

Choose a reason for hiding this comment

jimblandy left a comment • edited Loading

Choose a reason for hiding this comment

teoxoy commented Feb 26, 2025

jimblandy commented Feb 26, 2025

teoxoy commented Feb 27, 2025

kentslaney commented Feb 28, 2025

teoxoy left a comment

Choose a reason for hiding this comment

jimblandy commented Mar 6, 2025

jimblandy left a comment

Choose a reason for hiding this comment

kentslaney commented Mar 6, 2025

jimblandy commented Mar 7, 2025

kentslaney commented Mar 16, 2025

kentslaney commented Feb 7, 2025 •

edited

Loading

jimblandy Feb 26, 2025 •

edited

Loading

jimblandy left a comment •

edited

Loading