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

fix(hlsl-out): use Interlocked<op> intrinsic for atomic integers #2294

Merged
merged 5 commits into from
Apr 6, 2023

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Mar 29, 2023

Fixes #2284.


We currently assume that we are using raw RWByteAddressBuffer methods for all atomic operations (<pointer>.Interlocked<op>(<raw_byte_offset>, …)), which is only true when we use var<storage, read_write> globals. For var<workgroup> globals, we need Interlocked<op>(<pointer>, …), using the original expression as the first argument.

Fix this by branching on the pointer's address space in Atomic statements, and implementing the workgroup address space case with intrinsics.

@ErichDonGubler
Copy link
Member Author

@jimblandy and I are planning on pair-reviewing this draft Soon™. 😊

tests/snapshots.rs Outdated Show resolved Hide resolved
@ErichDonGubler ErichDonGubler force-pushed the hlsl-fix-interlocked-ops branch 3 times, most recently from 264e08b to f4c1539 Compare March 31, 2023 13:39
@ErichDonGubler
Copy link
Member Author

@jimblandy: I've pushed some significant updates to this during my afternoon yesterday. Most of the logic is the same, just renamed or shuffled around a bit.

@ErichDonGubler ErichDonGubler marked this pull request as ready for review March 31, 2023 13:52
@ErichDonGubler ErichDonGubler force-pushed the hlsl-fix-interlocked-ops branch 3 times, most recently from 1160b52 to 2557d13 Compare March 31, 2023 16:40
@ErichDonGubler
Copy link
Member Author

@jimblandy: Just pushed up newest changes branching on address space. TIL a bunch, thanks for the help! ❤️

ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Mar 31, 2023
I noticed that I had let `dbg!(…)` sneak into commits in gfx-rs#2294 by
accident. Clippy can help us with this! Set `deny(clippy::dbg_macro)`,
so CI catches this for future contributions.
@ErichDonGubler ErichDonGubler force-pushed the hlsl-fix-interlocked-ops branch 2 times, most recently from 4a76b78 to bf2a65e Compare April 1, 2023 03:30
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new test file looks much more complete than what we had before!

naga/tests/in/access.wgsl

Lines 154 to 169 in da8e911

@compute @workgroup_size(1)
fn atomics() {
var tmp: i32;
let value = atomicLoad(&bar.atom);
tmp = atomicAdd(&bar.atom, 5);
tmp = atomicSub(&bar.atom, 5);
tmp = atomicAnd(&bar.atom, 5);
tmp = atomicOr(&bar.atom, 5);
tmp = atomicXor(&bar.atom, 5);
tmp = atomicMin(&bar.atom, 5);
tmp = atomicMax(&bar.atom, 5);
tmp = atomicExchange(&bar.atom, 5);
// https://github.com/gpuweb/gpuweb/issues/2021
// tmp = atomicCompareExchangeWeak(&bar.atom, 5, 5);
atomicStore(&bar.atom, value);
}

I think we could remove this but the atomicExchange function seems to be missing from the new test file.

src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
@ErichDonGubler
Copy link
Member Author

@teoxoy:

The new test file looks much more complete than what we had before!

😊

I think we could remove this [(access.wgsl:154-169)] but the atomicExchange function seems to be missing from the new test file.

Agreed. Added coverage for atomicExchange with 340a698 and b14bd40, and removed the overlapping contents of access.wgsl with f8c7cb1 (and threw in e310121 for fun). Will autosquash momentarily so this branch's contents are still nice commits.

We currently assume that we are using raw `RWByteAddressBuffer` methods
for all atomic operations (`<pointer>.Interlocked<op>(<raw_byte_offset>,
…)`), which is only true when we use `var<storage, read_write>` globals.
For `var<workgroup>` globals, we need `Interlocked<op>(<pointer>, …)`,
using the original expression as the first argument.

Fix this by branching on the `pointer`'s address space in `Atomic`
statements, and implementing the workgroup address space case with
intrinsics.
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some minor cleanups needed.

src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
src/back/hlsl/writer.rs Outdated Show resolved Hide resolved
tests/out/hlsl/atomicOps.hlsl Show resolved Hide resolved
tests/out/hlsl/atomicOps.hlsl Show resolved Hide resolved
tests/out/hlsl/atomicOps.yaml Outdated Show resolved Hide resolved
@jimblandy jimblandy merged commit 99a7773 into gfx-rs:master Apr 6, 2023
ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Apr 10, 2023
I noticed that I had let `dbg!(…)` sneak into commits in gfx-rs#2294 by
accident. Clippy can help us with this! Set `deny(clippy::dbg_macro)`,
so CI catches this for future contributions.
ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request Apr 14, 2023
I noticed that I had let `dbg!(…)` sneak into commits in gfx-rs#2294 by
accident. Clippy can help us with this! Set `deny(clippy::dbg_macro)`,
so CI catches this for future contributions.
ErichDonGubler added a commit to ErichDonGubler/naga that referenced this pull request May 22, 2023
I noticed that I had let `dbg!(…)` sneak into commits in gfx-rs#2294 by
accident. Clippy can help us with this! Set `deny(clippy::dbg_macro)`,
so CI catches this for future contributions.
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.

hlsl-out miscompilation of atomic workgroup variable
3 participants