-
Notifications
You must be signed in to change notification settings - Fork 970
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
[spv-in] Support atomics in fields of global structs #6693
[spv-in] Support atomics in fields of global structs #6693
Conversation
f49946a
to
fe354df
Compare
@schell I pushed a commit with some doc fixes. Do they look okay? |
e18de74
to
01b89cf
Compare
@jimblandy that looks good to me 👍 ! |
I'm a little concerned that, if the input has a struct with N fields that are accessed as atomics, this will create N new copies of the struct, as we perform each upgrade. It feels like there's more to worry about here, although I can't put my finger on it this instant. I'll come back to this in a bit. |
@schell I was concerned about the fact that the algorithm might visit I just pushed a commit as a "big suggestion" that ensures we visit types only once, and that we have all the information we need when we get there. Could you take a look and see what you think? |
@teoxoy My last "big suggestion" commit here is large enough that I was wondering if someone else should look it over. What do you think? |
I think these changes look great! It would be good to get a snapshot test of a shader that uses atomics on multiple fields to ensure these are correct, which I'll work on, but I don't want to hold up this merging. |
The changes in the last commit look good to me as well. |
Handle SPIR-V input that performs atomic operations on structs that have more than one field. Track which fields of which struct types are used by atomic operations on which global variables, and then give those global variables new types in which exactly those fields have had their `Scalar` leaf types changed to `Atomic`. Add snapshot tests. Co-authored-by: Jim Blandy <jimb@red-bean.com>
5320166
to
54dd512
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.
Okay, this all looks good to me, and since my changes weren't trivial, they have been reviewed by @teoxoy. So I think we're good to go.
🚀 |
Connections
Description
Adds support for using atomic operations on specific fields of a global struct.
Testing
These changes add a snapshot test for the SPIR-V frontend of a shader that uses an atomic op on one of three fields of a global struct. As expected, this shader fails to validate on
trunk
, resulting inAtomicUpgradeError(MultiMemberStruct)
.These changes fix the atomic upgrade machinery to properly upgrade the type of only the specific field that the atomic operation is performed on. It does this by recording struct field access when the global variable is discovered and then unrolling those accesses later, during type upgrade.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.