Clarify assignment of undefined value to UAV (#2941)

The validation error message for when one or more of the components of a
storeBuffer (or similar) operation are undefined was terribly confusing
to anyone not developing the compiler. It mentioned mask mismatches when
one of the masks was generated based on which operands were undefined.

This changes the error message when the flaw results from the user under-
specifying the copied variable to simply state that the assignment involves
undefined values. If the masks mismatch because the type-based mask didn't
expect values that the user somehow defined, the original error message
results as this is likely a compiler error.

In addition, this consolidates some of the mask checking for rawbuffer,
texture, and generic buffer storage.

docs/DXIL.rst

@@ -3064,6 +3064,7 @@ INSTR.STATUS                              Resource status should only be used by
 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.UNDEFINEDVALUEFORUAVSTORE           Assignment of undefined values to UAV.
 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

@@ -136,6 +136,7 @@ enum class ValidationRule : unsigned {
   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.
+  InstrUndefinedValueForUAVStore, // Assignment of undefined values to UAV.
   InstrWriteMaskForTypedUAVStore, // store on typed uav must write to all four components of the UAV.
   InstrWriteMaskMatchValueForUAVStore, // uav store write mask must match store value mask, write mask is %0 and store value mask is %1.
 

lib/HLSL/DxilValidation.cpp

@@ -163,6 +163,7 @@ const char *hlsl::GetValidationRuleText(ValidationRule value) {
     case hlsl::ValidationRule::InstrResourceKindForSampleC: return "samplec requires resource declared as texture1D/2D/Cube/1DArray/2DArray/CubeArray.";
     case hlsl::ValidationRule::InstrResourceKindForGather: return "gather requires resource declared as texture/2D/Cube/2DArray/CubeArray.";
     case hlsl::ValidationRule::InstrWriteMaskMatchValueForUAVStore: return "uav store write mask must match store value mask, write mask is %0 and store value mask is %1.";
+    case hlsl::ValidationRule::InstrUndefinedValueForUAVStore: return "Assignment of undefined values to UAV.";
     case hlsl::ValidationRule::InstrResourceKindForBufferLoadStore: return "buffer load/store only works on Raw/Typed/StructuredBuffer.";
     case hlsl::ValidationRule::InstrResourceKindForTextureStore: return "texture store only works on Texture1D/1DArray/2D/2DArray/3D.";
     case hlsl::ValidationRule::InstrResourceKindForGetDim: return "Invalid resource kind on GetDimensions.";
@@ -1773,6 +1774,33 @@ static void ValidateImmOperandForMathDxilOp(CallInst *CI, DXIL::OpCode opcode,
   }
 }
 
+// Validate the type-defined mask compared to the store value mask which indicates which parts were defined
+// returns true if caller should continue validation
+static bool ValidateStorageMasks(Instruction *I, DXIL::OpCode opcode, ConstantInt *mask,
+                                 unsigned stValMask, bool isTyped, ValidationContext &ValCtx) {
+  if (!mask) {
+    // Mask for buffer store should be immediate.
+    ValCtx.EmitInstrFormatError(I, ValidationRule::InstrOpConst,
+                                {"Mask", hlsl::OP::GetOpCodeName(opcode)});
+    return false;
+  }
+
+  unsigned uMask = mask->getLimitedValue();
+  if (isTyped && uMask != 0xf) {
+    ValCtx.EmitInstrError(I, ValidationRule::InstrWriteMaskForTypedUAVStore);
+  }
+
+  // If a bit is set in the uMask (expected values) that isn't set in stValMask (user provided values)
+  // then the user failed to define some of the output values.
+  if (uMask & ~stValMask)
+    ValCtx.EmitInstrError(I, ValidationRule::InstrUndefinedValueForUAVStore);
+  else if (uMask != stValMask)
+    ValCtx.EmitInstrFormatError(I, ValidationRule::InstrWriteMaskMatchValueForUAVStore,
+                                {std::to_string(uMask), std::to_string(stValMask)});
+
+  return true;
+}
+
 static void ValidateResourceDxilOp(CallInst *CI, DXIL::OpCode opcode,
                                    ValidationContext &ValCtx) {
   switch (opcode) {
@@ -2012,23 +2040,14 @@ static void ValidateResourceDxilOp(CallInst *CI, DXIL::OpCode opcode,
     }
 
     ConstantInt *mask = dyn_cast<ConstantInt>(bufSt.get_mask());
-    if (!mask) {
-      // Mask for buffer store should be immediate.
-      ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrOpConst,
-                                  {"Mask", "BufferStore"});
-      return;
-    }
-    unsigned uMask = mask->getLimitedValue();
     unsigned stValMask =
         StoreValueToMask({bufSt.get_value0(), bufSt.get_value1(),
                           bufSt.get_value2(), bufSt.get_value3()});
 
-    if (stValMask != uMask) {
-      ValCtx.EmitInstrFormatError(
-          CI, ValidationRule::InstrWriteMaskMatchValueForUAVStore,
-          {std::to_string(uMask), std::to_string(stValMask)});
-    }
-
+    if (!ValidateStorageMasks(CI, opcode, mask, stValMask,
+                         resKind == DXIL::ResourceKind::TypedBuffer || resKind == DXIL::ResourceKind::TBuffer,
+                             ValCtx))
+      return;
     Value *offset = bufSt.get_coord1();
 
     switch (resKind) {
@@ -2044,11 +2063,6 @@ static void ValidateResourceDxilOp(CallInst *CI, DXIL::OpCode opcode,
         ValCtx.EmitInstrError(
             CI, ValidationRule::InstrCoordinateCountForRawTypedBuf);
       }
-
-      if (uMask != 0xf) {
-        ValCtx.EmitInstrError(CI,
-                              ValidationRule::InstrWriteMaskForTypedUAVStore);
-      }
       break;
     case DXIL::ResourceKind::StructuredBuffer:
     case DXIL::ResourceKind::StructuredBufferWithCounter:
@@ -2076,26 +2090,12 @@ static void ValidateResourceDxilOp(CallInst *CI, DXIL::OpCode opcode,
     }
 
     ConstantInt *mask = dyn_cast<ConstantInt>(texSt.get_mask());
-    if (!mask) {
-      // Mask for buffer store should be immediate.
-      ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrOpConst,
-                                  {"Mask", "TextureStore"});
-      return;
-    }
-    unsigned uMask = mask->getLimitedValue();
-    if (uMask != 0xf) {
-      ValCtx.EmitInstrError(CI, ValidationRule::InstrWriteMaskForTypedUAVStore);
-    }
-
     unsigned stValMask =
         StoreValueToMask({texSt.get_value0(), texSt.get_value1(),
                           texSt.get_value2(), texSt.get_value3()});
 
-    if (stValMask != uMask) {
-      ValCtx.EmitInstrFormatError(
-          CI, ValidationRule::InstrWriteMaskMatchValueForUAVStore,
-          {std::to_string(uMask), std::to_string(stValMask)});
-    }
+    if (!ValidateStorageMasks(CI, opcode, mask, stValMask, true /*isTyped*/, ValCtx))
+      return;
 
     switch (resKind) {
     case DXIL::ResourceKind::Texture1D:
@@ -2282,22 +2282,12 @@ static void ValidateResourceDxilOp(CallInst *CI, DXIL::OpCode opcode,
     }
 
     ConstantInt *mask = dyn_cast<ConstantInt>(bufSt.get_mask());
-    if (!mask) {
-      // Mask for buffer store should be immediate.
-      ValCtx.EmitInstrFormatError(CI, ValidationRule::InstrOpConst,
-                                  {"Mask", "BufferStore"});
-      return;
-    }
-    unsigned uMask = mask->getLimitedValue();
     unsigned stValMask =
         StoreValueToMask({bufSt.get_value0(), bufSt.get_value1(),
                           bufSt.get_value2(), bufSt.get_value3()});
 
-    if (stValMask != uMask) {
-      ValCtx.EmitInstrFormatError(
-          CI, ValidationRule::InstrWriteMaskMatchValueForUAVStore,
-          {std::to_string(uMask), std::to_string(stValMask)});
-    }
+    if (!ValidateStorageMasks(CI, opcode, mask, stValMask, false /*isTyped*/, ValCtx))
+      return;
 
     Value *offset = bufSt.get_elementOffset();
     Value *align = bufSt.get_alignment();

tools/clang/test/CodeGenHLSL/uav_store.hlsl

@@ -0,0 +1,9 @@
+// RUN: %dxc -E main -T ps_6_0 %s
+
+RWByteAddressBuffer uav1;
+
+float4 main(uint2 a : A, uint2 b : B) : SV_Target
+{
+  uav1.Store4(0, 2);
+  return 0;
+}

tools/clang/test/HLSLFileCheck/validation/UndefStore.hlsl

@@ -0,0 +1,19 @@
+// RUN: %dxc -Zi -E main -T cs_6_0 %s | FileCheck %s -check-prefix=CHECK -check-prefix=CHK_DB
+// RUN: %dxc -E main -T cs_6_0 %s | FileCheck %s -check-prefix=CHECK -check-prefix=CHK_NODB
+
+// CHK_DB: 18:17: error: Assignment of undefined values to UAV.
+// CHK_NODB: Function: main: error: Assignment of undefined values to UAV. Use /Zi for source location.
+
+RWBuffer<uint> output;
+
+uint Add(uint a, uint b)
+{
+	return a + b;
+}
+
+[numthreads(64,1,1)]
+void main(uint3 DTid : SV_DispatchThreadID)
+{
+	uint sum = Add(sum, (uint)DTid.x);	// Deliberate use of uninitialised variable 'sum'
+	output[DTid.x] = sum;
+}

tools/clang/unittests/HLSL/ValidationTest.cpp

@@ -71,6 +71,7 @@ public:
   TEST_METHOD(SignatureStreamIDForNonGS)
   TEST_METHOD(TypedUAVStoreFullMask0)
   TEST_METHOD(TypedUAVStoreFullMask1)
+  TEST_METHOD(UAVStoreMaskMatch)
   TEST_METHOD(Recursive)
   TEST_METHOD(Recursive2)
   TEST_METHOD(Recursive3)
@@ -1157,6 +1158,14 @@ TEST_F(ValidationTest, TypedUAVStoreFullMask1) {
       "Mask of BufferStore must be an immediate constant");
 }
 
+TEST_F(ValidationTest, UAVStoreMaskMatch) {
+  RewriteAssemblyCheckMsg(
+      L"..\\CodeGenHLSL\\uav_store.hlsl", "ps_6_0",
+      "i32 2, i8 15)",
+      "i32 2, i8 7)",
+      "uav store write mask must match store value mask, write mask is 7 and store value mask is 15.");
+}
+
 TEST_F(ValidationTest, Recursive) {
   // Includes coverage for user-defined functions.
   TestCheck(L"..\\CodeGenHLSL\\recursive.ll");

utils/hct/hctdb.py

@@ -2477,6 +2477,7 @@ class db_dxil(object):
         self.add_valrule("Instr.ResourceKindForSampleC", "samplec requires resource declared as texture1D/2D/Cube/1DArray/2DArray/CubeArray.")
         self.add_valrule("Instr.ResourceKindForGather", "gather requires resource declared as texture/2D/Cube/2DArray/CubeArray.")
         self.add_valrule("Instr.WriteMaskMatchValueForUAVStore", "uav store write mask must match store value mask, write mask is %0 and store value mask is %1.")
+        self.add_valrule("Instr.UndefinedValueForUAVStore", "Assignment of undefined values to UAV.")
         self.add_valrule("Instr.ResourceKindForBufferLoadStore", "buffer load/store only works on Raw/Typed/StructuredBuffer.")
         self.add_valrule("Instr.ResourceKindForTextureStore", "texture store only works on Texture1D/1DArray/2D/2DArray/3D.")
         self.add_valrule("Instr.ResourceKindForGetDim", "Invalid resource kind on GetDimensions.")