Disallow illegal Atomic targets + propagate consts

docs/DXIL.rst

@@ -2997,6 +2997,9 @@ FLOW.FUNCTIONCALL                         Function with parameter is not permitt
 FLOW.NORECUSION                           Recursion is not permitted.
 FLOW.REDUCIBLE                            Execution flow must be reducible.
 INSTR.ALLOWED                             Instructions must be of an allowed type.
+INSTR.ATOMICCONST                         Constant destination to atomic.
+INSTR.ATOMICINTRINNONUAV                  Non-UAV destination to atomic intrinsic.
+INSTR.ATOMICOPNONGROUPSHARED              Non-groupshared destination to atomic operation.
 INSTR.ATTRIBUTEATVERTEXNOINTERPOLATION    Attribute %0 must have nointerpolation mode in order to use GetAttributeAtVertex function.
 INSTR.BARRIERMODEFORNONCS                 sync in a non-Compute/Amplification/Mesh Shader must only sync UAV (sync_uglobal).
 INSTR.BARRIERMODENOMEMORY                 sync must include some form of memory barrier - _u (UAV) and/or _g (Thread Group Shared Memory).  Only _t (thread group sync) is optional.

include/dxc/HLSL/HLOperations.h

@@ -132,7 +132,6 @@ void SetHLWaveSensitive(llvm::Function *F);
 bool IsHLWaveSensitive(llvm::Function *F);
 
 // For intrinsic opcode.
-bool HasUnsignedOpcode(unsigned opcode);
 unsigned GetUnsignedOpcode(unsigned opcode);
 // For HLBinaryOpcode.
 bool HasUnsignedOpcode(HLBinaryOpcode opcode);

lib/HLSL/DxilValidation.cpp

@@ -2192,6 +2192,10 @@ static void ValidateDxilOperationCallInProfile(CallInst *CI,
     if ((pOverloadType->isIntegerTy(64)) && !pSM->IsSM66Plus())
       ValCtx.EmitInstrFormatError(CI, ValidationRule::SmOpcodeInInvalidFunction,
                                   {"64-bit atomic operations", "Shader Model 6.6+"});
+    Value *Handle = CI->getOperand(DXIL::OperandIndex::kAtomicBinOpHandleOpIdx);
+    if (!isa<CallInst>(Handle) ||
+        ValCtx.GetResourceFromVal(Handle).getResourceClass() != DXIL::ResourceClass::UAV)
+      ValCtx.EmitInstrError(CI, ValidationRule::InstrAtomicIntrinNonUAV);
   } break;
   case DXIL::OpCode::CreateHandle:
     if (ValCtx.isLibProfile) {
@@ -3180,11 +3184,34 @@ static void ValidateFunctionBody(Function *F, ValidationContext &ValCtx) {
       } break;
       case Instruction::AtomicCmpXchg:
       case Instruction::AtomicRMW: {
-        Type *T = cast<PointerType>(I.getOperand(AtomicRMWInst::getPointerOperandIndex())->getType())->getElementType();
+        Value *Ptr = I.getOperand(AtomicRMWInst::getPointerOperandIndex());
+        PointerType *ptrType = cast<PointerType>(Ptr->getType());
+        Type *elType = ptrType->getElementType();
         const ShaderModel *pSM = ValCtx.DxilMod.GetShaderModel();
-        if ((T->isIntegerTy(64)) && !pSM->IsSM66Plus())
+        if ((elType->isIntegerTy(64)) && !pSM->IsSM66Plus())
           ValCtx.EmitInstrFormatError(&I, ValidationRule::SmOpcodeInInvalidFunction,
                                       {"64-bit atomic operations", "Shader Model 6.6+"});
+
+        if (ptrType->getAddressSpace() != DXIL::kTGSMAddrSpace)
+          ValCtx.EmitInstrError(&I, ValidationRule::InstrAtomicOpNonGroupshared);
+
+        // Drill through GEP and bitcasts
+        while (true) {
+          if (GEPOperator *GEP = dyn_cast<GEPOperator>(Ptr)) {
+            Ptr = GEP->getPointerOperand();
+            continue;
+          }
+          if (BitCastInst *BC = dyn_cast<BitCastInst>(Ptr)) {
+            Ptr = BC->getOperand(0);
+            continue;
+          }
+          break;
+        }
+
+        if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Ptr)) {
+          if(GV->isConstant())
+            ValCtx.EmitInstrError(&I, ValidationRule::InstrAtomicConst);
+        }
       } break;
 
       }

