Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

kentslaney
Copy link
Contributor

@kentslaney kentslaney commented Feb 7, 2025

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 kentslaney requested a review from a team as a code owner February 7, 2025 18:01
@kentslaney kentslaney marked this pull request as draft February 7, 2025 18:02
@kentslaney kentslaney marked this pull request as ready for review February 9, 2025 18:00
@kentslaney
Copy link
Contributor Author

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 jimblandy force-pushed the array-size-overrides branch 6 times, most recently from 5153f2d to 7578531 Compare February 25, 2025 21:10
@jimblandy jimblandy force-pushed the array-size-overrides branch 3 times, most recently from bbe42d6 to d7b6ffd Compare February 25, 2025 21:25
@jimblandy
Copy link
Member

@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?

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

@jimblandy jimblandy Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #7227 for my confusion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment in #7227 (comment).

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

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

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

teoxoy commented Feb 26, 2025

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 jimblandy force-pushed the array-size-overrides branch from 6c18747 to 784c00c Compare February 26, 2025 19:43
@jimblandy
Copy link
Member

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
Copy link
Member

teoxoy commented Feb 27, 2025

Oh, yeah - that would work.

@kentslaney
Copy link
Contributor Author

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

@kentslaney kentslaney mentioned this pull request Feb 28, 2025
5 tasks
@kentslaney kentslaney requested a review from jimblandy March 3, 2025 17:36
@cwfitzgerald cwfitzgerald assigned teoxoy and unassigned jimblandy Mar 5, 2025
@cwfitzgerald cwfitzgerald removed the request for review from jimblandy March 5, 2025 16:11
@cwfitzgerald cwfitzgerald requested a review from teoxoy March 5, 2025 16:11
teoxoy
teoxoy previously requested changes Mar 6, 2025
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall - left comments about validation.

@jimblandy jimblandy force-pushed the array-size-overrides branch from 5737b5d to 4f4ce43 Compare March 6, 2025 21:48
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 jimblandy force-pushed the array-size-overrides branch from b14fef8 to e1b62e7 Compare March 6, 2025 22:12
@jimblandy
Copy link
Member

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

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments addressed.

@jimblandy jimblandy dismissed teoxoy’s stale review March 6, 2025 22:14

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

@jimblandy jimblandy enabled auto-merge (rebase) March 6, 2025 22:14
@kentslaney
Copy link
Contributor Author

wait, what about #7082 (comment)

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

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
Copy link
Contributor Author

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

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] panic when array override value already in arena
3 participants