Skip to content

Navigation Menu

Sign in
Appearance settings

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

Merged
merged 3 commits into from
May 27, 2025

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case would occur, for instance, when the register number would overflow an u32. Something like s4294967296.

It will have already reported the error (as part of handleUIntLiteral invoked by parseRegister) at this point and we are just propagating the error up.

@@ -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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted-out to reduce redundancy, as there is already testing for the errors that are encountered, albeit from a different code path.

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.

- 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
@inbelic inbelic changed the base branch from users/inbelic/pr-140152 to main May 26, 2025 20:49
@inbelic inbelic force-pushed the inbelic/rs-reg-sampler branch from e479220 to 05cd843 Compare May 26, 2025 20:50
@inbelic inbelic merged commit 0259541 into llvm:main May 27, 2025
12 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented May 27, 2025

LLVM Buildbot has detected a new failure on builder clang-riscv-rva23-evl-vec-2stage running on rise-clang-riscv-rva23-evl-vec-2stage while building clang,llvm at step 4 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/132/builds/1244

Here is the relevant piece of the build log for the reference
Step 4 (annotate) failure: '../llvm-zorg/zorg/buildbot/builders/annotated/rise-riscv-build.sh --jobs=16' (failure)
...
PASS: lit :: test-data.py (81373 of 84500)
PASS: lit :: unit/TestRunner.py (81374 of 84500)
PASS: lit :: shtest-format.py (81375 of 84500)
PASS: LLVM-Unit :: Support/./SupportTests/10/48 (81376 of 84500)
PASS: lit :: test-output-micro-resultdb.py (81377 of 84500)
PASS: lit :: unittest-adaptor.py (81378 of 84500)
PASS: lit :: test-output-micro.py (81379 of 84500)
PASS: lit :: test-output.py (81380 of 84500)
PASS: LLVM :: tools/llvm-reduce/reduce-distinct-metadata.ll (81381 of 84500)
PASS: lit :: show-result-codes.py (81382 of 84500)
FAIL: lit :: timeout-hang.py (81383 of 84500)
******************** TEST 'lit :: timeout-hang.py' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 13
not env -u FILECHECK_OPTS "/usr/bin/python3" /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/llvm/llvm/utils/lit/lit.py -j1 --order=lexical Inputs/timeout-hang/run-nonexistent.txt  --timeout=1 --param external=0 | "/usr/bin/python3" /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/stage2/utils/lit/tests/timeout-hang.py 1
# executed command: not env -u FILECHECK_OPTS /usr/bin/python3 /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/llvm/llvm/utils/lit/lit.py -j1 --order=lexical Inputs/timeout-hang/run-nonexistent.txt --timeout=1 --param external=0
# .---command stderr------------
# | lit.py: /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/llvm/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 1 seconds was requested on the command line. Forcing timeout to be 1 seconds.
# `-----------------------------
# executed command: /usr/bin/python3 /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/stage2/utils/lit/tests/timeout-hang.py 1
# .---command stdout------------
# | Testing took as long or longer than timeout
# `-----------------------------
# error: command failed with exit status: 1

--

