[HLSL][RootSignature] Add parsing of Register in params for RootDescriptors #140148

inbelic · 2025-05-15T22:01:12Z

defines the parseRootDescriptorParams infrastructure for parsing the params of a RootDescriptor
defines the register type to illustrate use
add tests to demonstrate functionality

Part 2 of #126577

llvmbot · 2025-05-15T22:01:47Z

@llvm/pr-subscribers-clang

Author: Finn Plummer (inbelic)

Changes

defines the parseRootParamParams infastructure for parsing the params of a RootParam
defines the register type to illustrate use
add tests to demonstrate functionality

Part 2 of #126577

Full diff: https://github.com/llvm/llvm-project/pull/140148.diff

4 Files Affected:

(modified) clang/include/clang/Parse/ParseHLSLRootSignature.h (+6)
(modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+42)
(modified) clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp (+9-4)
(modified) llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h (+1)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index b96416ab6c6c4..92b8e7389426a 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -89,6 +89,12 @@ class RootSignatureParser {
   };
   std::optional<ParsedConstantParams> parseRootConstantParams();
 
+  struct ParsedRootParamParams {
+    std::optional<llvm::hlsl::rootsig::Register> Reg;
+  };
+  std::optional<ParsedRootParamParams>
+  parseRootParamParams(RootSignatureToken::Kind RegType);
+
   struct ParsedClauseParams {
     std::optional<llvm::hlsl::rootsig::Register> Reg;
     std::optional<uint32_t> NumDescriptors;
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 3ed60442eaa14..de7c2452b3fd4 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -176,20 +176,37 @@ std::optional<RootParam> RootSignatureParser::parseRootParam() {
     return std::nullopt;
 
   RootParam Param;
+  TokenKind ExpectedReg;
   switch (ParamKind) {
   default:
     llvm_unreachable("Switch for consumed token was not provided");
   case TokenKind::kw_CBV:
     Param.Type = ParamType::CBuffer;
+    ExpectedReg = TokenKind::bReg;
     break;
   case TokenKind::kw_SRV:
     Param.Type = ParamType::SRV;
+    ExpectedReg = TokenKind::tReg;
     break;
   case TokenKind::kw_UAV:
     Param.Type = ParamType::UAV;
+    ExpectedReg = TokenKind::uReg;
     break;
   }
 
+  auto Params = parseRootParamParams(ExpectedReg);
+  if (!Params.has_value())
+    return std::nullopt;
+
+  // Check mandatory parameters were provided
+  if (!Params->Reg.has_value()) {
+    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+        << ExpectedReg;
+    return std::nullopt;
+  }
+
+  Param.Reg = Params->Reg.value();
+
   if (consumeExpectedToken(TokenKind::pu_r_paren,
                            diag::err_hlsl_unexpected_end_of_params,
                            /*param of=*/TokenKind::kw_RootConstants))
@@ -398,6 +415,31 @@ RootSignatureParser::parseRootConstantParams() {
   return Params;
 }
 
+std::optional<RootSignatureParser::ParsedRootParamParams>
+RootSignatureParser::parseRootParamParams(TokenKind RegType) {
+  assert(CurToken.TokKind == TokenKind::pu_l_paren &&
+         "Expects to only be invoked starting at given token");
+
+  ParsedRootParamParams Params;
+  do {
+    // ( `b` | `t` | `u`) POS_INT
+    if (tryConsumeExpectedToken(RegType)) {
+      if (Params.Reg.has_value()) {
+        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+            << CurToken.TokKind;
+        return std::nullopt;
+      }
+      auto Reg = parseRegister();
+      if (!Reg.has_value())
+        return std::nullopt;
+      Params.Reg = Reg;
+    }
+
+  } while (tryConsumeExpectedToken(TokenKind::pu_comma));
+
+  return Params;
+}
+
 std::optional<RootSignatureParser::ParsedClauseParams>
 RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
   assert(CurToken.TokKind == TokenKind::pu_l_paren &&
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 876dcd17f0389..0f32523493a53 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -346,9 +346,9 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootFlagsTest) {
 
 TEST_F(ParseHLSLRootSignatureTest, ValidParseRootParamsTest) {
   const llvm::StringLiteral Source = R"cc(
-    CBV(),
-    SRV(),
-    UAV()
+    CBV(b0),
+    SRV(t42),
+    UAV(u34893247)
   )cc";
 
   TrivialModuleLoader ModLoader;
@@ -368,15 +368,20 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootParamsTest) {
 
   RootElement Elem = Elements[0];
   ASSERT_TRUE(std::holds_alternative<RootParam>(Elem));
-  ASSERT_EQ(std::get<RootParam>(Elem).Type, ParamType::CBuffer);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.ViewType, RegisterType::BReg);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.Number, 0u);
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<RootParam>(Elem));
   ASSERT_EQ(std::get<RootParam>(Elem).Type, ParamType::SRV);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.ViewType, RegisterType::TReg);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.Number, 42u);
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<RootParam>(Elem));
   ASSERT_EQ(std::get<RootParam>(Elem).Type, ParamType::UAV);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.ViewType, RegisterType::UReg);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.Number, 34893247u);
 
   ASSERT_TRUE(Consumer->isSatisfied());
 }
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 2bcae22c6cfb2..5b2bb59fe9c02 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -88,6 +88,7 @@ struct RootConstants {
 using ParamType = llvm::dxil::ResourceClass;
 struct RootParam {
   ParamType Type;
+  Register Reg;
 };
 
 // Models the end of a descriptor table and stores its visibility

llvmbot · 2025-05-15T22:01:48Z

@llvm/pr-subscribers-hlsl

Author: Finn Plummer (inbelic)

Changes

defines the parseRootParamParams infastructure for parsing the params of a RootParam
defines the register type to illustrate use
add tests to demonstrate functionality

Part 2 of #126577

Full diff: https://github.com/llvm/llvm-project/pull/140148.diff

4 Files Affected:

(modified) clang/include/clang/Parse/ParseHLSLRootSignature.h (+6)
(modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+42)
(modified) clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp (+9-4)
(modified) llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h (+1)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index b96416ab6c6c4..92b8e7389426a 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -89,6 +89,12 @@ class RootSignatureParser {
   };
   std::optional<ParsedConstantParams> parseRootConstantParams();
 
+  struct ParsedRootParamParams {
+    std::optional<llvm::hlsl::rootsig::Register> Reg;
+  };
+  std::optional<ParsedRootParamParams>
+  parseRootParamParams(RootSignatureToken::Kind RegType);
+
   struct ParsedClauseParams {
     std::optional<llvm::hlsl::rootsig::Register> Reg;
     std::optional<uint32_t> NumDescriptors;
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 3ed60442eaa14..de7c2452b3fd4 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -176,20 +176,37 @@ std::optional<RootParam> RootSignatureParser::parseRootParam() {
     return std::nullopt;
 
   RootParam Param;
+  TokenKind ExpectedReg;
   switch (ParamKind) {
   default:
     llvm_unreachable("Switch for consumed token was not provided");
   case TokenKind::kw_CBV:
     Param.Type = ParamType::CBuffer;
+    ExpectedReg = TokenKind::bReg;
     break;
   case TokenKind::kw_SRV:
     Param.Type = ParamType::SRV;
+    ExpectedReg = TokenKind::tReg;
     break;
   case TokenKind::kw_UAV:
     Param.Type = ParamType::UAV;
+    ExpectedReg = TokenKind::uReg;
     break;
   }
 
+  auto Params = parseRootParamParams(ExpectedReg);
+  if (!Params.has_value())
+    return std::nullopt;
+
+  // Check mandatory parameters were provided
+  if (!Params->Reg.has_value()) {
+    getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+        << ExpectedReg;
+    return std::nullopt;
+  }
+
+  Param.Reg = Params->Reg.value();
+
   if (consumeExpectedToken(TokenKind::pu_r_paren,
                            diag::err_hlsl_unexpected_end_of_params,
                            /*param of=*/TokenKind::kw_RootConstants))
@@ -398,6 +415,31 @@ RootSignatureParser::parseRootConstantParams() {
   return Params;
 }
 
+std::optional<RootSignatureParser::ParsedRootParamParams>
+RootSignatureParser::parseRootParamParams(TokenKind RegType) {
+  assert(CurToken.TokKind == TokenKind::pu_l_paren &&
+         "Expects to only be invoked starting at given token");
+
+  ParsedRootParamParams Params;
+  do {
+    // ( `b` | `t` | `u`) POS_INT
+    if (tryConsumeExpectedToken(RegType)) {
+      if (Params.Reg.has_value()) {
+        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+            << CurToken.TokKind;
+        return std::nullopt;
+      }
+      auto Reg = parseRegister();
+      if (!Reg.has_value())
+        return std::nullopt;
+      Params.Reg = Reg;
+    }
+
+  } while (tryConsumeExpectedToken(TokenKind::pu_comma));
+
+  return Params;
+}
+
 std::optional<RootSignatureParser::ParsedClauseParams>
 RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
   assert(CurToken.TokKind == TokenKind::pu_l_paren &&
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 876dcd17f0389..0f32523493a53 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -346,9 +346,9 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootFlagsTest) {
 
 TEST_F(ParseHLSLRootSignatureTest, ValidParseRootParamsTest) {
   const llvm::StringLiteral Source = R"cc(
-    CBV(),
-    SRV(),
-    UAV()
+    CBV(b0),
+    SRV(t42),
+    UAV(u34893247)
   )cc";
 
   TrivialModuleLoader ModLoader;
@@ -368,15 +368,20 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootParamsTest) {
 
   RootElement Elem = Elements[0];
   ASSERT_TRUE(std::holds_alternative<RootParam>(Elem));
-  ASSERT_EQ(std::get<RootParam>(Elem).Type, ParamType::CBuffer);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.ViewType, RegisterType::BReg);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.Number, 0u);
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<RootParam>(Elem));
   ASSERT_EQ(std::get<RootParam>(Elem).Type, ParamType::SRV);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.ViewType, RegisterType::TReg);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.Number, 42u);
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<RootParam>(Elem));
   ASSERT_EQ(std::get<RootParam>(Elem).Type, ParamType::UAV);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.ViewType, RegisterType::UReg);
