Image atomics support by atlv24 · Pull Request #6706 · gfx-rs/wgpu

atlv24 · 2024-12-11T22:30:26Z

Connections
Broken off from #5537

Description
Adds image atomics to Vulkan, DirectX, and Metal backends.

Testing
naga snapshot tests and runtime tests are included

Checklist

Run cargo fmt.
Run taplo format.
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.

naga/src/front/glsl/parser/types.rs

jimblandy

I started reviewing with the Naga IR, and found something that should be addressed.

naga/src/lib.rs

jimblandy · 2024-12-13T22:27:43Z

@atlv24 I appreciate the docs!

jimblandy

Some more requests.

naga/src/front/wgsl/lower/mod.rs

naga/src/lib.rs

atlv24 · 2024-12-16T04:38:49Z

@jimblandy what did you force push may i ask? and thanks for the review!

jimblandy · 2024-12-16T04:44:59Z

@jimblandy what did you force push may i ask? and thanks for the review!

Just changing // to /// to make something a doc comment in the IR, in naga/src/lib.rs.

I always feel bad asking people to change stuff like that because it's trivial, so I just fix it myself, amend to keep the PR review a single commit, and force push. If you'd rather I not amend, that's fine. (And if you'd rather I make lots of minor change requests, I could do that too, but...)

atlv24 · 2024-12-16T05:34:44Z

nah its fine, was just curious. i can't see the diff and visual inspection yielded nothing

jimblandy

In Validator::validate_entry_point, we have the following code:

let allowed_usage = match var.space {
    ...
    crate::AddressSpace::Private | crate::AddressSpace::WorkGroup => GlobalUse::all(),
    ...
};

By adding ATOMIC to GlobalUse, this became incorrect: atomic accesses to Private address space variables isn't permitted.

I think using atomics in local variables is already covered by Validator::validate_local_var, but validate_entry_point should still not say strange things.

jimblandy · 2024-12-16T16:28:12Z

@atlv24 If it's ok, since I've requested some non-trivial changes, rather than finishing this review, I'm going to go review some other, simpler PRs first, and then come back to this one once you've got it revised.

atlv24 · 2024-12-16T22:30:21Z

@jimblandy thats fine. I am unsure how to remove the sample expression handle from the naga ir, because of the way spir-v wants to write cached expressions. before i added that field, i had tried for quite a while and not found a way to generate it on the fly once, so i settled on the dumb and simple solution.

jimblandy · 2024-12-17T04:58:08Z

@atlv24 If you call back::spv::Writer::get_constant_null, that should give you a SPIR-V id whose value is zero that you can use as the MS operand. That takes an argument that is a SPIR-V type id, which I think you can get from Writer::get_uint_type_id. Or, if it's easier, you could try Write::get_constant_scalar, something like here.

atlv24 · 2024-12-18T04:26:43Z

@jimblandy resolved all feedback

wgpu-types/src/lib.rs

teoxoy · 2025-01-09T12:57:17Z

Besides the few comments I left, the naga side of this looks good to me.

teoxoy · 2025-01-09T13:03:04Z

A design question that shouldn't block this PR is: Is it necessary to have an atomic access and atomic binding type? It seems to me that the atomic functionality works on read_write storage textures as long as the format supports atomic ops.

Co-authored-by: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com>

atlv24 · 2025-01-10T17:39:40Z

Its needed because Metal needs atomic usage to be marked as a texture usage, and if we dont have the shader specify that a texture will be used atomically then we dont have a way to validate metal usage

cwfitzgerald

Alright, approving wgpu side.

I've also done a review of the naga side and I had a few comments, but everything looks fine @teoxoy can you just make sure that I didn't miss anything, then we can get this in.

naga/src/back/glsl/mod.rs

naga/src/back/msl/writer.rs

naga/src/back/wgsl/writer.rs

naga/src/lib.rs

teoxoy

LGTM

The concern was addressed.

teoxoy · 2025-01-14T10:41:21Z

Its needed because Metal needs atomic usage to be marked as a texture usage, and if we dont have the shader specify that a texture will be used atomically then we dont have a way to validate metal usage

Could you point out where metal requires this? For texture access I only see:

enum class access { sample, read, write, read_write };

This is a working naga 28 update. I noticed some tests haven't passed (specifically cargo test --all-features) since before 0.14, so this PR doesn't attempt to make them pass. ## enumerate_adaptors `instance.enumerate_adapters` is async now (and available on webgpu): gfx-rs/wgpu#8230 . more details in the wgpu release notes. ## ControlBarrier & MemoryBarrier Barrier was split in two to support MemoryBarriers: gfx-rs/wgpu#7630 From the PR, it seems like falling back to ControlBarrier is fine so that's what I did. ## Ray Query enable ray queries require `enable wgpu_ray_query;`: gfx-rs/wgpu#8545 This doesn't currently seem to make it through, and the relevant test fails. ## ImageAtomic Image atomics were added in gfx-rs/wgpu#6706 ## Mesh Shaders Mesh shaders are a major feature of wgpu 28, ~~but I've set their fields to None here in the interest of doing an upgrade and not a feature add at the same time~~ https://github.com/gfx-rs/wgpu/releases/tag/v28.0.0 update: I found some time and built a wgpu mesh/task shader demo and used that to validate some of the mesh shader functionality. I've used this to successfully compile a task shader with naga-oil and run it, but there's still something missing from the mesh shader module output here. --------- Co-authored-by: robtfm <50659922+robtfm@users.noreply.github.com>