********************
PASS: lld :: COFF/alias-implib.s (81384 of 84500)
PASS: lld :: COFF/ar-comdat.test (81385 of 84500)
PASS: lld :: COFF/arm-thumb-branch20-error.s (81386 of 84500)
PASS: lld :: COFF/arm-thumb-thunks-multipass.s (81387 of 84500)
PASS: lld :: COFF/arm64-localimport-align.s (81388 of 84500)
PASS: lld :: COFF/allow-unknown-debug-info.test (81389 of 84500)
PASS: lld :: COFF/arm64-delayimport.yaml (81390 of 84500)
PASS: lld :: COFF/arm-thumb-thunks.s (81391 of 84500)
PASS: lld :: COFF/arm64-import2.test (81392 of 84500)
PASS: lld :: COFF/alternatename.test (81393 of 84500)
PASS: lld :: COFF/arm-thumb-thunks-pdb.s (81394 of 84500)
PASS: lld :: COFF/arm64-magic.yaml (81395 of 84500)
PASS: lld :: COFF/arm64-dynamicbase.s (81396 of 84500)
PASS: lld :: COFF/align.s (81397 of 84500)
PASS: lit :: xunit-output-report-failures-only.py (81398 of 84500)
PASS: lld :: COFF/arm64-relocs-imports.test (81399 of 84500)
PASS: lld :: COFF/arm64-thunks.s (81400 of 84500)
PASS: lit :: xunit-output.py (81401 of 84500)
Step 11 (llvm-project check-all) failure: llvm-project check-all (failure)
...
PASS: lit :: test-data.py (81373 of 84500)
PASS: lit :: unit/TestRunner.py (81374 of 84500)
PASS: lit :: shtest-format.py (81375 of 84500)
PASS: LLVM-Unit :: Support/./SupportTests/10/48 (81376 of 84500)
PASS: lit :: test-output-micro-resultdb.py (81377 of 84500)
PASS: lit :: unittest-adaptor.py (81378 of 84500)
PASS: lit :: test-output-micro.py (81379 of 84500)
PASS: lit :: test-output.py (81380 of 84500)
PASS: LLVM :: tools/llvm-reduce/reduce-distinct-metadata.ll (81381 of 84500)
PASS: lit :: show-result-codes.py (81382 of 84500)
FAIL: lit :: timeout-hang.py (81383 of 84500)
******************** TEST 'lit :: timeout-hang.py' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 13
not env -u FILECHECK_OPTS "/usr/bin/python3" /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/llvm/llvm/utils/lit/lit.py -j1 --order=lexical Inputs/timeout-hang/run-nonexistent.txt  --timeout=1 --param external=0 | "/usr/bin/python3" /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/stage2/utils/lit/tests/timeout-hang.py 1
# executed command: not env -u FILECHECK_OPTS /usr/bin/python3 /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/llvm/llvm/utils/lit/lit.py -j1 --order=lexical Inputs/timeout-hang/run-nonexistent.txt --timeout=1 --param external=0
# .---command stderr------------
# | lit.py: /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/llvm/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 1 seconds was requested on the command line. Forcing timeout to be 1 seconds.
# `-----------------------------
# executed command: /usr/bin/python3 /home/buildbot-worker/bbroot/clang-riscv-rva23-evl-vec-2stage/stage2/utils/lit/tests/timeout-hang.py 1
# .---command stdout------------
# | Testing took as long or longer than timeout
# `-----------------------------
# error: command failed with exit status: 1

--

********************
PASS: lld :: COFF/alias-implib.s (81384 of 84500)
PASS: lld :: COFF/ar-comdat.test (81385 of 84500)
PASS: lld :: COFF/arm-thumb-branch20-error.s (81386 of 84500)
PASS: lld :: COFF/arm-thumb-thunks-multipass.s (81387 of 84500)
PASS: lld :: COFF/arm64-localimport-align.s (81388 of 84500)
PASS: lld :: COFF/allow-unknown-debug-info.test (81389 of 84500)
PASS: lld :: COFF/arm64-delayimport.yaml (81390 of 84500)
PASS: lld :: COFF/arm-thumb-thunks.s (81391 of 84500)
PASS: lld :: COFF/arm64-import2.test (81392 of 84500)
PASS: lld :: COFF/alternatename.test (81393 of 84500)
PASS: lld :: COFF/arm-thumb-thunks-pdb.s (81394 of 84500)
PASS: lld :: COFF/arm64-magic.yaml (81395 of 84500)
PASS: lld :: COFF/arm64-dynamicbase.s (81396 of 84500)
PASS: lld :: COFF/align.s (81397 of 84500)
PASS: lit :: xunit-output-report-failures-only.py (81398 of 84500)
PASS: lld :: COFF/arm64-relocs-imports.test (81399 of 84500)
PASS: lld :: COFF/arm64-thunks.s (81400 of 84500)
PASS: lit :: xunit-output.py (81401 of 84500)

@inbelic inbelic deleted the inbelic/rs-reg-sampler branch June 2, 2025 20:33
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Jun 3, 2025
…vm#140180)

- 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 llvm#126574
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.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.