Skip to content
This repository has been archived by the owner on Jan 29, 2025. It is now read-only.

hlsl-out miscompilation of atomic workgroup variable #2284

Closed
simonask opened this issue Mar 17, 2023 · 3 comments · Fixed by #2294
Closed

hlsl-out miscompilation of atomic workgroup variable #2284

simonask opened this issue Mar 17, 2023 · 3 comments · Fixed by #2294
Assignees
Labels
area: back-end Outputs of shader conversion kind: bug Something isn't working lang: HLSL High-Level Shading Language

Comments

@simonask
Copy link

Some atomic operations on workgroup-shared variables produce invalid HLSL. Similar to #2137, but without arrays.

This is using naga v0.11.0.

With this compute shader (WGSL):

var<workgroup> foo: atomic<u32>;

@compute
@workgroup_size(2)
fn test_miscompilation(@builtin(local_invocation_id) id: vec3<u32>) {
    if id.x == 0u {
        atomicStore(&foo, 0u);
    }

    workgroupBarrier();

    atomicAdd(&foo, 1u);
}

Naga generates the following HLSL:

groupshared uint foo;

[numthreads(2, 1, 1)]
void test_miscompilation(uint3 id : SV_GroupThreadID, uint3 __global_invocation_id : SV_DispatchThreadID)
{
    if (all(__global_invocation_id == uint3(0u, 0u, 0u))) {
        foo = (uint)0;
    }
    GroupMemoryBarrierWithGroupSync();
    if ((id.x == 0u)) {
        foo = 0u;
    }
    GroupMemoryBarrierWithGroupSync();
    uint _e8; foo.InterlockedAdd(0, 1u, _e8);
    return;
}

... which, when compiled with dxc -T cs_6_7, produces the following error:

test.hlsl:15:19: error: invalid format for vector swizzle 'InterlockedAdd'
    uint _e8; foo.InterlockedAdd(0, 1u, _e8);
@simonask
Copy link
Author

I don't know much about HLSL, but it looks like foo.InterlockedAdd(...) (a "method" call style) is actually nonsense?

@teoxoy teoxoy added area: back-end Outputs of shader conversion lang: HLSL High-Level Shading Language labels Mar 20, 2023
@teoxoy teoxoy added this to the WGSL Specification V1 milestone Mar 20, 2023
@teoxoy teoxoy added the kind: bug Something isn't working label Mar 20, 2023
@teoxoy
Copy link
Member

teoxoy commented Mar 20, 2023

The underlying issue seems to be the same as #2137.
The atomic functions are only methods for RWByteAddressBuffer and we should instead use the intrinsic functions (ex InterlockedAdd) for variables in the workgroup address space.

@ErichDonGubler
Copy link
Member

I'll take this one!

jimblandy pushed a commit that referenced this issue Apr 6, 2023
…2294)

We currently assume that we are using raw `RWByteAddressBuffer` methods for all atomic operations (`<pointer>.Interlocked<op>(<raw_byte_offset>, …)`), which is only true when we use `var<storage, read_write>` globals. For `var<workgroup>` globals, we need `Interlocked<op>(<pointer>, …)`, using the original expression as the first argument.

Fix this by branching on the `pointer`'s address space in `Atomic` statements, and implementing the workgroup address space case with intrinsics.

Remove atomic ops from `access`, add new `atomicOps` test.

Fixes #2284
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: back-end Outputs of shader conversion kind: bug Something isn't working lang: HLSL High-Level Shading Language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants