-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[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
base: users/inbelic/pr-140152
Are you sure you want to change the base?
[HLSL][RootSignature] Add parsing infastructure for StaticSampler #140180
Conversation
- 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
@llvm/pr-subscribers-clang @llvm/pr-subscribers-hlsl Author: Finn Plummer (inbelic) Changes
First part of #126574 Full diff: https://github.com/llvm/llvm-project/pull/140180.diff 6 Files Affected:
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 {
|
return std::nullopt; | ||
} | ||
auto Reg = parseRegister(); | ||
if (!Reg.has_value()) |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
s
reg to demonstratefunctionality
First part of #126574