lib/HLSL/HLOperationLower.cpp

@@ -4421,6 +4421,44 @@ static Value* SkipAddrSpaceCast(Value* Ptr) {
   return Ptr;
 }
 
+// For known non-groupshared, verify that the destination param is valid
+void ValidateAtomicDestination(CallInst *CI, HLObjectOperationLowerHelper *pObjHelper) {
+  Value *dest = CI->getArgOperand(HLOperandIndex::kInterlockedDestOpIndex);
+  // If we encounter a gep, we may provide a more specific error message
+  bool hasGep = isa<GetElementPtrInst>(dest);
+
+  // Confirm that dest is a properly-used UAV
+
+  // Drill through subscripts and geps, anything else indicates a misuse
+  while(true) {
+    if (GetElementPtrInst *gep = dyn_cast<GetElementPtrInst>(dest)) {
+      dest = gep->getPointerOperand();
+      continue;
+    }
+    if (CallInst *handle = dyn_cast<CallInst>(dest)) {
+      hlsl::HLOpcodeGroup group = hlsl::GetHLOpcodeGroup(handle->getCalledFunction());
+      if (group != HLOpcodeGroup::HLSubscript)
+        break;
+      dest = handle->getArgOperand(HLOperandIndex::kSubscriptObjectOpIdx);
+      continue;
+    }
+    break;
+  }
+
+  if(pObjHelper->GetRC(dest) == DXIL::ResourceClass::UAV) {
+    DXIL::ResourceKind RK = pObjHelper->GetRK(dest);
+    if (DXIL::IsStructuredBuffer(RK))
+      return; // no errors
+    if (DXIL::IsTyped(RK)) {
+      if (hasGep)
+        dxilutil::EmitErrorOnInstruction(CI, "Typed resources used in atomic operations must have a scalar element type.");
+      return; // error emitted or else no errors
+    }
+  }
+
+  dxilutil::EmitErrorOnInstruction(CI, "Atomic operation targets must be groupshared or UAV.");
+}
+
 Value *TranslateIopAtomicBinaryOperation(CallInst *CI, IntrinsicOp IOP,
                                          DXIL::OpCode opcode,
                                          HLOperationLowerHelper &helper,  HLObjectOperationLowerHelper *pObjHelper, bool &Translated) {
@@ -4431,11 +4469,13 @@ Value *TranslateIopAtomicBinaryOperation(CallInst *CI, IntrinsicOp IOP,
   if (addressSpace == DXIL::kTGSMAddrSpace)
     TranslateSharedMemAtomicBinOp(CI, IOP, addr);
   else {
-    // buffer atomic translated in TranslateSubscript.
-    // Do nothing here.
-    // Mark not translated.
+    // If not groupshared, we either have an error case or will translate
+    // the atomic op in the process of translating users of the subscript operator
+    // Mark not translated and validate dest param
     Translated = false;
+    ValidateAtomicDestination(CI, pObjHelper);
   }
+
   return nullptr;
 }
 
@@ -4480,10 +4520,11 @@ Value *TranslateIopAtomicCmpXChg(CallInst *CI, IntrinsicOp IOP,
   if (addressSpace == DXIL::kTGSMAddrSpace)
     TranslateSharedMemAtomicCmpXChg(CI, addr);
   else {
-    // buffer atomic translated in TranslateSubscript.
-    // Do nothing here.
-    // Mark not translated.
+    // If not groupshared, we either have an error case or will translate
+    // the atomic op in the process of translating users of the subscript operator
+    // Mark not translated and validate dest param
     Translated = false;
+    ValidateAtomicDestination(CI, pObjHelper);
   }
 
   return nullptr;
@@ -7698,50 +7739,10 @@ void TranslateDefaultSubscript(CallInst *CI, HLOperationLowerHelper &helper,  HL
           LI->eraseFromParent();
           continue;
         }
-        if (!isa<CallInst>(GEPUser)) {
-          // Invalid operations.
-          Translated = false;
-          dxilutil::EmitErrorOnInstruction(GEP, "Invalid operation on typed buffer.");
-          return;
-        }
-        CallInst *userCall = cast<CallInst>(GEPUser);
-        HLOpcodeGroup group =
-            hlsl::GetHLOpcodeGroupByName(userCall->getCalledFunction());
-        if (group != HLOpcodeGroup::HLIntrinsic) {
-          // Invalid operations.
-          Translated = false;
-          dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer.");
-          return;
-        }
-        unsigned opcode = hlsl::GetHLOpcode(userCall);
-        IntrinsicOp IOP = static_cast<IntrinsicOp>(opcode);
-        switch (IOP) {
-        case IntrinsicOp::IOP_InterlockedAdd:
-        case IntrinsicOp::IOP_InterlockedAnd:
-        case IntrinsicOp::IOP_InterlockedExchange:
-        case IntrinsicOp::IOP_InterlockedMax:
-        case IntrinsicOp::IOP_InterlockedMin:
-        case IntrinsicOp::IOP_InterlockedUMax:
-        case IntrinsicOp::IOP_InterlockedUMin:
-        case IntrinsicOp::IOP_InterlockedOr:
-        case IntrinsicOp::IOP_InterlockedXor:
-        case IntrinsicOp::IOP_InterlockedCompareStore:
-        case IntrinsicOp::IOP_InterlockedCompareExchange:
-        case IntrinsicOp::IOP_InterlockedCompareStoreFloatBitwise:
-        case IntrinsicOp::IOP_InterlockedCompareExchangeFloatBitwise: {
-          // Invalid operations.
-          Translated = false;
-          dxilutil::EmitErrorOnInstruction(
-              userCall, "Typed resources used in atomic operations must have a scalar element type.");
-          return;
-        } break;
-        default:
-          // Invalid operations.
-          Translated = false;
-          dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer.");
-          return;
-          break;
-        }
+        // Invalid operations.
+        Translated = false;
+        dxilutil::EmitErrorOnInstruction(GEP, "Invalid operation on typed buffer.");
+        return;
       }
       GEP->eraseFromParent();
     } else {
@@ -7754,29 +7755,8 @@ void TranslateDefaultSubscript(CallInst *CI, HLOperationLowerHelper &helper,  HL
         if (RC == DXIL::ResourceClass::SRV) {
           // Invalid operations.
           Translated = false;
-          switch (IOP) {
-          case IntrinsicOp::IOP_InterlockedAdd:
-          case IntrinsicOp::IOP_InterlockedAnd:
-          case IntrinsicOp::IOP_InterlockedExchange:
-          case IntrinsicOp::IOP_InterlockedMax:
-          case IntrinsicOp::IOP_InterlockedMin:
-          case IntrinsicOp::IOP_InterlockedUMax:
-          case IntrinsicOp::IOP_InterlockedUMin:
-          case IntrinsicOp::IOP_InterlockedOr:
-          case IntrinsicOp::IOP_InterlockedXor:
-          case IntrinsicOp::IOP_InterlockedCompareStore:
-          case IntrinsicOp::IOP_InterlockedCompareExchange:
-          case IntrinsicOp::IOP_InterlockedCompareStoreFloatBitwise:
-          case IntrinsicOp::IOP_InterlockedCompareExchangeFloatBitwise: {
-            dxilutil::EmitErrorOnInstruction(
-                userCall, "Atomic operation targets must be groupshared or UAV.");
-            return;
-          } break;
-          default:
-            dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on typed buffer.");
-            return;
-            break;
-          }
+          dxilutil::EmitErrorOnInstruction(userCall, "Invalid operation on SRV.");
+          return;
         }
         switch (IOP) {
         case IntrinsicOp::IOP_InterlockedAdd: {

lib/HLSL/HLOperations.cpp

@@ -374,10 +374,6 @@ unsigned  GetRowMajorOpcode(HLOpcodeGroup group, unsigned opcode) {
   }
 }
 
-bool HasUnsignedOpcode(unsigned opcode) {
-  return HasUnsignedIntrinsicOpcode(static_cast<IntrinsicOp>(opcode));
-}
-
 unsigned GetUnsignedOpcode(unsigned opcode) {
   return GetUnsignedIntrinsicOpcode(static_cast<IntrinsicOp>(opcode));
 }

tools/clang/lib/Sema/SemaExpr.cpp

@@ -4285,7 +4285,7 @@ Sema::CreateBuiltinArraySubscriptExpr(Expr *Base, SourceLocation LLoc,
       // We need to make sure to preserve qualifiers on array types, since these
       // are in effect references.
       if (LHSTy.hasQualifiers())
-        ResultType.setLocalFastQualifiers(LHSTy.getLocalFastQualifiers());
+        ResultType.setLocalFastQualifiers(LHSTy.getQualifiers().getFastQualifiers());
     } else {
     // HLSL Change Ends
       Diag(LHSExp->getLocStart(), diag::ext_subscript_non_lvalue) <<