[HLSL][RootSignature] Add basic parameter validations of Root Elements

clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -13091,6 +13091,9 @@ def err_invalid_hlsl_resource_type: Error<
 def err_hlsl_spirv_only: Error<"%0 is only available for the SPIR-V target">;
 def err_hlsl_vk_literal_must_contain_constant: Error<"the argument to vk::Literal must be a vk::integral_constant">;
 
+def err_hlsl_invalid_rootsig_value : Error<"value must be in the range [%0, %1]">;
+def err_hlsl_invalid_rootsig_flag : Error< "invalid flags for version 1.%0">;
+
 def subst_hlsl_format_ranges: TextSubstitution<
   "%select{t|u|b|s}0[%1;%select{%3]|unbounded)}2">;
 

clang/lib/Sema/SemaHLSL.cpp

@@ -43,7 +43,9 @@
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/DXILABI.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/TargetParser/Triple.h"
+#include <cmath>
 #include <cstddef>
 #include <iterator>
 #include <utility>
@@ -1083,6 +1085,92 @@ void SemaHLSL::ActOnFinishRootSignatureDecl(
 
 bool SemaHLSL::handleRootSignatureElements(
     ArrayRef<hlsl::RootSignatureElement> Elements) {
+  // Define some common error handling functions
+  bool HadError = false;
+  auto ReportError = [this, &HadError](SourceLocation Loc, uint32_t LowerBound,
+                                       uint32_t UpperBound) {
+    HadError = true;
+    this->Diag(Loc, diag::err_hlsl_invalid_rootsig_value)
+        << LowerBound << UpperBound;
+  };
+
+  auto ReportFloatError = [this, &HadError](SourceLocation Loc,
+                                            float LowerBound,
+                                            float UpperBound) {
+    HadError = true;
+    this->Diag(Loc, diag::err_hlsl_invalid_rootsig_value)
+        << llvm::formatv("{0:f}", LowerBound).sstr<6>()
+        << llvm::formatv("{0:f}", UpperBound).sstr<6>();
+  };
+
+  auto VerifyRegister = [ReportError](SourceLocation Loc, uint32_t Register) {
+    if (!llvm::hlsl::rootsig::verifyRegisterValue(Register))
+      ReportError(Loc, 0, 0xfffffffe);
+  };
+
+  auto VerifySpace = [ReportError](SourceLocation Loc, uint32_t Space) {
+    if (!llvm::hlsl::rootsig::verifyRegisterSpace(Space))
+      ReportError(Loc, 0, 0xffffffef);
+  };
+
+  const uint32_t Version =
+      llvm::to_underlying(SemaRef.getLangOpts().HLSLRootSigVer);
+  const uint32_t VersionEnum = Version - 1;
+  auto ReportFlagError = [this, &HadError, VersionEnum](SourceLocation Loc) {
+    HadError = true;
+    this->Diag(Loc, diag::err_hlsl_invalid_rootsig_flag)
+        << /*version minor*/ VersionEnum;
+  };
+
+  // Iterate through the elements and do basic validations
+  for (const hlsl::RootSignatureElement &RootSigElem : Elements) {
+    SourceLocation Loc = RootSigElem.getLocation();
+    const llvm::hlsl::rootsig::RootElement &Elem = RootSigElem.getElement();
+    if (const auto *Descriptor =
+            std::get_if<llvm::hlsl::rootsig::RootDescriptor>(&Elem)) {
+      VerifyRegister(Loc, Descriptor->Reg.Number);
+      VerifySpace(Loc, Descriptor->Space);
+
+      if (!llvm::hlsl::rootsig::verifyRootDescriptorFlag(
+              Version, llvm::to_underlying(Descriptor->Flags)))
+        ReportFlagError(Loc);
+    } else if (const auto *Constants =
+                   std::get_if<llvm::hlsl::rootsig::RootConstants>(&Elem)) {
+      VerifyRegister(Loc, Constants->Reg.Number);
+      VerifySpace(Loc, Constants->Space);
+    } else if (const auto *Sampler =
+                   std::get_if<llvm::hlsl::rootsig::StaticSampler>(&Elem)) {
+      VerifyRegister(Loc, Sampler->Reg.Number);
+      VerifySpace(Loc, Sampler->Space);
+
+      assert(!std::isnan(Sampler->MaxLOD) && !std::isnan(Sampler->MinLOD) &&
+             "By construction, parseFloatParam can't produce a NaN from a "
+             "float_literal token");
+
+      if (!llvm::hlsl::rootsig::verifyMaxAnisotropy(Sampler->MaxAnisotropy))
+        ReportError(Loc, 0, 16);
+      if (!llvm::hlsl::rootsig::verifyMipLODBias(Sampler->MipLODBias))
+        ReportFloatError(Loc, -16.f, 15.99);
+    } else if (const auto *Clause =
+                   std::get_if<llvm::hlsl::rootsig::DescriptorTableClause>(
+                       &Elem)) {
+      VerifyRegister(Loc, Clause->Reg.Number);
+      VerifySpace(Loc, Clause->Space);
+
+      if (!llvm::hlsl::rootsig::verifyNumDescriptors(Clause->NumDescriptors)) {
+        // NumDescriptor could techincally be ~0u but that is reserved for
+        // unbounded, so the diagnostic will not report that as a valid int
+        // value
+        ReportError(Loc, 1, 0xfffffffe);
+      }
+
+      if (!llvm::hlsl::rootsig::verifyDescriptorRangeFlag(
+              Version, llvm::to_underlying(Clause->Type),
+              llvm::to_underlying(Clause->Flags)))
+        ReportFlagError(Loc);
+    }
+  }
+
   using RangeInfo = llvm::hlsl::rootsig::RangeInfo;
   using OverlappingRanges = llvm::hlsl::rootsig::OverlappingRanges;
   using InfoPairT = std::pair<RangeInfo, const hlsl::RootSignatureElement *>;
@@ -1130,7 +1218,10 @@ bool SemaHLSL::handleRootSignatureElements(
                        &Elem)) {
       RangeInfo Info;
       Info.LowerBound = Clause->Reg.Number;
-      assert(0 < Clause->NumDescriptors && "Verified as part of TODO(#129940)");
+      // Relevant error will have already been reported above and needs to be
+      // fixed before we can conduct range analysis, so shortcut error return
+      if (Clause->NumDescriptors == 0)
+        return true;
       Info.UpperBound = Clause->NumDescriptors == RangeInfo::Unbounded
                             ? RangeInfo::Unbounded
                             : Info.LowerBound + Clause->NumDescriptors -

clang/test/AST/HLSL/RootSignatures-AST.hlsl

@@ -13,14 +13,14 @@
 
 #define SampleRS "RootFlags( ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT | " \
                              "DENY_VERTEX_SHADER_ROOT_ACCESS), " \
-                 "CBV(b0, space = 1, flags = DATA_STATIC), " \
+                 "CBV(b0, space = 1, flags = DATA_VOLATILE), " \
                  "SRV(t0), " \
                  "UAV(u0), " \
                  "DescriptorTable( CBV(b1), " \
                                    "SRV(t1, numDescriptors = 8, " \
-                                   "        flags = DESCRIPTORS_VOLATILE), " \
+                                   "        flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE), " \
                                    "UAV(u1, numDescriptors = unbounded, " \
-                                   "        flags = DESCRIPTORS_VOLATILE)), " \
+                                   "        flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE)), " \
                  "DescriptorTable(Sampler(s0, space=1, numDescriptors = 4)), " \
                  "RootConstants(num32BitConstants=3, b10), " \
                  "StaticSampler(s1)," \
@@ -34,7 +34,7 @@
 // CHECK-SAME: RootElements{
 // CHECK-SAME: RootFlags(AllowInputAssemblerInputLayout | DenyVertexShaderRootAccess),
 // CHECK-SAME: RootCBV(b0,
-// CHECK-SAME:   space = 1, visibility = All, flags = DataStatic
+// CHECK-SAME:   space = 1, visibility = All, flags = DataVolatile
 // CHECK-SAME: ),
 // CHECK-SAME: RootSRV(t0,
 // CHECK-SAME:   space = 0, visibility = All,
@@ -50,10 +50,10 @@
 // CHECK-V1_1-SAME: flags = DataStaticWhileSetAtExecute
 // CHECK-SAME: ),
 // CHECK-SAME: SRV(
-// CHECK-SAME:   t1, numDescriptors = 8, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile
+// CHECK-SAME:   t1, numDescriptors = 8, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile | DataVolatile
 // CHECK-SAME: ),
 // CHECK-SAME: UAV(
-// CHECK-SAME:   u1, numDescriptors = unbounded, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile
+// CHECK-SAME:   u1, numDescriptors = unbounded, space = 0, offset = DescriptorTableOffsetAppend, flags = DescriptorsVolatile | DataVolatile
 // CHECK-SAME: ),
 // CHECK-SAME: DescriptorTable(
 // CHECK-SAME:   numClauses = 3, visibility = All
@@ -92,14 +92,14 @@ void same_rs_main() {}
 #define SampleSameRS \
    "RootFlags( ALLOW_INPUT_ASSEMBLER_INPUT_LAYOUT | " \
                "DENY_VERTEX_SHADER_ROOT_ACCESS), " \
-   "CBV(b0, space = 1, flags = DATA_STATIC), " \
+   "CBV(b0, space = 1, flags = DATA_VOLATILE), " \
    "SRV(t0), " \
    "UAV(u0), " \
    "DescriptorTable( CBV(b1), " \
-                     "SRV(t1, numDescriptors = 8, " \
-                     "        flags = DESCRIPTORS_VOLATILE), " \
-                     "UAV(u1, numDescriptors = unbounded, " \
-                     "        flags = DESCRIPTORS_VOLATILE)), " \
+   "  SRV(t1, numDescriptors = 8, " \
+   "          flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE), " \
+   "  UAV(u1, numDescriptors = unbounded, " \
+   "          flags = DATA_VOLATILE | DESCRIPTORS_VOLATILE)), " \
    "DescriptorTable(Sampler(s0, space=1, numDescriptors = 4)), " \
    "RootConstants(num32BitConstants=3, b10), " \
    "StaticSampler(s1)," \

clang/test/CodeGenHLSL/RootSignature.hlsl

@@ -76,7 +76,8 @@ void RootDescriptorsEntry() {}
 // CHECK-SAME: i32 2, i32 3, i32 5,
 
 // checking mipLODBias, maxAnisotropy, comparisonFunc, borderColor
-// CHECK-SAME: float 0x40403999A0000000, i32 9, i32 3, i32 2,
+// note: the hex value is the float bit representation of 12.45
+// CHECK-SAME: float 0x4028E66660000000, i32 9, i32 3, i32 2,
 
 // checking minLOD, maxLOD
 // CHECK-SAME: float -1.280000e+02, float 1.280000e+02,
@@ -90,7 +91,7 @@ void RootDescriptorsEntry() {}
   " addressU = TEXTURE_ADDRESS_MIRROR, " \
   " addressV = TEXTURE_ADDRESS_CLAMP, " \
   " addressW = TEXTURE_ADDRESS_MIRRORONCE, " \
-  " mipLODBias = 32.45f, maxAnisotropy = 9, " \
+  " mipLODBias = 12.45f, maxAnisotropy = 9, " \
   " comparisonFunc = COMPARISON_EQUAL, " \
   " borderColor = STATIC_BORDER_COLOR_OPAQUE_WHITE, " \
   " minLOD = -128.f, maxLOD = 128.f, " \

clang/test/SemaHLSL/RootSignature-err.hlsl

@@ -103,3 +103,39 @@ void bad_root_signature_22() {}
 // expected-error@+1 {{invalid value of RootFlags}}
 [RootSignature("RootFlags(local_root_signature | root_flag_typo)")]
 void bad_root_signature_23() {}
+
+// Basic validation of register value and space
+
+// expected-error@+2 {{value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{value must be in the range [0, 4294967279]}}
+[RootSignature("CBV(b4294967295, space = 4294967280)")]
+void basic_validation_0() {}
+
+// expected-error@+2 {{value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{value must be in the range [0, 4294967279]}}
+[RootSignature("RootConstants(b4294967295, space = 4294967280, num32BitConstants = 1)")]
+void basic_validation_1() {}
+
+// expected-error@+2 {{value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{value must be in the range [0, 4294967279]}}
+[RootSignature("StaticSampler(s4294967295, space = 4294967280)")]
+void basic_validation_2() {}
+
+// expected-error@+2 {{value must be in the range [0, 4294967294]}}
+// expected-error@+1 {{value must be in the range [0, 4294967279]}}
+[RootSignature("DescriptorTable(SRV(t4294967295, space = 4294967280))")]
+void basic_validation_3() {}
+
+// expected-error@+2 {{value must be in the range [1, 4294967294]}}
+// expected-error@+1 {{value must be in the range [1, 4294967294]}}
+[RootSignature("DescriptorTable(UAV(u0, numDescriptors = 0), Sampler(s0, numDescriptors = 0))")]
+void basic_validation_4() {}
+
+// expected-error@+2 {{value must be in the range [0, 16]}}
+// expected-error@+1 {{value must be in the range [-16.00, 15.99]}}
+[RootSignature("StaticSampler(s0, maxAnisotropy = 17, mipLODBias = -16.000001)")]
+void basic_validation_5() {}
+
+// expected-error@+1 {{value must be in the range [-16.00, 15.99]}}
+[RootSignature("StaticSampler(s0, mipLODBias = 15.990001)")]
+void basic_validation_6() {}

clang/test/SemaHLSL/RootSignature-flags-err.hlsl

@@ -0,0 +1,57 @@
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only \
+// RUN:   -fdx-rootsignature-version=rootsig_1_0 %s -verify=v10
+// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library -x hlsl -fsyntax-only \
+// RUN:   -fdx-rootsignature-version=rootsig_1_1 %s -verify=v11
+
+// Root Descriptor Flags:
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("CBV(b0, flags = DATA_STATIC)")]
+void bad_root_descriptor_flags_0() {}
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("CBV(b0, flags = DATA_STATIC_WHILE_SET_AT_EXECUTE)")]
+void bad_root_descriptor_flags_1() {}
+
+// v10-error@+2 {{invalid flags for version 1.0}}
+// v11-error@+1 {{invalid flags for version 1.1}}
+[RootSignature("CBV(b0, flags = DATA_STATIC | DATA_VOLATILE)")]
+void bad_root_descriptor_flags_2() {}
+
+// Descriptor Range Flags:
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DATA_VOLATILE))")]
+void bad_descriptor_range_flags_0() {}
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC))")]
+void bad_descriptor_range_flags_1() {}
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC_WHILE_SET_AT_EXECUTE | DESCRIPTORS_VOLATILE))")]
+void bad_descriptor_range_flags_2() {}
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE))")]
+void bad_descriptor_range_flags_3() {}
+
+// v10-error@+1 {{invalid flags for version 1.0}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS))")]
+void bad_descriptor_range_flags_4() {}
+
+// v10-error@+2 {{invalid flags for version 1.0}}
+// v11-error@+1 {{invalid flags for version 1.1}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DATA_STATIC | DATA_STATIC_WHILE_SET_AT_EXECUTE))")]
+void bad_descriptor_range_flags_5() {}
+
+// v10-error@+2 {{invalid flags for version 1.0}}
+// v11-error@+1 {{invalid flags for version 1.1}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE | DESCRIPTORS_STATIC_KEEPING_BUFFER_BOUNDS_CHECKS))")]
+void bad_descriptor_range_flags_6() {}
+
+// v10-error@+2 {{invalid flags for version 1.0}}
+// v11-error@+1 {{invalid flags for version 1.1}}
+[RootSignature("DescriptorTable(CBV(b0, flags = DESCRIPTORS_VOLATILE | DATA_STATIC))")]
+void bad_descriptor_range_flags_7() {}
+