+  ASSERT_EQ(std::get<RootParam>(Elem).Reg.Number, 34893247u);
 
   ASSERT_TRUE(Consumer->isSatisfied());
 }
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 2bcae22c6cfb2..5b2bb59fe9c02 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -88,6 +88,7 @@ struct RootConstants {
 using ParamType = llvm::dxil::ResourceClass;
 struct RootParam {
   ParamType Type;
+  Register Reg;
 };
 
 // Models the end of a descriptor table and stores its visibility

clang/lib/Parse/ParseHLSLRootSignature.cpp

joaosaffran · 2025-05-20T17:28:54Z

clang/include/clang/Parse/ParseHLSLRootSignature.h

@@ -89,6 +89,12 @@ class RootSignatureParser {
  };
  std::optional<ParsedConstantParams> parseRootConstantParams();

+  struct ParsedRootParamParams {


Comment this in the previous pr, but those seems like Root Descriptors, not Root Parameters

- defines the `parseRootParamParams` infastructure for parsing the params of a RootParam - defines the register type to illustrate use - add tests to demonstrate functionality

bogner

Looks basically good. A question about test coverage below.

bogner · 2025-05-22T16:55:05Z

clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp

-    CBV(),
-    SRV(),
-    UAV()


Do we have a test somewhere that we error/reject empty for these now that the required parameters are wired up?

There are the InvalidMissingParameterTests that check this diagnostic being produced.
However, not on this direct code-path (it is tested for DescriptorTable CBV clauses with the same logic). I will add a quick test here

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" HLSL HLSL Language Support labels May 15, 2025

alsepkow reviewed May 16, 2025

View reviewed changes

clang/lib/Parse/ParseHLSLRootSignature.cpp Show resolved Hide resolved

inbelic linked an issue May 16, 2025 that may be closed by this pull request

[HLSL] Implement Root Signature Parsing of Root Parameters #126577

Closed

4 tasks

joaosaffran reviewed May 20, 2025

View reviewed changes

inbelic changed the title ~~[HLSL][RootSignature] Add parsing of Register in params for RootParams~~ [HLSL][RootSignature] Add parsing of Register in params for RootDescriptors May 21, 2025

joaosaffran approved these changes May 21, 2025

View reviewed changes

inbelic added 2 commits May 21, 2025 21:55

[HLSL][RootSignature] Add parsing of Register in params for RootParams

ae3cffb

- defines the `parseRootParamParams` infastructure for parsing the params of a RootParam - defines the register type to illustrate use - add tests to demonstrate functionality

review: rename RootParam to RootDescriptor

bc276aa

inbelic changed the base branch from users/inbelic/pr-140147 to main May 21, 2025 22:01

inbelic force-pushed the inbelic/rs-mand-root-param branch from 7b81667 to bc276aa Compare May 21, 2025 22:02

self-review: fix typo from rebase

1103a27

bogner approved these changes May 22, 2025

View reviewed changes

review: add missing register error

9ab1473

inbelic merged commit 0c42aef into llvm:main May 22, 2025
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HLSL][RootSignature] Add parsing of Register in params for RootDescriptors #140148

[HLSL][RootSignature] Add parsing of Register in params for RootDescriptors #140148

Uh oh!

inbelic commented May 15, 2025 •

edited

Loading

Uh oh!

llvmbot commented May 15, 2025

Uh oh!

llvmbot commented May 15, 2025

Uh oh!

Uh oh!

joaosaffran May 20, 2025

Uh oh!

bogner left a comment

Uh oh!

bogner May 22, 2025

Uh oh!

inbelic May 22, 2025

Uh oh!

Uh oh!

Uh oh!

[HLSL][RootSignature] Add parsing of Register in params for RootDescriptors #140148

[HLSL][RootSignature] Add parsing of Register in params for RootDescriptors #140148

Uh oh!

Conversation

inbelic commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 15, 2025

Uh oh!

llvmbot commented May 15, 2025

Uh oh!

Uh oh!

joaosaffran May 20, 2025

Choose a reason for hiding this comment

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

bogner May 22, 2025

Choose a reason for hiding this comment

Uh oh!

inbelic May 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

inbelic commented May 15, 2025 •

edited

Loading