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

Add 64 bit atomics #5383

Merged
merged 1 commit into from
Jun 9, 2024
Merged

Add 64 bit atomics #5383

merged 1 commit into from
Jun 9, 2024

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Mar 13, 2024

Connections
Part of an incoming series of PRs needed by #5123
Others in this series: #5155 #5154
Addresses #4424
Depends on #5498 and #5497
Ultimately for bevy meshlets pipeline bevyengine/bevy#10164

Description
Shaders currently do not support 64 bit integer atomics. Add support for these gated by a feature flag when the capability is present.

Metal 64bit atomics are quite different from VK/DX style atomics, they do not have a return/output value of any kind and are only available in min/max variants. To accommodate this, a separate feature was added for min/max 64 bit atomics, and AtomicFunctionNoReturn was created in the naga IR to represent these return-less atomics.

DirectX ShaderModel version detection was also implemented, as SM 6.0 was insufficient for this feature.

Testing
A couple snapshot tests have been added to test int64 atomic operations.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

wgpu-hal/src/dx12/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
naga/tests/in/atomicCompareExchange-int64.wgsl Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
@cwfitzgerald cwfitzgerald self-requested a review March 14, 2024 17:00
naga/src/valid/mod.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
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.

Looking good from the wgpu side, small comments.

There should be at least one execution test that makes sure everything at least seems to behave correctly and gets through all validation layers and such. Check calling atomicAdd N times results in a value of N, etc.

wgpu-core/src/device/resource.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
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.

Still reviewing, this is just the stuff I've noticed so far.

wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
naga/src/lib.rs Outdated Show resolved Hide resolved
naga/src/valid/mod.rs Outdated Show resolved Hide resolved
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.

Before I review this further I'd like to see the doc comments filled in enough.

naga/src/lib.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

@atlv24 Thanks very much for the new comments - these are helpful.

@jimblandy
Copy link
Member

I wanted to make some changes to this PR, but in the process came across a few things that need to be fixed first. #5735 was my misunderstanding, but I think #5741 and #5763 need to be addressed. Those are both in progress.

@jimblandy
Copy link
Member

jimblandy commented Jun 4, 2024

@atlv24 I just pushed a new commit to this branch; the log message is below. Could you look it over and see what you think?

Make Statement::Atomic::result optional.

Rather than introducing an entirely new statement variant AtomicNoResult, make Statement::Atomic::result an Option. This reduces the patch size [edit] a bit, and has no effect on snapshot output other than the direct IR dumps.

In HLSL output, don't bother to generated "discard" temporaries for functions where the final original_value parameter is not required. This improves snapshot output.

Improve documentation for Expression::AtomicResult, Statement::Atomic, and the new valid::Capabilities flags.

Rewrite Atomic statement validation. Consolidate validation of AtomicResult expressions with this; #5771 ensures that there is indeed some Atomic statement referring to every AtomicResult expression, so we can be sure it will be validated.

Fixes #5742.

@jimblandy
Copy link
Member

(No offense meant by removing your docs, after asking you to write them - they were helpful in getting started!)

@jimblandy
Copy link
Member

but I think #5741 and #5763 need to be addressed.

#5741 is fixed. #5763 is not, but I think fixing #5742 should be good enough for now.

@jimblandy
Copy link
Member

Merged trunk so CI would pay attention

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.

If my changes look good, then the rest of this looks good to me.

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.

Took a brief look at the feature detection code.

wgpu-hal/src/dx12/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
@jimblandy
Copy link
Member

If my changes look good, then the rest of this looks good to me.

@atlv24 This is Just waiting for your comments on my commit, and for you to look over @teoxoy's review.

@atlv24
Copy link
Contributor Author

atlv24 commented Jun 8, 2024

@jimblandy I reviewed, looks great! I wish I had thought of using Option myself haha. Would have saved me a lot of copy pasting :)

Add the following flags to `wgpu_types::Features`:

- `SHADER_INT64_ATOMIC_ALL_OPS` enables all atomic operations on `atomic<i64>` and
  `atomic<u64>` values.

- `SHADER_INT64_ATOMIC_MIN_MAX` is a subset of the above, enabling only
  `AtomicFunction::Min` and `AtomicFunction::Max` operations on `atomic<i64>` and
  `atomic<u64>` values in the `Storage` address space. These are the only 64-bit
  atomic operations available on Metal as of 3.1.

Add corresponding flags to `naga::valid::Capabilities`. These are supported by the
WGSL front end, and all Naga backends.

Platform support:

- On Direct3d 12, in `D3D12_FEATURE_DATA_D3D12_OPTIONS9`, if
  `AtomicInt64OnTypedResourceSupported` and `AtomicInt64OnGroupSharedSupported` are
  both available, then both wgpu features described above are available.

- On Metal, `SHADER_INT64_ATOMIC_MIN_MAX` is available on Apple9 hardware, and on
  hardware that advertises both Apple8 and Mac2 support. This also requires Metal
  Shading Language 2.4 or later. Metal does not yet support the more general
  `SHADER_INT64_ATOMIC_ALL_OPS`.

- On Vulkan, if the `VK_KHR_shader_atomic_int64` extension is available with both the
  `shader_buffer_int64_atomics` and `shader_shared_int64_atomics` features, then both
  wgpu features described above are available.
@jimblandy jimblandy enabled auto-merge (rebase) June 9, 2024 01:02
@jimblandy jimblandy dismissed cwfitzgerald’s stale review June 9, 2024 01:36

Requested changes were made.

@jimblandy jimblandy merged commit abba12a into gfx-rs:trunk Jun 9, 2024
25 checks passed
@teoxoy teoxoy mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants