[SPIR-V] Fix InterlockedMin/Max codegen (#6609)

RWByteAddressBuffer has overloads for InterlockedMin and InterlockedMax
for signed ints that were failing to compile due to mismatched types in
the generated SPIR-V instruction. This adds the missing cast if
necessary.

At the same time, some redundant code is removed from the
InterlockedMin/Max intrinsic non-member functions' codegen to modify the
opcode. If it was necessary in the past, the frontend has since been
fixed and it is no longer necessary. Tests to verify these combinations
and the necessary implicit casts have also been added.

Fixes #3196
Related to #4189, #6254, #5707

Author: sudonatalie

tools/clang/lib/SPIRV/SpirvEmitter.cpp

@@ -3873,11 +3873,24 @@ SpirvInstruction *SpirvEmitter::processRWByteAddressBufferAtomicMethods(
                              expr->getArg(3)->getLocStart(), range);
     }
   } else {
-    auto *value = doExpr(expr->getArg(1));
+    const Expr *value = expr->getArg(1);
+    SpirvInstruction *valueInstr = doExpr(expr->getArg(1));
+
+    // Since a RWAB is represented by an array of 32-bit unsigned integers, the
+    // destination pointee type will always be unsigned, and thus the SPIR-V
+    // instruction's result type and value type must also be unsigned. The
+    // signedness of the opcode is determined correctly by frontend and will
+    // correctly determine the signedness of the actual operation, but the
+    // necessary argument type cast will not be added by the frontend in the
+    // case of a signed value.
+    valueInstr =
+        castToType(valueInstr, value->getType(), astContext.UnsignedIntTy,
+                   value->getExprLoc(), range);
+
     SpirvInstruction *originalVal = spvBuilder.createAtomicOp(
         translateAtomicHlslOpcodeToSpirvOpcode(opcode),
         astContext.UnsignedIntTy, ptr, spv::Scope::Device,
-        spv::MemorySemanticsMask::MaskNone, value,
+        spv::MemorySemanticsMask::MaskNone, valueInstr,
         expr->getCallee()->getExprLoc(), range);
     if (expr->getNumArgs() > 2) {
       originalVal = castToType(originalVal, astContext.UnsignedIntTy,
@@ -9203,21 +9216,7 @@ SpirvEmitter::processIntrinsicInterlockedMethod(const CallExpr *expr,
       writeToOutputArg(originalVal, expr, 3);
   } else {
     auto *value = doArg(expr, 1);
-    // Since these atomic operations write through the provided pointer, the
-    // signed vs. unsigned opcode must be decided based on the pointee type
-    // of the first argument. However, the frontend decides the opcode based on
-    // the second argument (value). Therefore, the HLSL opcode provided by the
-    // frontend may be wrong. Therefore we need the following code to make sure
-    // we are using the correct SPIR-V opcode.
     spv::Op atomicOp = translateAtomicHlslOpcodeToSpirvOpcode(opcode);
-    if (atomicOp == spv::Op::OpAtomicUMax && baseType->isSignedIntegerType())
-      atomicOp = spv::Op::OpAtomicSMax;
-    if (atomicOp == spv::Op::OpAtomicSMax && baseType->isUnsignedIntegerType())
-      atomicOp = spv::Op::OpAtomicUMax;
-    if (atomicOp == spv::Op::OpAtomicUMin && baseType->isSignedIntegerType())
-      atomicOp = spv::Op::OpAtomicSMin;
-    if (atomicOp == spv::Op::OpAtomicSMin && baseType->isUnsignedIntegerType())
-      atomicOp = spv::Op::OpAtomicUMin;
     auto *originalVal = spvBuilder.createAtomicOp(
         atomicOp, baseType, ptr, scope, spv::MemorySemanticsMask::MaskNone,
         value, srcLoc);

tools/clang/test/CodeGenSPIRV/intrinsics.interlocked-methods.cs.hlsl

@@ -49,10 +49,22 @@ void main()
 // CHECK-NEXT:                    OpStore %original_i_val [[asmax29]]
   InterlockedMax(dest_i, 10,  original_i_val);
 
+// CHECK:      [[val30:%[0-9]+]] = OpBitcast %uint %int_n5
+// CHECK-NEXT: [[aumax:%[0-9]+]] = OpAtomicUMax %uint %dest_u %uint_2 %uint_0 [[val30]]
+// CHECK-NEXT: [[res30:%[0-9]+]] = OpBitcast %int [[aumax]]
+// CHECK-NEXT:                     OpStore %original_i_val [[res30]]
+  InterlockedMax(dest_u, -5,  original_i_val);
+
 // CHECK:      [[umin30:%[0-9]+]] = OpAtomicUMin %uint %dest_u %uint_2 %uint_0 %uint_10
 // CHECK-NEXT:                   OpStore %original_u_val [[umin30]]
   InterlockedMin(dest_u, 10,  original_u_val);
 
+// CHECK:      [[val31:%[0-9]+]] = OpBitcast %int %uint_5
+// CHECK-NEXT: [[asmin:%[0-9]+]] = OpAtomicSMin %int %dest_i %uint_2 %uint_0 [[val31]]
+// CHECK-NEXT: [[res31:%[0-9]+]] = OpBitcast %uint [[asmin]]
+// CHECK-NEXT:                     OpStore %original_u_val [[res31]]
+  InterlockedMin(dest_i, 5u,  original_u_val);
+
 // CHECK:      [[val2_31:%[0-9]+]] = OpLoad %int %val2
 // CHECK-NEXT:   [[or31:%[0-9]+]] = OpAtomicOr %int %dest_i %uint_2 %uint_0 [[val2_31]]
 // CHECK-NEXT:                   OpStore %original_i_val [[or31]]

tools/clang/test/CodeGenSPIRV/method.rw-byte-address-buffer.atomic.hlsl

@@ -116,5 +116,26 @@ float4 main() : SV_Target
 // CHECK-NOT:                    [[val]]
     myBuffer.InterlockedCompareStore(/*offset=*/16, /*compare_value=*/30, /*value=*/42);
 
+// CHECK:      [[offset_19:%[0-9]+]] = OpShiftRightLogical %uint %uint_0 %uint_2
+// CHECK-NEXT:    [[ptr_19:%[0-9]+]] = OpAccessChain %_ptr_Uniform_uint %myBuffer %uint_0 [[offset_19]]
+// CHECK-NEXT:    [[val_19:%[0-9]+]] = OpBitcast %uint %int_n1
+// CHECK-NEXT:           {{%[0-9]+}} = OpAtomicSMin %uint [[ptr_19]] %uint_1 %uint_0 [[val_19]]
+    myBuffer.InterlockedMin(0u, -1);
+
+// CHECK:      [[offset_20:%[0-9]+]] = OpShiftRightLogical %uint %uint_0 %uint_2
+// CHECK-NEXT:    [[ptr_20:%[0-9]+]] = OpAccessChain %_ptr_Uniform_uint %myBuffer %uint_0 [[offset_20]]
+// CHECK-NEXT:    [[val_20:%[0-9]+]] = OpBitcast %uint %int_n1
+// CHECK-NEXT:    [[res_20:%[0-9]+]] = OpAtomicSMax %uint [[ptr_20]] %uint_1 %uint_0 [[val_20]]
+// CHECK-NEXT:                         OpStore %originalVal [[res_20]]
+    myBuffer.InterlockedMax(0u, -1, originalVal);
+
+// CHECK:      [[offset_21:%[0-9]+]] = OpShiftRightLogical %uint %uint_0 %uint_2
+// CHECK-NEXT:    [[ptr_21:%[0-9]+]] = OpAccessChain %_ptr_Uniform_uint %myBuffer %uint_0 [[offset_21]]
+// CHECK-NEXT:    [[val_21:%[0-9]+]] = OpBitcast %uint %int_n1
+// CHECK-NEXT:    [[res_21:%[0-9]+]] = OpAtomicSMin %uint [[ptr_21]] %uint_1 %uint_0 [[val_21]]
+// CHECK-NEXT:   [[res_21b:%[0-9]+]] = OpBitcast %int [[res_21]]
+// CHECK-NEXT:                         OpStore %originalValAsInt [[res_21b]]
+    myBuffer.InterlockedMin(0u, -1, originalValAsInt);
+
     return 1.0;
 }