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

Coherent storage #6277

Closed
wants to merge 2 commits into from
Closed

Coherent storage #6277

wants to merge 2 commits into from

Conversation

JMS55
Copy link
Contributor

@JMS55 JMS55 commented Sep 15, 2024

Connections
Related to #4444

Description
Implement coherent read_write storage buffers (and storage textures?). Writes to this variable from one workgroup should be visible to another workgroup.

Syntax is var<storage, read_write, coherent>: foo.

Questions/TODO

  • Where in naga's AST should this qualifier live?
  • Is this allowed on storage textures?
  • We need a new capability in naga, and then where do we validate this?
  • Where do we validate that this is only valid with read_write access mode?
  • Metal support? (requires metal 3.2)

Generally everything around the frontend I have little idea what I'm doing.

The backend is very easy for spv (add a decoration) and hlsl (prefix RWStructuredBuffer/RWByteAddressBuffer/RWTexture2D), and mostly on glsl (goes somewhere in the variable qualifiers, order unclear), and metal I haven't even looked at.

Testing
Explain how this change is tested.

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.

@JMS55 JMS55 requested review from a team as code owners September 15, 2024 04:31
@JMS55 JMS55 marked this pull request as draft September 15, 2024 04:31
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.

So, this is something I'm not really familiar with either but:

I think what we want here is something much simpler: just have the SPIR-V back end slap the Coherent decoration on all writable storage buffers. This seems to be what Dawn does.

@jimblandy
Copy link
Member

I'm not seeing any code in Dawn to add the qualifiers in HLSL or GLSL. Is Bevy sure those are necessary?

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 18, 2024

I think what we want here is something much simpler: just have the SPIR-V back end slap the Coherent decoration on all writable storage buffers

We could do that... and wgpu should for the spec, according to #4444. But it would probably cost a good bit of performance. From what I understand, it causes a cache flush every time you write to the buffer. Not great for general purpose, hence why I wanted this to be configurable (and the WebGPU backend could default to just making everything read_write_coherent in the future).

Coherent buffers basically solve the problem that when you do a buffer write, it's not visible to any other threads. You can call storageBarrier() and force it to be visible to the workgroup, but it's still not visible to other workgroups. Coherent buffers flush the caches to force visibility to other workgroups, which is useful for some advanced techniques.

I think the spv backend might actually be a bit more complicated though, there's implications with the vulkan memory model (no idea if wgpu has this enabled or not) that complicate things

Coherent is not allowed if the declared memory model is Vulkan. The memory operand bits MakePointerAvailable and MakePointerVisible or the image operand bits MakeTexelAvailable and MakeTexelVisible can be used instead.

Tldr; this is a useful feature, and needed for WebGPU apparently, but costs perf and is complicated and I don't quite know what I'm doing

@jimblandy
Copy link
Member

Yes, eventually we will want a way to disable it selectively. But I don't think that's what Bevy needs, right? If that's so, then let's leave user control to some later PR, when someone needs it.

@jimblandy
Copy link
Member

jimblandy commented Sep 18, 2024

From what I understand, it causes a cache flush every time you write to the buffer.

I'm almost positive this is not the case. The committee would never have settled on coherence as the default if it had this kind of cost.

@jimblandy
Copy link
Member

It's forbidden to use the Coherent decoration in the Vulkan memory model, and our SPIR-V output always requests the GLSL 450 memory model.

@cwfitzgerald
Copy link
Member

From https://gpuweb.github.io/gpuweb/wgsl/#private-vs-non-private

All non-atomic read accesses in the storage or workgroup address spaces are considered non-private and correspond to read operations with NonPrivatePointer | MakePointerVisible memory operands with the Workgroup scope.

All non-atomic write accesses in the storage or workgroup address spaces are considered non-private and correspond to write operations with NonPrivatePointer | MakePointerAvailable memory operands with the Workgroup scope.

This seems to suggest the coherency is just within the workgroup level? My hunch is that this requires Coherent on vulkan but not globallycoherent on HLSL. We should chat with the dawn folks, who have thought about this more.

@JMS55
Copy link
Contributor Author

JMS55 commented Sep 18, 2024

Hmm looking at the glsl spec for coherent, you also need an appropriate barrier. I'm not quite sure how any of this works anymore 😓 .

@JMS55
Copy link
Contributor Author

JMS55 commented Oct 14, 2024

Going to close this, as I've run out of free time to work on it. That said, I'm remembering that the reason I started this was because it blocks some stuff in my engine... I might either come back to this, or have to use spv-passthrough for now.

@JMS55 JMS55 closed this Oct 14, 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.

3 participants