-
Notifications
You must be signed in to change notification settings - Fork 948
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
Merge override-expression sized arrays into existing types once resolved #6746
base: trunk
Are you sure you want to change the base?
Conversation
8b8bc89
to
767250e
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.
wgpu side good after nit
I'm not a fan of the new |
Compact is used twice as it is. This would only add to that total when array size override-expressions overlap. I'm also not sure what scale we expect this unique type arena to be. The usage of the compactor makes sense to me in this case because types are being merged and it already has the logic to go through and adjust the handles. |
Connections
Resolves #6722
Description
When an array size override-expression resolves into an existing type, naga panics because types are in a unique arena. The solution for when this happens is to add a
Resolved
variant to thePendingArraySize
type which is unique by the global expression it is initialized by. The struct also has ahandle
field for the type it is being merged into. If any of these are used, compact is called again after the pipeline overrides are resolved. Compact then removes thoseResolved
types and maps the handle index to the type it is merging into.Testing
This change modifies the existing tests by adding an array with the size of the test array when the size is overridden.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:cargo xtask test
to run tests.CHANGELOG.md
. (It's an issue that fixes a change that hadn't been released yet, should it still have a line in the release notes?)