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

Invalid SPIR-V for shared memory atomics #5989

Closed
Axel-Reactor opened this issue Jan 2, 2025 · 9 comments
Closed

Invalid SPIR-V for shared memory atomics #5989

Axel-Reactor opened this issue Jan 2, 2025 · 9 comments
Assignees
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang

Comments

@Axel-Reactor
Copy link

#version 460

layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;

shared uint32_t SHARED_MEMORY_test_atomics_SharedMemory_shared_u32;

[shader("compute")] void main()
{
    atomicAdd((SHARED_MEMORY_test_atomics_SharedMemory_shared_u32), 1u);
}
slangc test.slang -target spirv -o test.spv
spirv-val test.spv

->

error: line 17: AtomicIAdd: storage class forbidden by universal validation rules.
  %6 = OpAtomicIAdd %uint %SHARED_MEMORY_test_atomics_SharedMemory_shared_u32 %uint_1 %uint_64 %uint_1
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 2, 2025

We do not support shared keyword at the moment, consider use groupshared instead.

@csyonghe
Copy link
Collaborator

csyonghe commented Jan 2, 2025

The current shared keyword is occupied by the D3D11 effect system syntax, and it should be safe for us to repurpose it to mean groupshared when in GLSL mode.

@Axel-Reactor
Copy link
Author

Axel-Reactor commented Jan 3, 2025

Thank you for the insight.
I still think it's a bug to not error and instead produce invalid SPIR-V?
I have a hard time finding documentation of the syntax, am I just not looking in the right place?

@juliusikkala
Copy link
Contributor

FWIW, I just got the exact same SPIR-V validation error, but in the GLSL mode using the shared keyword. I can take a look at mapping shared to groupshared in GLSL mode over the weekend unless it's already fixed by then :)

@bmillsNV bmillsNV added the goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang label Jan 7, 2025
@bmillsNV bmillsNV added this to the Q1 2025 (Winter) milestone Jan 7, 2025
@aleino-nv
Copy link
Collaborator

aleino-nv commented Jan 9, 2025

@csyonghe @juliusikkala What is left to do here?
With the PR, the generated SPIR-V is now passing validation.

@juliusikkala
Copy link
Contributor

juliusikkala commented Jan 9, 2025

The PR only fixed the 'shared' keyword in GLSL to parse into HLSLGroupSharedModifier, but not the root cause of generating invalid SPIR-V if the AST contains HLSLEffectSharedModifier instead. This issue can probably be closed if it's not possible to cause this situation via the HLSL syntax anymore either, but I haven't yet tested if that's possible.

Or maybe that should be a separate issue anyway, it's not really my call to make as a random outsider contributor :)

@aleino-nv
Copy link
Collaborator

Before the PR, the shared modifier was basically just ignored (i.e. if you remove it you get the same code) as you would expect since in HLSL it's a "hint to the compiler" https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-syntax.

So in that sense, the generated SPIR-V was not invalid because the AST contained HLSLEffectSharedModifier, but rather it was invalid because no storage class had really been specified.

@csyonghe
Copy link
Collaborator

csyonghe commented Jan 9, 2025

There needs to be a backend 4xxxx error if we see an Atomic instruction where the destination pointer isn’t group shared or from a device buffer.

@aleino-nv
Copy link
Collaborator

There needs to be a backend 4xxxx error if we see an Atomic instruction where the destination pointer isn’t group shared or from a device buffer.

That makes sense -- we are still emitting invalid SPIR-V and to avoid this we instead want to emit an error similar to the SPIR-V validation error, except in Slang terminology.

aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 15, 2025
Many of these functions map directly to atomic IR instructions.
The functions taking atomic_uint are left as they are.

This helps to address shader-slang#5989, since the destination pointer type validation can then be
written only for the atomic IR instructions.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 15, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 15, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 16, 2025
Many of these functions map directly to atomic IR instructions.
The functions taking atomic_uint are left as they are.

This helps to address shader-slang#5989, since the destination pointer type validation can then be
written only for the atomic IR instructions.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 16, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 16, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 17, 2025
Many of these functions map directly to atomic IR instructions.
The functions taking atomic_uint are left as they are.

This helps to address shader-slang#5989, since the destination pointer type validation can then be
written only for the atomic IR instructions.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 17, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 17, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 17, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 17, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 20, 2025
Many of these functions map directly to atomic IR instructions.
The functions taking atomic_uint are left as they are.

This helps to address shader-slang#5989, since the destination pointer type validation can then be
written only for the atomic IR instructions.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 20, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 22, 2025
Many of these functions map directly to atomic IR instructions.
The functions taking atomic_uint are left as they are.

This helps to address shader-slang#5989, since the destination pointer type validation can then be
written only for the atomic IR instructions.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 22, 2025
Diagnose an error if the destination of the atomic operation is not appropriate, where
appropriate means it's either:
- 'groupshared'
- from a device buffer

This closes shader-slang#5989.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang
Projects
None yet
Development

No branches or pull requests

5 participants