Correct const check for atomic methods (microsoft#4472)

The check for a const destination param for atomic functions mistakenly
encompassed the first param of atomic methods, which is an offset and
can very well be const. This changes the check to account for methods
when relevant.

Adds a test for this and a few other aspects of atomic methods

Author: pow2clk

tools/clang/lib/Sema/SemaHLSL.cpp

@@ -1862,7 +1862,10 @@ static bool IsBuiltinTable(LPCSTR tableName) {
   return tableName == kBuiltinIntrinsicTableName;
 }
 
-static bool IsAtomicOperation(LPCSTR tableName, IntrinsicOp op) {
+// Return true if the give <op> within the <tableName> namespace
+// maps to an atomic operation.
+// <acceptMethods> determines if atomic method operations will return true
+static bool IsAtomicOperation(LPCSTR tableName, IntrinsicOp op, bool acceptMethods=true) {
   if (!IsBuiltinTable(tableName))
     return false;
   switch (op) {
@@ -1877,6 +1880,7 @@ static bool IsAtomicOperation(LPCSTR tableName, IntrinsicOp op) {
   case IntrinsicOp::IOP_InterlockedMin:
   case IntrinsicOp::IOP_InterlockedOr:
   case IntrinsicOp::IOP_InterlockedXor:
+    return true;
   case IntrinsicOp::MOP_InterlockedAdd:
   case IntrinsicOp::MOP_InterlockedAnd:
   case IntrinsicOp::MOP_InterlockedCompareExchange:
@@ -1898,7 +1902,7 @@ static bool IsAtomicOperation(LPCSTR tableName, IntrinsicOp op) {
   case IntrinsicOp::MOP_InterlockedExchangeFloat:
   case IntrinsicOp::MOP_InterlockedCompareExchangeFloatBitwise:
   case IntrinsicOp::MOP_InterlockedCompareStoreFloatBitwise:
-    return true;
+    return acceptMethods;
   default:
     return false;
   }
@@ -6180,8 +6184,9 @@ bool HLSLExternalSource::MatchArguments(
 
     // Catch invalid atomic dest parameters
     if (iArg == kAtomicDstOperandIdx &&
-        IsAtomicOperation(tableName, builtinOp)) {
-      // This produces an error for bitfields that is a bit confusing because it says uint can't cast to uint
+        IsAtomicOperation(tableName, builtinOp, false /*acceptMethods*/)) {
+      // This produces an error for bitfields that is a bit confusing
+      // because it says uint can't cast to uint
       if (pType.isConstant(actx) || pCallArg->getObjectKind() == OK_BitField) {
         badArgIdx = std::min(badArgIdx, iArg);
       }

tools/clang/test/HLSLFileCheck/hlsl/intrinsics/atomic/atomic_method_restypes.hlsl

@@ -0,0 +1,92 @@
+// RUN: %dxc -T cs_6_0 -DDEST=ro_babuf -DIDX=0 %s | %FileCheck %s -check-prefixes=CHKERR
+// RUN: %dxc -T cs_6_0 -DDEST=rw_babuf -DIDX=0 %s | %FileCheck %s
+// RUN: %dxc -T cs_6_0 -DDEST=rw_babuf -DIDX=SC_IDX %s | %FileCheck %s
+// RUN: %dxc -T cs_6_0 -DDEST=rw_babuf -DIDX=C_IDX %s | %FileCheck %s
+// RUN: %dxc -T cs_6_0 -DDEST=rw_babuf -DIDX=ix %s | %FileCheck %s
+
+
+// Test various Interlocked ops using read-only byteaddressbuffers and const params
+// The way the dest param of atomic ops is lowered is unique and missed a lot of
+// these invalid uses.
+
+RWByteAddressBuffer rw_babuf;
+ByteAddressBuffer ro_babuf;
+
+RWStructuredBuffer<float4> output; // just something to keep the variables live
+
+static const uint SC_IDX = 6;
+
+uint C_IDX;
+
+[numthreads(1,1,1)]
+void main(uint ix : SV_GroupIndex) {
+
+  int val = 1;
+  uint comp = 1;
+  uint orig;
+
+  // add
+  // CHKERR: error: no member named 'InterlockedAdd' in 'ByteAddressBuffer'
+  // CHKERR: error: no member named 'InterlockedAdd' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedAdd(IDX, val);
+  DEST.InterlockedAdd(IDX, val, orig);
+
+  // min
+  // CHKERR: error: no member named 'InterlockedMin' in 'ByteAddressBuffer'
+  // CHKERR: error: no member named 'InterlockedMin' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedMin(IDX, val);
+  DEST.InterlockedMin(IDX, val, orig);
+
+  // max
+  // CHKERR: error: no member named 'InterlockedMax' in 'ByteAddressBuffer'
+  // CHKERR: error: no member named 'InterlockedMax' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedMax(IDX, val);
+  DEST.InterlockedMax(IDX, val, orig);
+
+  // and
+  // CHKERR: error: no member named 'InterlockedAnd' in 'ByteAddressBuffer'
+  // CHKERR: error: no member named 'InterlockedAnd' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedAnd(IDX, val);
+  DEST.InterlockedAnd(IDX, val, orig);
+
+  // or
+  // CHKERR: error: no member named 'InterlockedOr' in 'ByteAddressBuffer'
+  // CHKERR: error: no member named 'InterlockedOr' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedOr(IDX, val);
+  DEST.InterlockedOr(IDX, val, orig);
+
+  // xor
+  // CHKERR: error: no member named 'InterlockedXor' in 'ByteAddressBuffer'
+  // CHKERR: error: no member named 'InterlockedXor' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedXor(IDX, val);
+  DEST.InterlockedXor(IDX, val, orig);
+
+  // compareStore
+  // CHKERR: error: no member named 'InterlockedCompareStore' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicCompareExchange.i32(i32 79,
+  DEST.InterlockedCompareStore(IDX, comp, val);
+
+  // exchange
+  // CHKERR: error: no member named 'InterlockedExchange' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicBinOp.i32(i32 78,
+  DEST.InterlockedExchange(IDX, val, orig);
+
+  // compareExchange
+  // CHKERR: error: no member named 'InterlockedCompareExchange' in 'ByteAddressBuffer'
+  // CHECK: call i32 @dx.op.atomicCompareExchange.i32(i32 79,
+  DEST.InterlockedCompareExchange(IDX, comp, val, orig);
+
+  output[ix] = (float)DEST.Load<float4>(IDX);
+}