atlv24 requested a review from a team December 11, 2024 22:30

atlv24 requested a review from a team as a code owner December 11, 2024 22:30

atlv24 mentioned this pull request Dec 11, 2024

64 bit Image Atomics #5537

Merged

6 tasks

atlv24 force-pushed the image-atomics branch from 158dedb to 9fed96c Compare December 11, 2024 22:40

cwfitzgerald self-assigned this Dec 11, 2024

atlv24 force-pushed the image-atomics branch 5 times, most recently from 6685477 to d33c0ad Compare December 12, 2024 07:38

atlv24 commented Dec 12, 2024

View reviewed changes

naga/src/front/glsl/parser/types.rs Show resolved Hide resolved

atlv24 force-pushed the image-atomics branch from d33c0ad to a43bd86 Compare December 12, 2024 08:53

jimblandy requested changes Dec 13, 2024

View reviewed changes

naga/src/lib.rs Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

jimblandy force-pushed the image-atomics branch from a43bd86 to 5bb6d54 Compare December 13, 2024 23:19

jimblandy requested changes Dec 15, 2024

View reviewed changes

naga/src/front/wgsl/lower/mod.rs Outdated Show resolved Hide resolved

naga/src/lib.rs Outdated Show resolved Hide resolved

naga/src/lib.rs Show resolved Hide resolved

jimblandy previously requested changes Dec 16, 2024

View reviewed changes

atlv24 force-pushed the image-atomics branch 2 times, most recently from 26598f2 to 00b45cc Compare December 18, 2024 04:25

atlv24 requested a review from jimblandy December 18, 2024 04:26

atlv24 force-pushed the image-atomics branch 2 times, most recently from 6c2359a to 5a4b0a8 Compare December 18, 2024 04:31

atlv24 added 3 commits January 7, 2025 20:40

fix merge

ef362fd

Fixes

24dc762

Add a couple tests

5c29250

cwfitzgerald assigned teoxoy Jan 8, 2025

teoxoy reviewed Jan 9, 2025

View reviewed changes

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved

Update wgpu-types/src/lib.rs

eec0710

Co-authored-by: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com>

atlv24 added 2 commits January 11, 2025 12:48

Merge branch 'trunk' into image-atomics

0fbac57

feedback

e1a6713

cwfitzgerald approved these changes Jan 11, 2025

View reviewed changes

naga/src/back/glsl/mod.rs Show resolved Hide resolved

naga/src/back/msl/writer.rs Outdated Show resolved Hide resolved

naga/src/back/wgsl/writer.rs Show resolved Hide resolved

naga/src/lib.rs Outdated Show resolved Hide resolved

atlv24 added 9 commits January 11, 2025 14:20

feedback

0385bb6

glsl

7714d36

glsl fix

91cdbe7

fix glsl

56bf007

fix fix

b6b6857

fix fix fic

4038041

fix?

dca50a8

fix

f70125a

Merge branch 'trunk' into image-atomics

9b8bc90

teoxoy approved these changes Jan 13, 2025

View reviewed changes

teoxoy added this pull request to the merge queue Jan 13, 2025

Merged via the queue into gfx-rs:trunk with commit 18471d8 Jan 13, 2025

atlv24 deleted the image-atomics branch June 29, 2025 21:45

ChristopherBiscardi mentioned this pull request Dec 24, 2025

naga 28 update bevyengine/naga_oil#132

Merged

Conversation

atlv24 commented Dec 11, 2024

Uh oh!

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimblandy commented Dec 13, 2024

Uh oh!

This comment was marked as outdated.

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

atlv24 commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimblandy commented Dec 16, 2024

Uh oh!

atlv24 commented Dec 16, 2024

Uh oh!

jimblandy left a comment

Choose a reason for hiding this comment

Uh oh!

jimblandy commented Dec 16, 2024

Uh oh!

atlv24 commented Dec 16, 2024

Uh oh!

jimblandy commented Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atlv24 commented Dec 18, 2024

Uh oh!

Uh oh!

teoxoy commented Jan 9, 2025

Uh oh!

teoxoy commented Jan 9, 2025

Uh oh!

atlv24 commented Jan 10, 2025

Uh oh!

cwfitzgerald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teoxoy left a comment

Choose a reason for hiding this comment

Uh oh!

teoxoy commented Jan 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

atlv24 commented Dec 16, 2024 •

edited

Loading

jimblandy commented Dec 17, 2024 •

edited

Loading