Check Race Condition for TGSM (#48)

docs/DXIL.rst

@@ -2178,6 +2178,7 @@ INSTR.SAMPLERMODEFORSAMPLE            sample/_l/_d/_cl_s/gather instruction requ
 INSTR.SAMPLERMODEFORSAMPLEC           sample_c_*/gather_c instructions require sampler declared in comparison mode
 INSTR.STRUCTBITCAST                   Bitcast on struct types is not allowed
 INSTR.TEXTUREOFFSET                   offset texture instructions must take offset which can resolve to integer literal in the range -8 to 7
+INSTR.TGSMRACECOND                    Race condition writing to shared memory detected, consider making this write conditional
 INSTR.UNDEFRESULTFORGETDIMENSION      GetDimensions used undef dimension %0 on %1
 INSTR.WRITEMASKFORTYPEDUAVSTORE       store on typed uav must write to all four components of the UAV
 INSTR.WRITEMASKMATCHVALUEFORUAVSTORE  uav store write mask must match store value mask, write mask is %0 and store value mask is %1

include/dxc/HLSL/DxilValidation.h

@@ -96,6 +96,7 @@ enum class ValidationRule : unsigned {
   InstrSamplerModeForSample, // sample/_l/_d/_cl_s/gather instruction requires sampler declared in default mode
   InstrSamplerModeForSampleC, // sample_c_*/gather_c instructions require sampler declared in comparison mode
   InstrStructBitCast, // Bitcast on struct types is not allowed
+  InstrTGSMRaceCond, // Race condition writing to shared memory detected, consider making this write conditional
   InstrTextureOffset, // offset texture instructions must take offset which can resolve to integer literal in the range -8 to 7
   InstrUndefResultForGetDimension, // GetDimensions used undef dimension %0 on %1
   InstrWriteMaskForTypedUAVStore, // store on typed uav must write to all four components of the UAV

lib/HLSL/DxilValidation.cpp

@@ -25,6 +25,7 @@
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"
+#include "llvm/IR/Operator.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Constants.h"
@@ -36,6 +37,7 @@
 #include <unordered_set>
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/Analysis/PostDominators.h"
 #include "dxc/HLSL/DxilSpanAllocator.h"
 #include "dxc/HLSL/DxilSignatureAllocator.h"
 #include <algorithm>
@@ -158,6 +160,7 @@ const char *hlsl::GetValidationRuleText(ValidationRule value) {
     case hlsl::ValidationRule::InstrCBufferClassForCBufferHandle: return "Expect Cbuffer for CBufferLoad handle";
     case hlsl::ValidationRule::InstrFailToResloveTGSMPointer: return "TGSM pointers must originate from an unambiguous TGSM global variable.";
     case hlsl::ValidationRule::InstrExtractValue: return "ExtractValue should only be used on dxil struct types and cmpxchg";
+    case hlsl::ValidationRule::InstrTGSMRaceCond: return "Race condition writing to shared memory detected, consider making this write conditional";
     case hlsl::ValidationRule::TypesNoVector: return "Vector type '%0' is not allowed";
     case hlsl::ValidationRule::TypesDefined: return "Type '%0' is not defined on DXIL primitives";
     case hlsl::ValidationRule::TypesIntWidth: return "Int type '%0' has an invalid width";
@@ -2551,15 +2554,57 @@ static void ValidateGlobalVariable(GlobalVariable &GV,
   }
 }
 
+static void
+CollectFixAddressAccess(Value *V, std::vector<Instruction *> &fixAddrTGSMList) {
+  for (User *U : V->users()) {
+    if (GEPOperator *GEP = dyn_cast<GEPOperator>(U)) {
+      if (isa<ConstantExpr>(GEP) || GEP->hasAllConstantIndices()) {
+        CollectFixAddressAccess(GEP, fixAddrTGSMList);
+      }
+	} else if (isa<StoreInst>(U)) {
+	}
+  }
+}
+
+static void
+ValidateTGSMRaceCondition(std::vector<Instruction *> &fixAddrTGSMList,
+                          ValidationContext &ValCtx) {
+  std::unordered_set<Function *> fixAddrTGSMFuncSet;
+  for (Instruction *I : fixAddrTGSMList) {
+	BasicBlock *BB = I->getParent();
+	fixAddrTGSMFuncSet.insert(BB->getParent());
+  }
+
+  for (auto &F : ValCtx.DxilMod.GetModule()->functions()) {
+      continue;
+
+    PostDominatorTree PDT;
+    PDT.runOnFunction(F);
+
+    BasicBlock *Entry = &F.getEntryBlock();
+
+    for (Instruction *I : fixAddrTGSMList) {
+      BasicBlock *BB = I->getParent();
+      if (BB->getParent() == &F) {
+        if (PDT.dominates(BB, Entry)) {
+          ValCtx.EmitInstrError(I, ValidationRule::InstrTGSMRaceCond);
+        }
+      }
+    }
+  }
+}
+
 static void ValidateGlobalVariables(ValidationContext &ValCtx) {
   DxilModule &M = ValCtx.DxilMod;
 
   unsigned TGSMSize = 0;
+  std::vector<Instruction*> fixAddrTGSMList;
   const DataLayout &DL = M.GetModule()->getDataLayout();
   for (GlobalVariable &GV : M.GetModule()->globals()) {
     ValidateGlobalVariable(GV, ValCtx);
     if (GV.getType()->getAddressSpace() == DXIL::kTGSMAddrSpace) {
       TGSMSize += DL.getTypeAllocSize(GV.getType()->getElementType());
+      CollectFixAddressAccess(&GV, fixAddrTGSMList);
     }
   }
 
@@ -2568,6 +2613,9 @@ static void ValidateGlobalVariables(ValidationContext &ValCtx) {
                            {std::to_string(TGSMSize).c_str(),
                             std::to_string(DXIL::kMaxTGSMSize).c_str()});
   }
+  if (!fixAddrTGSMList.empty()) {
+    ValidateTGSMRaceCondition(fixAddrTGSMList, ValCtx);
+  }
 }
 
 static void ValidateValidatorVersion(ValidationContext &ValCtx) {

tools/clang/test/CodeGenHLSL/RaceCond.hlsl

@@ -0,0 +1,15 @@
+// RUN: %dxc -E main  -T cs_6_0 %s | FileCheck %s
+
+// CHECK: Race condition writing to shared memory detected, consider making this write conditional
+
+RWBuffer< int > g_Intensities : register(u1);
+
+groupshared int sharedData;
+
+[ numthreads( 64, 2, 2 ) ]
+void main( uint GI : SV_GroupIndex)
+{
+sharedData = 0;
+InterlockedAdd(sharedData, g_Intensities[GI]);
+g_Intensities[GI] = sharedData;
+}

tools/clang/test/CodeGenHLSL/structInBuffer.hlsl

@@ -15,7 +15,9 @@ groupshared Foo sharedData;
 [ numthreads( 64, 2, 2 ) ]
 void main( uint GI : SV_GroupIndex)
 {
+   if (GI==0)
 	sharedData = inputs[GI];
+
 	int rtn;
 	InterlockedAdd(sharedData.d, g_Intensities[GI], rtn);
 	g_Intensities[GI] = rtn + sharedData.d;

tools/clang/test/CodeGenHLSL/structInBuffer2.hlsl

@@ -13,6 +13,7 @@ groupshared Foo sharedData;
 [ numthreads( 64, 2, 2 ) ]
 void main( uint GI : SV_GroupIndex)
 {
+  if (GI==0)
 	sharedData = inputs[GI];
 	int rtn;
 	InterlockedAdd(sharedData.d[0], g_Intensities[GI], rtn);

tools/clang/test/CodeGenHLSL/structInBuffer3.hlsl

@@ -14,12 +14,10 @@ RWBuffer< int > g_Intensities : register(u1);
 
 groupshared Foo sharedData;
 
-#ifdef DX12
-[RootSignature("DescriptorTable(UAV(u1, numDescriptors=1), SRV(t1), visibility=SHADER_VISIBILITY_ALL)")]
-#endif
 [ numthreads( 64, 2, 2 ) ]
 void main( uint GI : SV_GroupIndex)
 {
+    if(GI==0)
 	sharedData = inputs[GI];
 	int rtn;
 	InterlockedAdd(sharedData.d, g_Intensities[GI], rtn);

tools/clang/unittests/HLSL/ValidationTest.cpp

@@ -125,6 +125,8 @@ public:
   TEST_METHOD(I8Type)
   TEST_METHOD(EmptyStructInBuffer)
   TEST_METHOD(BigStructInBuffer)
+  TEST_METHOD(TGSMRaceCond)
+  TEST_METHOD(TGSMRaceCond2)
   TEST_METHOD(AddUint64Odd)
 
   TEST_METHOD(ClipCullMaxComponents)
@@ -1309,6 +1311,18 @@ TEST_F(ValidationTest, BigStructInBuffer) {
   TestCheck(L"..\\CodeGenHLSL\\BigStructInBuffer.hlsl");
 }
 
+TEST_F(ValidationTest, TGSMRaceCond) {
+  TestCheck(L"..\\CodeGenHLSL\\RaceCond.hlsl");
+}
+
+TEST_F(ValidationTest, TGSMRaceCond2) {
+    RewriteAssemblyCheckMsg(L"..\\CodeGenHLSL\\structInBuffer.hlsl", "cs_6_0",
+        "ret void",
+        "store i32 0, i32 addrspace(3)* @\"\\01?sharedData@@3UFoo@@A.3\", align 4\n"
+        "ret void",
+        "Race condition writing to shared memory detected, consider making this write conditional");
+}
+
 TEST_F(ValidationTest, AddUint64Odd) {
   TestCheck(L"..\\CodeGenHLSL\\AddUint64Odd.hlsl");
 }

utils/hct/hctdb.py

@@ -1562,6 +1562,7 @@ class db_dxil(object):
         self.add_valrule("Instr.CBufferClassForCBufferHandle", "Expect Cbuffer for CBufferLoad handle")
         self.add_valrule("Instr.FailToResloveTGSMPointer", "TGSM pointers must originate from an unambiguous TGSM global variable.")
         self.add_valrule("Instr.ExtractValue", "ExtractValue should only be used on dxil struct types and cmpxchg")
+        self.add_valrule("Instr.TGSMRaceCond", "Race condition writing to shared memory detected, consider making this write conditional")
 
         # Some legacy rules:
         # - space is only supported for shader targets 5.1 and higher