-
Notifications
You must be signed in to change notification settings - Fork 995
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 parsing Op::AtomicIIncrement #5702
Conversation
1de7a0e
to
0c434df
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.
Great to see this!
Can't comment on the code itself (though it generally looks good), but I don't think we need a special feature for it, this can just be part of the main spirv backend.
fd148b1
to
2fbdd7a
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 fine so far. Before we merge this I'd like the test to be turned into a snapshot test.
I'm kind of guessing at a bunch of stuff here so if I say something that doesn't sound right, definitely push back until it makes sense.
2fbdd7a
to
4787020
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.
LGTM
4b4b897
to
292d38a
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.
Two small issues, but otherwise looks good for now. Once you've addressed these, give me a ping and I'll merge it.
292d38a
to
c4ed78c
Compare
This is fine for now, but ideally commit comments should be a clearer explanation of the changes, of the sort one might include in CHANGELOG.txt or something, but with more detail. For example, see: 8d73e5a [edit: well, maybe that message talks a bit too much about a boring change. But the hope is that at least occasionally the commit message could save someone the time of actually reading the diff.] |
Thanks very much! Looking forward to the next episode! |
Connections
Partially adresses #4489.
This is mostly for @jimblandy and @cwfitzgerald to ensure that I'm not totally off course on my quest to add atomics.
I'm trying to make my PRs as small as possible to reduce the overhead of reviewing these as they come in :)
Description
Begins work adding support for atomics in
naga
's SPIR-V frontend.spv-in-atomics
that enables the new featureOp::AtomicIIncrement
Frontend
to track atomics use for a later passThis does not generate a valid
Module
as it predictably fails to type check ;)Testing
Added a test to parse and validate (with empty flags) a
.spv
file with anAtomicIIncrement
instruction.Checklist
cargo fmt
.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.