Address code review feedback

- Fix non-portable static_assert(false) to use sizeof(T)==0 idiom
- Replace placeholder TestId GUID with f00df946-9877-4453-8844-b1f4c8977953
- Extend isFloatingPointType to cover SNorm/UNorm/F8 wrapper types
- Rename ValidationConfig member to Validation to avoid type/member shadowing
- Rename local variables in dispatchTest to avoid type shadowing (CurOp, OpConfig)
- Add row-major assumption comment in doMatricesMatch
- Add MATRIX_LAYOUT comment explaining 0=RowMajor, 1=ColMajor
- Add TODO for non-square K dimension test coverage
- Clarify pre-staged input set comments in LinearAlgebraTestData.h
- Fix 'constuctor' typo in HLSLTestDataTypes.h

Co-authored-by: Copilot <[email protected]>

Author: alsepkow

tools/clang/unittests/HLSLExec/HLSLTestDataTypes.h

@@ -18,7 +18,7 @@
 namespace HLSLTestDataTypes {
 
 // A helper struct because C++ bools are 1 byte and HLSL bools are 4 bytes.
-// Take int32_t as a constuctor argument and convert it to bool when needed.
+// Take int32_t as a constructor argument and convert it to bool when needed.
 // Comparisons cast to a bool because we only care if the bool representation is
 // true or false.
 struct HLSLBool_t {
@@ -498,7 +498,11 @@ struct F8E5M2_t {
 
 template <typename T> constexpr bool isFloatingPointType() {
   return std::is_same_v<T, float> || std::is_same_v<T, double> ||
-         std::is_same_v<T, HLSLHalf_t>;
+         std::is_same_v<T, HLSLHalf_t> || std::is_same_v<T, SNormF16_t> ||
+         std::is_same_v<T, UNormF16_t> || std::is_same_v<T, SNormF32_t> ||
+         std::is_same_v<T, UNormF32_t> || std::is_same_v<T, SNormF64_t> ||
+         std::is_same_v<T, UNormF64_t> || std::is_same_v<T, F8E4M3_t> ||
+         std::is_same_v<T, F8E5M2_t>;
 }
 
 enum class ValidationType {
@@ -521,17 +525,17 @@ struct ValidationConfig {
 
 // Default validation: ULP for floating point, exact for integers.
 template <typename T> struct DefaultValidation {
-  ValidationConfig ValidationConfig;
+  ValidationConfig Validation;
 
   DefaultValidation() {
     if constexpr (isFloatingPointType<T>())
-      ValidationConfig = ValidationConfig::Ulp(1.0f);
+      Validation = ValidationConfig::Ulp(1.0f);
   }
 };
 
 // Strict validation: exact match by default.
 struct StrictValidation {
-  ValidationConfig ValidationConfig;
+  ValidationConfig Validation;
 };
 
 //

tools/clang/unittests/HLSLExec/LinearAlgebra.cpp

@@ -70,7 +70,7 @@ struct DataType {
 };
 
 template <typename T> const DataType &getDataType() {
-  static_assert(false && "Unknown data type");
+  static_assert(sizeof(T) == 0, "Unknown data type");
 }
 
 #define DATA_TYPE(TYPE, HLSL_STRING, COMP_TYPE, HLSL_SIZE, IS_16BIT)           \
@@ -118,6 +118,7 @@ bool doMatricesMatch(const std::vector<T> &Actual,
   for (size_t Index : MismatchedIndexes) {
     std::wstringstream Wss(L"");
     Wss << std::setprecision(15);
+    // Assumes row-major layout for (row,col) decomposition.
     Wss << L"Mismatch at (" << Index / N << L"," << Index % N << L")";
     Wss << L" Actual:" << Actual[Index];
     Wss << L" Expected:" << Expected[Index];
@@ -162,7 +163,7 @@ std::string getCompilerOptionsString(const Operation &Op,
   if (KDim > 0)
     Options << " -DK_DIM=" << KDim;
 
-  Options << " -DMATRIX_LAYOUT=0";
+  Options << " -DMATRIX_LAYOUT=0"; // 0 = RowMajor, 1 = ColMajor
 
   return Options.str();
 }
@@ -416,26 +417,28 @@ template <typename T, OpType OP>
 void dispatchTest(ID3D12Device *D3DDevice, bool VerboseLogging) {
 
   const std::vector<MatrixDims> Sizes = getMatrixSizesToTest();
-  constexpr const Operation &Operation = getOperation(OP);
-  Op<OP, T> Op;
+  constexpr const Operation &CurOp = getOperation(OP);
+  Op<OP, T> OpConfig;
 
   for (const MatrixDims &Dims : Sizes) {
     const size_t Rows = Dims.Rows;
     const size_t Cols = Dims.Cols;
-    const size_t KDim = (Operation.Arity >= 2) ? Cols : 0;
+    // TODO: K dimension currently equals Cols for simplicity (square inner
+    // dimension). Add non-square K sizes for better multiply coverage.
+    const size_t KDim = (CurOp.Arity >= 2) ? Cols : 0;
 
     // FillMatrix has special input handling (scalar, not a matrix).
     InputSets<T> Inputs;
     if constexpr (OP == OpType::FillMatrix)
-      Inputs = ExpectedBuilder<OP, T>::buildInputs(Operation, Rows, Cols, KDim);
+      Inputs = ExpectedBuilder<OP, T>::buildInputs(CurOp, Rows, Cols, KDim);
     else
-      Inputs = buildTestInputs<T>(Operation, Rows, Cols, KDim);
+      Inputs = buildTestInputs<T>(CurOp, Rows, Cols, KDim);
 
-    auto Expected =
-        ExpectedBuilder<OP, T>::buildExpected(Op, Inputs, Rows, Cols, KDim);
+    auto Expected = ExpectedBuilder<OP, T>::buildExpected(OpConfig, Inputs,
+                                                          Rows, Cols, KDim);
 
-    runAndVerify(D3DDevice, VerboseLogging, Operation, Inputs, Expected,
-                 Op.ValidationConfig, Rows, Cols, KDim);
+    runAndVerify(D3DDevice, VerboseLogging, CurOp, Inputs, Expected,
+                 OpConfig.Validation, Rows, Cols, KDim);
   }
 }
 
@@ -535,7 +538,7 @@ class DxilConf_SM610_LinearAlgebra : public LinAlgTestClassCommon {
   BEGIN_TEST_CLASS(DxilConf_SM610_LinearAlgebra)
   TEST_CLASS_PROPERTY("Kits.TestName",
                       "D3D12 - Shader Model 6.10 - Linear Algebra Tests")
-  TEST_CLASS_PROPERTY("Kits.TestId", "a1b2c3d4-e5f6-7890-abcd-ef1234567890")
+  TEST_CLASS_PROPERTY("Kits.TestId", "f00df946-9877-4453-8844-b1f4c8977953")
   TEST_CLASS_PROPERTY("Kits.Description",
                       "Validates SM 6.10 linear algebra matrix operations")
   TEST_CLASS_PROPERTY(

tools/clang/unittests/HLSLExec/LinearAlgebraTestData.h

@@ -86,7 +86,8 @@ INPUT_SET(InputSet::Fill, 42)
 INPUT_SET(InputSet::Identity, 1)
 END_INPUT_SETS()
 
-// --- Additional scalar types ---
+// --- Additional scalar types (pre-staged for upcoming SM 6.10 ComponentTypes)
+// ---
 
 BEGIN_INPUT_SETS(int8_t)
 INPUT_SET(InputSet::Seed, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14)
@@ -124,7 +125,7 @@ INPUT_SET(InputSet::Fill, 42)
 INPUT_SET(InputSet::Identity, 1)
 END_INPUT_SETS()
 
-// --- Normalized types (SNorm [-1,1], UNorm [0,1]) ---
+// --- Normalized types (pre-staged for SM 6.10 SNorm/UNorm ComponentTypes) ---
 
 BEGIN_INPUT_SETS(SNormF16_t)
 INPUT_SET(InputSet::Seed, SNormF16_t(HLSLHalf_t(-0.9f)),
@@ -190,7 +191,7 @@ INPUT_SET(InputSet::Fill, UNormF64_t(0.5))
 INPUT_SET(InputSet::Identity, UNormF64_t(1.0))
 END_INPUT_SETS()
 
-// --- FP8 types (packed 4 elements per scalar in HLSL) ---
+// --- FP8 types (pre-staged for SM 6.10 packed ComponentTypes) ---
 
 BEGIN_INPUT_SETS(F8E4M3_t)
 INPUT_SET(InputSet::Seed, F8E4M3_t(1.0f), F8E4M3_t(1.5f), F8E4M3_t(2.0f),

tools/clang/unittests/HLSLExec/LongVectors.cpp

@@ -1170,7 +1170,7 @@ template <typename T> struct ExpectedBuilder<OpType::Dot, T> {
       AbsoluteEpsilon +=
           computeAbsoluteEpsilon<T>((SumPos + SumNeg), ULPTolerance);
 
-    Op.ValidationConfig = ValidationConfig::Epsilon(AbsoluteEpsilon);
+    Op.Validation = ValidationConfig::Epsilon(AbsoluteEpsilon);
 
     std::vector<T> Expected;
     Expected.push_back(static_cast<T>(DotProduct));
@@ -1683,7 +1683,7 @@ void dispatchTest(ID3D12Device *D3DDevice, bool VerboseLogging,
     auto Expected = ExpectedBuilder<OP, T>::buildExpected(Op, Inputs);
 
     runAndVerify(D3DDevice, VerboseLogging, Operation, Inputs, Expected,
-                 Op.ValidationConfig);
+                 Op.Validation);
   }
 }
 
@@ -1708,7 +1708,7 @@ void dispatchWaveOpTest(ID3D12Device *D3DDevice, bool VerboseLogging,
     auto Expected = ExpectedBuilder<OP, T>::buildExpected(Op, Inputs, WaveSize);
 
     runAndVerify(D3DDevice, VerboseLogging, Operation, Inputs, Expected,
-                 Op.ValidationConfig, AdditionalCompilerOptions);
+                 Op.Validation, AdditionalCompilerOptions);
   }
 }