Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[HLSL][RootSignature] Add parsing infastructure for StaticSampler #140180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: users/inbelic/pr-140152
Choose a base branch
Loading
from

Conversation

inbelic
Copy link
Contributor

@inbelic inbelic commented May 16, 2025

  • define StaticSampler in-memory representation
  • implement the infastructure for parsing parameters of StaticSampler
  • define and implement parsing of the s reg to demonstrate
    functionality
  • add unit tests

First part of #126574

inbelic added 2 commits May 15, 2025 22:33
- define StaticSampler in-memory representation
- implement the infastructure for parsing parameters of StaticSampler
- define and implement parsing of the `s` reg to demonstrate
functionality
- add unit tests
@llvmbot 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 16, 2025
@llvmbot
Copy link
Member

llvmbot commented May 16, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-hlsl

Author: Finn Plummer (inbelic)

Changes
  • define StaticSampler in-memory representation
  • implement the infastructure for parsing parameters of StaticSampler
  • define and implement parsing of the s reg to demonstrate
    functionality
  • add unit tests

First part of #126574


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

6 Files Affected:

  • (modified) clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def (+1)
  • (modified) clang/include/clang/Parse/ParseHLSLRootSignature.h (+6)
  • (modified) clang/lib/Parse/ParseHLSLRootSignature.cpp (+62)
  • (modified) clang/unittests/Lex/LexHLSLRootSignatureTest.cpp (+1-1)
  • (modified) clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp (+28)
  • (modified) llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h (+11-5)
diff --git a/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def b/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
index c6f7f8928bc91..ddebe82987197 100644
--- a/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
+++ b/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
@@ -80,6 +80,7 @@ KEYWORD(RootSignature) // used only for diagnostic messaging
 KEYWORD(RootFlags)
 KEYWORD(DescriptorTable)
 KEYWORD(RootConstants)
+KEYWORD(StaticSampler)
 
 // RootConstants Keywords:
 KEYWORD(num32BitConstants)
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 7b9168290d62a..80fedc2f16574 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -77,6 +77,7 @@ class RootSignatureParser {
   std::optional<llvm::hlsl::rootsig::DescriptorTable> parseDescriptorTable();
   std::optional<llvm::hlsl::rootsig::DescriptorTableClause>
   parseDescriptorTableClause();
+  std::optional<llvm::hlsl::rootsig::StaticSampler> parseStaticSampler();
 
   /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
   /// order and only exactly once. The following methods define a
@@ -108,6 +109,11 @@ class RootSignatureParser {
   std::optional<ParsedClauseParams>
   parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType);
 
+  struct ParsedStaticSamplerParams {
+    std::optional<llvm::hlsl::rootsig::Register> Reg;
+  };
+  std::optional<ParsedStaticSamplerParams> parseStaticSamplerParams();
+
   // Common parsing methods
   std::optional<uint32_t> parseUIntParam();
   std::optional<llvm::hlsl::rootsig::Register> parseRegister();
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index faf261cc9b7fe..6e4bb4d59e109 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -55,6 +55,13 @@ bool RootSignatureParser::parse() {
         return true;
       Elements.push_back(*RootParam);
     }
+
+    if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
+      auto Sampler = parseStaticSampler();
+      if (!Sampler.has_value())
+        return true;
+      Elements.push_back(*Sampler);
+    }
   } while (tryConsumeExpectedToken(TokenKind::pu_comma));
 
   return consumeExpectedToken(TokenKind::end_of_stream,
@@ -348,6 +355,37 @@ RootSignatureParser::parseDescriptorTableClause() {
   return Clause;
 }
 
+std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() {
+  assert(CurToken.TokKind == TokenKind::kw_StaticSampler &&
+         "Expects to only be invoked starting at given keyword");
+
+  if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
+                           CurToken.TokKind))
+    return std::nullopt;
+
+  StaticSampler Sampler;
+
+  auto Params = parseStaticSamplerParams();
+  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)
+        << TokenKind::sReg;
+    return std::nullopt;
+  }
+
+  Sampler.Reg = Params->Reg.value();
+
+  if (consumeExpectedToken(TokenKind::pu_r_paren,
+                           diag::err_hlsl_unexpected_end_of_params,
+                           /*param of=*/TokenKind::kw_StaticSampler))
+    return std::nullopt;
+
+  return Sampler;
+}
+
 // Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
 // order and only exactly once. The following methods will parse through as
 // many arguments as possible reporting an error if a duplicate is seen.
@@ -606,6 +644,30 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
   return Params;
 }
 
