Skip to content

Properly document support for shared memory (aka groupshared) (was: implement too) #695

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

Closed
hrydgard opened this issue Jul 28, 2021 · 6 comments · Fixed by #713
Closed

Properly document support for shared memory (aka groupshared) (was: implement too) #695

hrydgard opened this issue Jul 28, 2021 · 6 comments · Fixed by #713
Labels
t: enhancement A new feature or improvement to an existing one.

Comments

@hrydgard
Copy link
Contributor

hrydgard commented Jul 28, 2021

EDIT: Answer in #695 (comment) , but docs still need improvement.

Shared memory within a group (as annotated with groupshared in HLSL) doesn't seem to be documented at least, and I can't figure out how to use it.

Basically, this in HLSL:

groupshared float4 buffer[window_size];

where threads in a whole group can share data with each other (with careful use of group barriers).

The documentation for the barriers should also be linked from the documentation for this. Also, barriers need better documentation with examples. For example https://embarkstudios.github.io/rust-gpu/api/spirv_std/arch/fn.memory_barrier.html# , it really could use a copy-pastable line. It's not super clear what to pass into the two generics arguments.

So if support isn't there, it needs to be implemented, or if it is, it needs to be documented .

@hrydgard hrydgard added the t: enhancement A new feature or improvement to an existing one. label Jul 28, 2021
@Cazadorro
Copy link

This appears to be referenced here:

#180

and here:

#8

But the actual implementation I can't find in issues.

Another problem is that "groupshared" is not standard nomenclature, and is HLSL specific, and probably should not be used as the name for this.

  • OpenCL calls this local memory. This however is confusing because it conflicts with CUDAs concept of local memory which ironically isn't local.

  • SYCL uses the same nomenclature as OpenCL.

  • OneAPI (DPC++) uses the same nomenclature as OpenCL.

  • CUDA calls this shared memory. They are the people who coined this in the first place.

  • OpenGL calls this shared memory.

  • Vulkan calls this shared memory.

  • HIP/ ROCM calls this shared memory.

AFAIK the only reason HLSL calls this "groupshared" is because it screwed up the naming of "shared" to refer to something else before the advent of GPGPU computing (I still don't understand what " Mark a variable for sharing between effects; this is a hint to the compiler. " even means, and I can't even find an example of raw "shared" HLSL memory usage anywhere).

Heck HLSL even refers to this memory as "shared memory" in their own documentation:

HLSL enables threads of a compute shader to exchange values via shared memory. HLSL provides barrier primitives such as GroupMemoryBarrierWithGroupSync, and so on to ensure the correct ordering of reads and writes to shared memory in the shader and to avoid data races.

@hrydgard hrydgard changed the title Implement (or document) support for groupshared memory Implement (or document) support for shared memory (aka groupshared) Jul 28, 2021
@hrydgard
Copy link
Contributor Author

Good point, slightly changed the issue.

@msiglreith
Copy link
Contributor

Shared memory needs to be specified in the entry point with the workgroup storage class. Besides that it works the same as a normal/generic slice.

#[spirv(compute(threads(256)))]
pub unsafe fn downsample_cs(
    ..
    #[spirv(workgroup)] scratch: &mut [[f32x4; 16]; 16],
) {

@hrydgard
Copy link
Contributor Author

hrydgard commented Jul 29, 2021

Ah, thanks! Then this issue is only about improving the documentation so that it's easy to find. Maybe it should actually be
#[spirv(shared)]
instead to be more intuitive.

Also need to figure out barriers..

@hrydgard hrydgard changed the title Implement (or document) support for shared memory (aka groupshared) Properly document support for shared memory (aka groupshared) (was: implement too) Jul 29, 2021
@msiglreith
Copy link
Contributor

I like the workgroup naming as it matches the SPIR-V naming scheme.
Regarding barriers, the test cases for the arch instrinsics are a good starting point, just needs replacing of the scopes and semantics:
https://github.com/EmbarkStudios/rust-gpu/blob/main/tests/ui/arch/memory_barrier.rs
https://github.com/EmbarkStudios/rust-gpu/blob/main/tests/ui/arch/control_barrier.rs

@Cazadorro
Copy link

@hrydgard Actually didn't know this either, and even though I suggested using shared, I think following spirv semantics in code is much more important, so I agree with @msiglreith that nothing should change codewise, though clearly there is an issue with finding if these features have been implemented or are being worked on or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement A new feature or improvement to an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants