-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
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. |
5153f2d
to
7578531
Compare
bbe42d6
to
d7b6ffd
Compare
@kentslaney I'm confused about the relationship between |
@@ -533,6 +533,7 @@ impl super::Validator { | |||
| TypeFlags::COPY | |||
| TypeFlags::HOST_SHAREABLE | |||
| TypeFlags::ARGUMENT | |||
| TypeFlags::CONSTRUCTIBLE |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
b2cb1cc
to
6c18747
Compare
There was a problem hiding this 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.
This looks nice indeed!
This would probably be troublesome since the backends expect fully evaluated constant expressions. |
6c18747
to
784c00c
Compare
The idea is that |
Oh, yeah - that would work. |
It seems like the |
There was a problem hiding this 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.
5737b5d
to
4f4ce43
Compare
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.
b14fef8
to
e1b62e7
Compare
I need this change for other work, so I've addressed the review comments, rebased, and squashed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments addressed.
I've implemented the changes Teo requested, and I'd like to merge this.
wait, what about #7082 (comment) |
Oh, good catch - I just didn't see that one. I'm probably not able to fix it today. |
Now that the more immediate problem has been resolved, I'll look into #7082 (review) over the next couple days. |
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
cargo fmt
.taplo format
.cargo clippy
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.