+std::optional<RootSignatureParser::ParsedStaticSamplerParams>
+RootSignatureParser::parseStaticSamplerParams() {
+  assert(CurToken.TokKind == TokenKind::pu_l_paren &&
+         "Expects to only be invoked starting at given token");
+
+  ParsedStaticSamplerParams Params;
+  do {
+    // `s` POS_INT
+    if (tryConsumeExpectedToken(TokenKind::sReg)) {
+      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<uint32_t> RootSignatureParser::parseUIntParam() {
   assert(CurToken.TokKind == TokenKind::pu_equal &&
          "Expects to only be invoked starting at given keyword");
diff --git a/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp b/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
index 1f8d8be64e323..3e38c281f4fb1 100644
--- a/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
@@ -128,7 +128,7 @@ TEST_F(LexHLSLRootSignatureTest, ValidLexAllTokensTest) {
 
     RootSignature
 
-    RootFlags DescriptorTable RootConstants
+    RootFlags DescriptorTable RootConstants StaticSampler
 
     num32BitConstants
 
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 7ed286589f8fa..14c3101f3eafa 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -223,6 +223,34 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
   ASSERT_TRUE(Consumer->isSatisfied());
 }
 
+TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) {
+  const llvm::StringLiteral Source = R"cc(
+    StaticSampler(s0)
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = createPP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test no diagnostics produced
+  Consumer->setNoDiag();
+
+  ASSERT_FALSE(Parser.parse());
+
+  ASSERT_EQ(Elements.size(), 1u);
+
+  RootElement Elem = Elements[0];
+  ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem));
+  ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.ViewType, RegisterType::SReg);
+  ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.Number, 0u);
+
+  ASSERT_TRUE(Consumer->isSatisfied());
+}
+
 TEST_F(ParseHLSLRootSignatureTest, ValidSamplerFlagsTest) {
   // This test will checks we can set the valid enum for Sampler descriptor
   // range flags
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 98fa5f09429e3..25df9a7235ef3 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -158,8 +158,12 @@ struct DescriptorTableClause {
   void dump(raw_ostream &OS) const;
 };
 
+struct StaticSampler {
+  Register Reg;
+};
+
 /// Models RootElement : RootFlags | RootConstants | RootParam
-///  | DescriptorTable | DescriptorTableClause
+///  | DescriptorTable | DescriptorTableClause | StaticSampler
 ///
 /// A Root Signature is modeled in-memory by an array of RootElements. These
 /// aim to map closely to their DSL grammar reprsentation defined in the spec.
@@ -167,14 +171,16 @@ struct DescriptorTableClause {
 /// Each optional parameter has its default value defined in the struct, and,
 /// each mandatory parameter does not have a default initialization.
 ///
-/// For the variants RootFlags, RootConstants and DescriptorTableClause: each
-/// data member maps directly to a parameter in the grammar.
+/// For the variants RootFlags, RootConstants, RootParam, StaticSampler and
+/// DescriptorTableClause: each data member maps directly to a parameter in the
+/// grammar.
 ///
 /// The DescriptorTable is modelled by having its Clauses as the previous
 /// RootElements in the array, and it holds a data member for the Visibility
 /// parameter.
-using RootElement = std::variant<RootFlags, RootConstants, RootParam,
-                                 DescriptorTable, DescriptorTableClause>;
+using RootElement =
+    std::variant<RootFlags, RootConstants, RootParam, DescriptorTable,
+                 DescriptorTableClause, StaticSampler>;
 void dumpRootElements(raw_ostream &OS, ArrayRef<RootElement> Elements);
 
 class MetadataBuilder {

@inbelic inbelic linked an issue May 16, 2025 that may be closed by this pull request
4 tasks
return std::nullopt;
}
auto Reg = parseRegister();
if (!Reg.has_value())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this case look like? Why is this not a parsing error?

@@ -223,6 +223,34 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
ASSERT_TRUE(Consumer->isSatisfied());
}

TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you add a test where the parsing is unsuccessful?

return std::nullopt;
Params.Reg = Reg;
}
} while (tryConsumeExpectedToken(TokenKind::pu_comma));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this parsing code allows stuff of the form 'StaticSampler(,,,,,,,,,,,,,,,,,)', and from looking at a DXC example it seems that is allowed https://godbolt.org/z/j7qP11h9W. It is amusing this is so permissive, and I wonder if there is a reason for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category HLSL HLSL Language Support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HLSL] Implement Root Signature Parsing of Static Samplers
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.