-
Notifications
You must be signed in to change notification settings - Fork 923
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
Coherent storage #6277
Conversation
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.
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.
I'm not seeing any code in Dawn to add the qualifiers in HLSL or GLSL. Is Bevy sure those are necessary? |
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
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 |
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. |
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. |
It's forbidden to use the |
From https://gpuweb.github.io/gpuweb/wgsl/#private-vs-non-private
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. |
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 😓 . |
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. |
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
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
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.