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

Merge override-expression sized arrays into existing types once resolved #6746

Closed
wants to merge 16 commits into from

Conversation

kentslaney
Copy link
Contributor

@kentslaney kentslaney commented Dec 14, 2024

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 the PendingArraySize type which is unique by the global expression it is initialized by. The struct also has a handle 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 those Resolved 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

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy. If applicable, add:
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md.

@kentslaney kentslaney marked this pull request as ready for review December 16, 2024 19:04
@kentslaney kentslaney requested a review from a team December 16, 2024 19:04
@kentslaney kentslaney requested a review from a team as a code owner December 16, 2024 19:04
Copy link
Member

@cwfitzgerald cwfitzgerald left a 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

tests/tests/shader/array_size_overrides.rs Show resolved Hide resolved
@teoxoy
Copy link
Member

teoxoy commented Dec 17, 2024

I'm not a fan of the new ResolvedArraySize and having to use the compactor in this case but I'm having a hard time coming up with a better solution. I asked @jimblandy for a 2nd opinion.

@kentslaney
Copy link
Contributor Author

kentslaney commented Dec 17, 2024

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.

@teoxoy
Copy link
Member

teoxoy commented Dec 19, 2024

@jimblandy and I talked about this and he proposed that we try to not rebuild the types arena and instead make the backends responsible for resolving the array length.

Since I already proposed a few things that didn't work in the end I figured I'd try to make the changes myself. I think it turned out well: #6787.

@kentslaney
Copy link
Contributor Author

Obsolete due to #6787

@kentslaney kentslaney closed this Jan 6, 2025
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