[spirv] Invalid codegen in RWByteAddressBuffer InterlockedMin and Max

[Jasper-Bekkers]

RWByteAddressBuffer has overloads for InterlockedMin and InterlockedMax for signed ints that fail to compile when used from SPIR-V.

    fatal error: generated SPIR-V is invalid: AtomicSMax: expected Value to be of type Result Type
      %536 = OpAtomicSMax %uint %535 %uint_1 %uint_0 %532

Obviously we can't return a %uint from a signed atomic operation, however in processRWByteAddressBufferAtomicMethods the call was always assumed to take %uint's.

This PR attempt to resolve that in the same way as for other Interlocked operations by using the result expected type instead.

Repo cases

RWByteAddressBuffer rwbab: register(u0);

[numthreads(4, 16, 1)]
void main(uint3 global_id : SV_DispatchThreadID, uint3 local_id : SV_GroupThreadID, uint3 wg_id : SV_GroupID)
{
    int k = 100;
    rwbab.InterlockedMax(0, k);

    return;
}

[Jasper-Bekkers]

Iteration times made it such that I couldn't test this until now: this change doesn't work (yet).

[s-perron]

You will want to add a unit test. That could help with your turn around time too.

See examples in https://github.com/microsoft/DirectXShaderCompiler/tree/main/tools/clang/test/CodeGenSPIRV_Lit.

The can be run by building the target check-all. If you use ninja for you build tools, then do ninja check-all.

[Jasper-Bekkers]

I actually think the fix I did is not correct, but I lack the expertise to make it correct.

An alternative way to address this I think would be to do a twos complement flip for negative numbers before feeding them into the u32 atomics.

[s-perron]

Looking at the DXIL generated for these cases, the type of the operation is determined by the type of value, not the type of dest, which is actually an offset.

[s-perron]

The problem is that the type for baseType does not match the type for value. That causes a validation error. You cannot cast the pointer to another type, but you could cast the input type to baseType, when they differ, and are the same number of bits. Do not change the opcode. The opcode seem correct as is.

[s-perron]

Take this example: https://godbolt.org/z/3sz6MEx9s.

It produces:

         %39 = OpAccessChain %_ptr_StorageBuffer_uint %buffer %uint_0 %37
         %40 = OpLoad %int %inputValue
         %41 = OpAtomicSMax %uint %39 %uint_1 %uint_0 %40

This is invalid because the pointer %39 points to a uint, and the value %40 is %int. However you can make it valid by changing it to:

         %39 = OpAccessChain %_ptr_StorageBuffer_uint %buffer %uint_0 %37
         %40 = OpLoad %int %inputValue
         %bc = OpBitcast %uint %40
         %41 = OpAtomicSMax %uint %39 %uint_1 %uint_0 %bc

[s-perron]

These changes are not needed. I believe this will give incorrect results. The parser already determine the correct opcode.

[sudonatalie]

@Jasper-Bekkers Are you still looking to address this PR or should we opt to close it at this point?

[Jasper-Bekkers]

@Jasper-Bekkers Are you still looking to address this PR or should we opt to close it at this point?

Probably alright, I don't have the bandwidth to look into this anymore from our side but the bug still remains unfortunately.