[HLSL][RootSignature] Implement initial parsing of the descriptor table clause params #133800
+336
−30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Finn Plummer (inbelic) Changes
Part two of implementing: #126569 Full diff: https://github.com/llvm/llvm-project/pull/133800.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 2582e1e5ef0f6..ab12159ba5ae1 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1830,8 +1830,11 @@ def err_hlsl_virtual_function
def err_hlsl_virtual_inheritance
: Error<"virtual inheritance is unsupported in HLSL">;
-// HLSL Root Siganture diagnostic messages
+// HLSL Root Signature Parser Diagnostics
def err_hlsl_unexpected_end_of_params
: Error<"expected %0 to denote end of parameters, or, another valid parameter of %1">;
+def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' multiple times">;
+def err_hlsl_rootsig_missing_param : Error<"did not specify mandatory parameter '%0'">;
+def err_hlsl_number_literal_overflow : Error<"integer literal is too large to be represented as a 32-bit %select{signed |}0 integer type">;
} // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 43b41315b88b5..02e99e83875db 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -69,6 +69,46 @@ class RootSignatureParser {
bool parseDescriptorTable();
bool parseDescriptorTableClause();
+ /// Each unique ParamType will have a custom parse method defined that we can
+ /// use to invoke the parameters.
+ ///
+ /// This function will switch on the ParamType using std::visit and dispatch
+ /// onto the corresponding parse method
+ bool parseParam(llvm::hlsl::rootsig::ParamType Ref);
+
+ /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
+ /// order, exactly once, and only a subset are mandatory. This function acts
+ /// as the infastructure to do so in a declarative way.
+ ///
+ /// For the example:
+ /// SmallDenseMap<TokenKind, ParamType> Params = {
+ /// TokenKind::bReg, &Clause.Register,
+ /// TokenKind::kw_space, &Clause.Space
+ /// };
+ /// SmallDenseSet<TokenKind> Mandatory = {
+ /// TokenKind::kw_numDescriptors
+ /// };
+ ///
+ /// We can read it is as:
+ ///
+ /// when 'b0' is encountered, invoke the parse method for the type
+ /// of &Clause.Register (Register *) and update the parameter
+ /// when 'space' is encountered, invoke a parse method for the type
+ /// of &Clause.Space (uint32_t *) and update the parameter
+ ///
+ /// and 'bReg' must be specified
+ bool parseParams(
+ llvm::SmallDenseMap<TokenKind, llvm::hlsl::rootsig::ParamType> &Params,
+ llvm::SmallDenseSet<TokenKind> &Mandatory);
+
+ /// Parameter parse methods corresponding to a ParamType
+ bool parseUIntParam(uint32_t *X);
+ bool parseRegister(llvm::hlsl::rootsig::Register *Reg);
+
+ /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned
+ /// 32-bit integer
+ bool handleUIntLiteral(uint32_t *X);
+
/// Invoke the Lexer to consume a token and update CurToken with the result
void consumeNextToken() { CurToken = Lexer.ConsumeToken(); }
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 33caca5fa1c82..62d29baea49d3 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -8,6 +8,8 @@
#include "clang/Parse/ParseHLSLRootSignature.h"
+#include "clang/Lex/LiteralSupport.h"
+
#include "llvm/Support/raw_ostream.h"
using namespace llvm::hlsl::rootsig;
@@ -39,12 +41,11 @@ bool RootSignatureParser::parse() {
break;
}
- if (!tryConsumeExpectedToken(TokenKind::end_of_stream)) {
- getDiags().Report(CurToken.TokLoc, diag::err_hlsl_unexpected_end_of_params)
- << /*expected=*/TokenKind::end_of_stream
- << /*param of=*/TokenKind::kw_RootSignature;
+ if (consumeExpectedToken(TokenKind::end_of_stream,
+ diag::err_hlsl_unexpected_end_of_params,
+ /*param of=*/TokenKind::kw_RootSignature))
return true;
- }
+
return false;
}
@@ -70,12 +71,10 @@ bool RootSignatureParser::parseDescriptorTable() {
break;
}
- if (!tryConsumeExpectedToken(TokenKind::pu_r_paren)) {
- getDiags().Report(CurToken.TokLoc, diag::err_hlsl_unexpected_end_of_params)
- << /*expected=*/TokenKind::pu_r_paren
- << /*param of=*/TokenKind::kw_DescriptorTable;
+ if (consumeExpectedToken(TokenKind::pu_r_paren,
+ diag::err_hlsl_unexpected_end_of_params,
+ /*param of=*/TokenKind::kw_DescriptorTable))
return true;
- }
Elements.push_back(Table);
return false;
@@ -87,37 +86,174 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.Kind == TokenKind::kw_UAV ||
CurToken.Kind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.Kind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.Kind) {
+ TokenKind ExpectedRegister;
+ switch (ParamKind) {
default:
llvm_unreachable("Switch for consumed token was not provided");
case TokenKind::kw_CBV:
Clause.Type = ClauseType::CBuffer;
+ ExpectedRegister = TokenKind::bReg;
break;
case TokenKind::kw_SRV:
Clause.Type = ClauseType::SRV;
+ ExpectedRegister = TokenKind::tReg;
break;
case TokenKind::kw_UAV:
Clause.Type = ClauseType::UAV;
+ ExpectedRegister = TokenKind::uReg;
break;
case TokenKind::kw_Sampler:
Clause.Type = ClauseType::Sampler;
+ ExpectedRegister = TokenKind::sReg;
break;
}
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
- CurToken.Kind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.Kind))
+ llvm::SmallDenseMap<TokenKind, ParamType> Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet<TokenKind> Mandatory = {
+ ExpectedRegister,
+ };
+
+ if (parseParams(Params, Mandatory))
+ return true;
+
+ if (consumeExpectedToken(TokenKind::pu_r_paren,
+ diag::err_hlsl_unexpected_end_of_params,
+ /*param of=*/ParamKind))
return true;
Elements.push_back(Clause);
return false;
}
+// Helper struct defined to use the overloaded notation of std::visit.
+template <class... Ts> struct ParseMethods : Ts... { using Ts::operator()...; };
+template <class... Ts> ParseMethods(Ts...) -> ParseMethods<Ts...>;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+ return consumeExpectedToken(TokenKind::pu_equal,
+ diag::err_expected_after,
+ CurToken.Kind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+ llvm::SmallDenseMap<TokenKind, ParamType> &Params,
+ llvm::SmallDenseSet<TokenKind> &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector<TokenKind> Keywords;
+ for (auto Pair : Params)
+ Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet<TokenKind> Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+ if (Seen.contains(CurToken.Kind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.Kind;
+ return true;
+ }
+ Seen.insert(CurToken.Kind);
+
+ if (parseParam(Params[CurToken.Kind]))
+ return true;
+
+ if (!tryConsumeExpectedToken(TokenKind::pu_comma))
+ break;
+ }
+
+ bool AllMandatoryDefined = true;
+ for (auto Kind : Mandatory) {
+ bool SeenParam = Seen.contains(Kind);
+ if (!SeenParam) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+ << Kind;
+ }
+ AllMandatoryDefined &= SeenParam;
+ }
+
+ return !AllMandatoryDefined;
+}
+
+bool RootSignatureParser::parseUIntParam(uint32_t *X) {
+ assert(CurToken.Kind == TokenKind::pu_equal &&
+ "Expects to only be invoked starting at given keyword");
+ tryConsumeExpectedToken(TokenKind::pu_plus);
+ return consumeExpectedToken(TokenKind::int_literal, diag::err_expected_after,
+ CurToken.Kind) ||
+ handleUIntLiteral(X);
+}
+
+bool RootSignatureParser::parseRegister(Register *Register) {
+ assert(
+ (CurToken.Kind == TokenKind::bReg || CurToken.Kind == TokenKind::tReg ||
+ CurToken.Kind == TokenKind::uReg || CurToken.Kind == TokenKind::sReg) &&
+ "Expects to only be invoked starting at given keyword");
+
+ switch (CurToken.Kind) {
+ case TokenKind::bReg:
+ Register->ViewType = RegisterType::BReg;
+ break;
+ case TokenKind::tReg:
+ Register->ViewType = RegisterType::TReg;
+ break;
+ case TokenKind::uReg:
+ Register->ViewType = RegisterType::UReg;
+ break;
+ case TokenKind::sReg:
+ Register->ViewType = RegisterType::SReg;
+ break;
+ default:
+ break; // Unreachable given Try + assert pattern
+ }
+
+ if (handleUIntLiteral(&Register->Number))
+ return true; // propogate NumericLiteralParser error
+
+ return false;
+}
+
+bool RootSignatureParser::handleUIntLiteral(uint32_t *X) {
+ // Parse the numeric value and do semantic checks on its specification
+ clang::NumericLiteralParser Literal(CurToken.NumSpelling, CurToken.TokLoc,
+ PP.getSourceManager(), PP.getLangOpts(),
+ PP.getTargetInfo(), PP.getDiagnostics());
+ if (Literal.hadError)
+ return true; // Error has already been reported so just return
+
+ assert(Literal.isIntegerLiteral() && "IsNumberChar will only support digits");
+
+ llvm::APSInt Val = llvm::APSInt(32, false);
+ if (Literal.GetIntegerValue(Val)) {
+ // Report that the value has overflowed
+ PP.getDiagnostics().Report(CurToken.TokLoc,
+ diag::err_hlsl_number_literal_overflow)
+ << 0 << CurToken.NumSpelling;
+ return true;
+ }
+
+ *X = Val.getExtValue();
+ return false;
+}
+
bool RootSignatureParser::peekExpectedToken(TokenKind Expected) {
return peekExpectedToken(ArrayRef{Expected});
}
@@ -139,6 +275,7 @@ bool RootSignatureParser::consumeExpectedToken(TokenKind Expected,
case diag::err_expected:
DB << Expected;
break;
+ case diag::err_hlsl_unexpected_end_of_params:
case diag::err_expected_either:
case diag::err_expected_after:
DB << Expected << Context;
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index acdf455a5d6aa..5b162e9a1b4cd 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -130,10 +130,10 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
const llvm::StringLiteral Source = R"cc(
DescriptorTable(
- CBV(),
- SRV(),
- Sampler(),
- UAV()
+ CBV(b0),
+ SRV(space = 3, t42),
+ Sampler(s987, space = +2),
+ UAV(u4294967294)
),
DescriptorTable()
)cc";
@@ -155,18 +155,34 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
RootElement Elem = Elements[0];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::BReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 0u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u);
Elem = Elements[1];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::TReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 42u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 3u);
Elem = Elements[2];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::SReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 987u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 2u);
Elem = Elements[3];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::UReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 4294967294u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u);
Elem = Elements[4];
ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
@@ -176,6 +192,32 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
Elem = Elements[5];
ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 0u);
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, ValidTrailingCommaTest) {
+ // This test will checks we can handling trailing commas ','
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(b0, ),
+ SRV(t42),
+ )
+ )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_TRUE(Consumer->isSatisfied());
}
@@ -237,6 +279,102 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedEndOfStreamTest) {
// Test correct diagnostic produced - end of stream
Consumer->setExpected(diag::err_expected_after);
+
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidMissingParameterTest) {
+ // This test will check that the parsing fails due a mandatory
+ // parameter (register) not being specified
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV()
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_rootsig_missing_param);
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryParameterTest) {
+ // This test will check that the parsing fails due the same mandatory
+ // parameter being specified multiple times
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(b32, b84)
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_rootsig_repeat_param);
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedOptionalParameterTest) {
+ // This test will check that the parsing fails due the same optional
+ // parameter being specified multiple times
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(space = 2, space = 0)
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_rootsig_repeat_param);
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedNumberTest) {
+ // This test will check that the lexing fails due to an integer overflow
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(b4294967296)
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_number_literal_overflow);
ASSERT_TRUE(Parser.parse());
ASSERT_TRUE(Consumer->isSatisfied());
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index c1b67844c747f..0ae8879b6c7d5 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -23,6 +23,13 @@ namespace rootsig {
// Definitions of the in-memory data layout structures
+// Models the different registers: bReg | tReg | uReg | sReg
+enum class RegisterType { BReg, TReg, UReg, SReg };
+struct Register {
+ RegisterType ViewType;
+ uint32_t Number;
+};
+
// Models the end of a descriptor table and stores its visibility
struct DescriptorTable {
uint32_t NumClauses = 0; // The number of clauses in the table
@@ -32,11 +39,19 @@ struct DescriptorTable {
using ClauseType = llvm::dxil::ResourceClass;
struct DescriptorTableClause {
ClauseType Type;
+ Register Register;
+ uint32_t Space = 0;
};
// Models RootElement : DescriptorTable | DescriptorTableClause
using RootElement = std::variant<DescriptorTable, DescriptorTableClause>;
+// ParamType is used as an 'any' type that will reference to a parameter in
+// RootElement. Each variant of ParamType is expected to have a Parse method
+// defined that will be dispatched on when we are attempting to parse a
+// parameter
+using ParamType = std::variant<uint32_t *, Register *>;
+
} // namespace rootsig
} // namespace hlsl
} // namespace llvm
|
|
@llvm/pr-subscribers-hlsl Author: Finn Plummer (inbelic) Changes
Part two of implementing: #126569 Full diff: https://github.com/llvm/llvm-project/pull/133800.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 2582e1e5ef0f6..ab12159ba5ae1 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1830,8 +1830,11 @@ def err_hlsl_virtual_function
def err_hlsl_virtual_inheritance
: Error<"virtual inheritance is unsupported in HLSL">;
-// HLSL Root Siganture diagnostic messages
+// HLSL Root Signature Parser Diagnostics
def err_hlsl_unexpected_end_of_params
: Error<"expected %0 to denote end of parameters, or, another valid parameter of %1">;
+def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' multiple times">;
+def err_hlsl_rootsig_missing_param : Error<"did not specify mandatory parameter '%0'">;
+def err_hlsl_number_literal_overflow : Error<"integer literal is too large to be represented as a 32-bit %select{signed |}0 integer type">;
} // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 43b41315b88b5..02e99e83875db 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -69,6 +69,46 @@ class RootSignatureParser {
bool parseDescriptorTable();
bool parseDescriptorTableClause();
+ /// Each unique ParamType will have a custom parse method defined that we can
+ /// use to invoke the parameters.
+ ///
+ /// This function will switch on the ParamType using std::visit and dispatch
+ /// onto the corresponding parse method
+ bool parseParam(llvm::hlsl::rootsig::ParamType Ref);
+
+ /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
+ /// order, exactly once, and only a subset are mandatory. This function acts
+ /// as the infastructure to do so in a declarative way.
+ ///
+ /// For the example:
+ /// SmallDenseMap<TokenKind, ParamType> Params = {
+ /// TokenKind::bReg, &Clause.Register,
+ /// TokenKind::kw_space, &Clause.Space
+ /// };
+ /// SmallDenseSet<TokenKind> Mandatory = {
+ /// TokenKind::kw_numDescriptors
+ /// };
+ ///
+ /// We can read it is as:
+ ///
+ /// when 'b0' is encountered, invoke the parse method for the type
+ /// of &Clause.Register (Register *) and update the parameter
+ /// when 'space' is encountered, invoke a parse method for the type
+ /// of &Clause.Space (uint32_t *) and update the parameter
+ ///
+ /// and 'bReg' must be specified
+ bool parseParams(
+ llvm::SmallDenseMap<TokenKind, llvm::hlsl::rootsig::ParamType> &Params,
+ llvm::SmallDenseSet<TokenKind> &Mandatory);
+
+ /// Parameter parse methods corresponding to a ParamType
+ bool parseUIntParam(uint32_t *X);
+ bool parseRegister(llvm::hlsl::rootsig::Register *Reg);
+
+ /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned
+ /// 32-bit integer
+ bool handleUIntLiteral(uint32_t *X);
+
/// Invoke the Lexer to consume a token and update CurToken with the result
void consumeNextToken() { CurToken = Lexer.ConsumeToken(); }
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 33caca5fa1c82..62d29baea49d3 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -8,6 +8,8 @@
#include "clang/Parse/ParseHLSLRootSignature.h"
+#include "clang/Lex/LiteralSupport.h"
+
#include "llvm/Support/raw_ostream.h"
using namespace llvm::hlsl::rootsig;
@@ -39,12 +41,11 @@ bool RootSignatureParser::parse() {
break;
}
- if (!tryConsumeExpectedToken(TokenKind::end_of_stream)) {
- getDiags().Report(CurToken.TokLoc, diag::err_hlsl_unexpected_end_of_params)
- << /*expected=*/TokenKind::end_of_stream
- << /*param of=*/TokenKind::kw_RootSignature;
+ if (consumeExpectedToken(TokenKind::end_of_stream,
+ diag::err_hlsl_unexpected_end_of_params,
+ /*param of=*/TokenKind::kw_RootSignature))
return true;
- }
+
return false;
}
@@ -70,12 +71,10 @@ bool RootSignatureParser::parseDescriptorTable() {
break;
}
- if (!tryConsumeExpectedToken(TokenKind::pu_r_paren)) {
- getDiags().Report(CurToken.TokLoc, diag::err_hlsl_unexpected_end_of_params)
- << /*expected=*/TokenKind::pu_r_paren
- << /*param of=*/TokenKind::kw_DescriptorTable;
+ if (consumeExpectedToken(TokenKind::pu_r_paren,
+ diag::err_hlsl_unexpected_end_of_params,
+ /*param of=*/TokenKind::kw_DescriptorTable))
return true;
- }
Elements.push_back(Table);
return false;
@@ -87,37 +86,174 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.Kind == TokenKind::kw_UAV ||
CurToken.Kind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.Kind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.Kind) {
+ TokenKind ExpectedRegister;
+ switch (ParamKind) {
default:
llvm_unreachable("Switch for consumed token was not provided");
case TokenKind::kw_CBV:
Clause.Type = ClauseType::CBuffer;
+ ExpectedRegister = TokenKind::bReg;
break;
case TokenKind::kw_SRV:
Clause.Type = ClauseType::SRV;
+ ExpectedRegister = TokenKind::tReg;
break;
case TokenKind::kw_UAV:
Clause.Type = ClauseType::UAV;
+ ExpectedRegister = TokenKind::uReg;
break;
case TokenKind::kw_Sampler:
Clause.Type = ClauseType::Sampler;
+ ExpectedRegister = TokenKind::sReg;
break;
}
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
- CurToken.Kind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.Kind))
+ llvm::SmallDenseMap<TokenKind, ParamType> Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet<TokenKind> Mandatory = {
+ ExpectedRegister,
+ };
+
+ if (parseParams(Params, Mandatory))
+ return true;
+
+ if (consumeExpectedToken(TokenKind::pu_r_paren,
+ diag::err_hlsl_unexpected_end_of_params,
+ /*param of=*/ParamKind))
return true;
Elements.push_back(Clause);
return false;
}
+// Helper struct defined to use the overloaded notation of std::visit.
+template <class... Ts> struct ParseMethods : Ts... { using Ts::operator()...; };
+template <class... Ts> ParseMethods(Ts...) -> ParseMethods<Ts...>;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+ return consumeExpectedToken(TokenKind::pu_equal,
+ diag::err_expected_after,
+ CurToken.Kind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+ llvm::SmallDenseMap<TokenKind, ParamType> &Params,
+ llvm::SmallDenseSet<TokenKind> &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector<TokenKind> Keywords;
+ for (auto Pair : Params)
+ Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet<TokenKind> Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+ if (Seen.contains(CurToken.Kind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.Kind;
+ return true;
+ }
+ Seen.insert(CurToken.Kind);
+
+ if (parseParam(Params[CurToken.Kind]))
+ return true;
+
+ if (!tryConsumeExpectedToken(TokenKind::pu_comma))
+ break;
+ }
+
+ bool AllMandatoryDefined = true;
+ for (auto Kind : Mandatory) {
+ bool SeenParam = Seen.contains(Kind);
+ if (!SeenParam) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
+ << Kind;
+ }
+ AllMandatoryDefined &= SeenParam;
+ }
+
+ return !AllMandatoryDefined;
+}
+
+bool RootSignatureParser::parseUIntParam(uint32_t *X) {
+ assert(CurToken.Kind == TokenKind::pu_equal &&
+ "Expects to only be invoked starting at given keyword");
+ tryConsumeExpectedToken(TokenKind::pu_plus);
+ return consumeExpectedToken(TokenKind::int_literal, diag::err_expected_after,
+ CurToken.Kind) ||
+ handleUIntLiteral(X);
+}
+
+bool RootSignatureParser::parseRegister(Register *Register) {
+ assert(
+ (CurToken.Kind == TokenKind::bReg || CurToken.Kind == TokenKind::tReg ||
+ CurToken.Kind == TokenKind::uReg || CurToken.Kind == TokenKind::sReg) &&
+ "Expects to only be invoked starting at given keyword");
+
+ switch (CurToken.Kind) {
+ case TokenKind::bReg:
+ Register->ViewType = RegisterType::BReg;
+ break;
+ case TokenKind::tReg:
+ Register->ViewType = RegisterType::TReg;
+ break;
+ case TokenKind::uReg:
+ Register->ViewType = RegisterType::UReg;
+ break;
+ case TokenKind::sReg:
+ Register->ViewType = RegisterType::SReg;
+ break;
+ default:
+ break; // Unreachable given Try + assert pattern
+ }
+
+ if (handleUIntLiteral(&Register->Number))
+ return true; // propogate NumericLiteralParser error
+
+ return false;
+}
+
+bool RootSignatureParser::handleUIntLiteral(uint32_t *X) {
+ // Parse the numeric value and do semantic checks on its specification
+ clang::NumericLiteralParser Literal(CurToken.NumSpelling, CurToken.TokLoc,
+ PP.getSourceManager(), PP.getLangOpts(),
+ PP.getTargetInfo(), PP.getDiagnostics());
+ if (Literal.hadError)
+ return true; // Error has already been reported so just return
+
+ assert(Literal.isIntegerLiteral() && "IsNumberChar will only support digits");
+
+ llvm::APSInt Val = llvm::APSInt(32, false);
+ if (Literal.GetIntegerValue(Val)) {
+ // Report that the value has overflowed
+ PP.getDiagnostics().Report(CurToken.TokLoc,
+ diag::err_hlsl_number_literal_overflow)
+ << 0 << CurToken.NumSpelling;
+ return true;
+ }
+
+ *X = Val.getExtValue();
+ return false;
+}
+
bool RootSignatureParser::peekExpectedToken(TokenKind Expected) {
return peekExpectedToken(ArrayRef{Expected});
}
@@ -139,6 +275,7 @@ bool RootSignatureParser::consumeExpectedToken(TokenKind Expected,
case diag::err_expected:
DB << Expected;
break;
+ case diag::err_hlsl_unexpected_end_of_params:
case diag::err_expected_either:
case diag::err_expected_after:
DB << Expected << Context;
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index acdf455a5d6aa..5b162e9a1b4cd 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -130,10 +130,10 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
const llvm::StringLiteral Source = R"cc(
DescriptorTable(
- CBV(),
- SRV(),
- Sampler(),
- UAV()
+ CBV(b0),
+ SRV(space = 3, t42),
+ Sampler(s987, space = +2),
+ UAV(u4294967294)
),
DescriptorTable()
)cc";
@@ -155,18 +155,34 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
RootElement Elem = Elements[0];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::BReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 0u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u);
Elem = Elements[1];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::TReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 42u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 3u);
Elem = Elements[2];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::SReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 987u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 2u);
Elem = Elements[3];
ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+ RegisterType::UReg);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 4294967294u);
+ ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u);
Elem = Elements[4];
ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
@@ -176,6 +192,32 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
Elem = Elements[5];
ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 0u);
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, ValidTrailingCommaTest) {
+ // This test will checks we can handling trailing commas ','
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(b0, ),
+ SRV(t42),
+ )
+ )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_TRUE(Consumer->isSatisfied());
}
@@ -237,6 +279,102 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedEndOfStreamTest) {
// Test correct diagnostic produced - end of stream
Consumer->setExpected(diag::err_expected_after);
+
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidMissingParameterTest) {
+ // This test will check that the parsing fails due a mandatory
+ // parameter (register) not being specified
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV()
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_rootsig_missing_param);
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryParameterTest) {
+ // This test will check that the parsing fails due the same mandatory
+ // parameter being specified multiple times
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(b32, b84)
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_rootsig_repeat_param);
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedOptionalParameterTest) {
+ // This test will check that the parsing fails due the same optional
+ // parameter being specified multiple times
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(space = 2, space = 0)
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_rootsig_repeat_param);
+ ASSERT_TRUE(Parser.parse());
+
+ ASSERT_TRUE(Consumer->isSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedNumberTest) {
+ // This test will check that the lexing fails due to an integer overflow
+ const llvm::StringLiteral Source = R"cc(
+ DescriptorTable(
+ CBV(b4294967296)
+ )
+ )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 correct diagnostic produced
+ Consumer->setExpected(diag::err_hlsl_number_literal_overflow);
ASSERT_TRUE(Parser.parse());
ASSERT_TRUE(Consumer->isSatisfied());
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index c1b67844c747f..0ae8879b6c7d5 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -23,6 +23,13 @@ namespace rootsig {
// Definitions of the in-memory data layout structures
+// Models the different registers: bReg | tReg | uReg | sReg
+enum class RegisterType { BReg, TReg, UReg, SReg };
+struct Register {
+ RegisterType ViewType;
+ uint32_t Number;
+};
+
// Models the end of a descriptor table and stores its visibility
struct DescriptorTable {
uint32_t NumClauses = 0; // The number of clauses in the table
@@ -32,11 +39,19 @@ struct DescriptorTable {
using ClauseType = llvm::dxil::ResourceClass;
struct DescriptorTableClause {
ClauseType Type;
+ Register Register;
+ uint32_t Space = 0;
};
// Models RootElement : DescriptorTable | DescriptorTableClause
using RootElement = std::variant<DescriptorTable, DescriptorTableClause>;
+// ParamType is used as an 'any' type that will reference to a parameter in
+// RootElement. Each variant of ParamType is expected to have a Parse method
+// defined that will be dispatched on when we are attempting to parse a
+// parameter
+using ParamType = std::variant<uint32_t *, Register *>;
+
} // namespace rootsig
} // namespace hlsl
} // namespace llvm
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
inbelic
commented
Apr 1, 2025
inbelic
commented
Apr 1, 2025
added 3 commits
April 2, 2025 17:42
- defines `ParamType` as a way to represent a reference to some parameter in a root signature - defines `ParseParam` and `ParseParams` as an infastructure to define how the parameters of a given struct should be parsed in an orderless manner
inbelic
force-pushed
the
inbelic/rs-parse-param
branch
from
751e8a1 to
156e5fe
Compare
inbelic
force-pushed
the
inbelic/rs-parse-param
branch
from
156e5fe to
3a598fd
Compare
inbelic
commented
Apr 3, 2025
| // RootElement. Each variant of ParamType is expected to have a Parse method | ||
| // defined that will be dispatched on when we are attempting to parse a | ||
| // parameter | ||
| using ParamType = std::variant<uint32_t *, Register *>; |
There was a problem hiding this comment.
This is probably better moved to ParseHLSLRootSignature.h since it is specific to there
There was a problem hiding this comment.
Can be removed completely now if we use the struct approach
joaosaffran
reviewed
Apr 4, 2025
llvm-beanz
reviewed
Apr 8, 2025
| return true; | ||
|
|
||
| if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after, | ||
| CurToken.TokKind)) | ||
| llvm::SmallDenseMap<TokenKind, ParamType> Params = { |
There was a problem hiding this comment.
Hypothetically, based on the grammar what is the maximum number of token->param mappings we could have here?
The reason I'm asking: is it better to have a map with dynamic allocations and lookup, or just a struct full of optionals to serve as the state?
There was a problem hiding this comment.
For descriptor table clauses specifically it will grow to 6 params, which would be maintainable as a struct. But if we want to reuse the parseParams logic for StaticSampler then a common state struct would be around ~20 members.
Notably, this mapping also serves as a mapping to which parse method should be invoked based on the encountered token, it is not immediately clear how we would retain that info using the optionals struct. Although maybe you are implicitly suggesting that we don't have such a generic parseParams method.
If we are concerned about dynamic allocations/lookup, the size of this mapping is known statically, so we could also do something like:
template <usize N>
struct ParamMap {
TokenKind Kinds[N];
ParamType Types[N];
bool Mandatory[N];
bool Seen[N];
}
WDYT?
There was a problem hiding this comment.
I'm a bit torn. The pattern used in Clang for things that can be parsed in arbitrary order (like attributes) might be a bit overkill. Clang fully separates parsing from semantic analysis, so it parses all your attributes separate from converting them into AST nodes and attaching them to the proper parent.
The structure-of-array approach storing all tokens has some scalability concerns to me, and I'm not sure the genericness-gives benefit. Similarly I'm not sure the map approach as implemented is the right approach.
My gut feeling is that we probably shouldn't use the same implementation for parsing descriptor table parameters and sampler parameters.
I think a parameters structure for 6 parameters with either bools (which could be uint32_t bitfield members) or optionals to signal if they are seen makes sense. I suspect if they're mandatory should be something we know from context and don't need additional tracking of.
There was a problem hiding this comment.
Hm, okay. I think that is where we disagree then: whether it is worthwhile to make this generic or not.
I will make the (opinionated) argument for having it implemented generically:
- There isn't much of a difference in how we will parse the different root parameter types,
RootSBV(...), CBV(...), StaticSampler(...), RootConstants(...), etc, so, adding their respective parse methods in future prs will just be something like:
bool RootSignatureParser::parseStaticSampler() {
... // parse initial '('
using StaticSamplerParams = ParamMap<13>; // N = # of params locally
StaticSampler Sampler;
StaticSamplerParams Params = {
/* Kinds=*/ = {
TokenKind::sReg,
TokenKind::kw_filter,
...
},
/* ParamType=*/ = {
&Sampler.Register
&Sampler.Filter,
...
},
/*Seen=*/ 0x0,
/*Mandatory=*/0x1 // only register is required
};
if (parseParams(Params))
return true;
... // parse closing ')' and add Sampler to elements
}
- We can contrast that with DXC, here, where it needs to redefine the same "seen" and "mandatory" functionality over and over again.
- I assume that if we don't want to have a generic method in clang then the code flow would follow a similar pattern as DXC (maybe I don't understand the struct approach correctly and that is a wrong assumption?).
- Therefore, using a generic way to parse the parameters of root parameter types will be clearer in its intent (it is declarative) and easier to extend (instead of copying functionality over) when we add the other parts of the RootSignatureParser
These reasons are why I went with the current generic implementation.
Regarding scalability, the struct of arrays or the map will have a statically known N elements (or pairs), where N is the number of parameters for a given root parameter type. (N is not equal to the total number of token kinds). The largest N would be is for StaticSamplers with 13, and then for example DescriptorTableClause it is 5. And we could make Mandatory/Seen just be two uint32_t bitfields.
There was a problem hiding this comment.
Hm, okay. I think that is where we disagree then: whether it is worthwhile to make this generic or not.
I will make the (opinionated) argument for having it implemented generically:
- There isn't much of a difference in how we will parse the different root parameter types,
RootSBV(...), CBV(...), StaticSampler(...), RootConstants(...), etc, so, adding their respective parse methods in future prs will just be something like:
I don't think what you've described int he code example here is where the genericness becomes a problem.
- We can contrast that with DXC, here, where it needs to redefine the same "seen" and "mandatory" functionality over and over again.
- I assume that if we don't want to have a generic method in clang then the code flow would follow a similar pattern as DXC (maybe I don't understand the struct approach correctly and that is a wrong assumption?).
I think you draw a false equivalence. You seem to be saying we either do this generically, or we do this like DXC... That's not what I'm saying. The approach that Clang takes generally in parsing is to parse things out, and from the parsed information construct a declaration or statement, which then gets validated during construction.
That is not how DXC's parser for root signatures works, nor is it what you're proposing, but maybe it's the better approach.
- Therefore, using a generic way to parse the parameters of root parameter types will be clearer in its intent (it is declarative) and easier to extend (instead of copying functionality over) when we add the other parts of the RootSignatureParser
I think the bigger question is at what layer of abstraction does being generic help and at what layer of abstraction is it a hinderance.
Having a parseRootParam function that "generically" parses a root-level parameter seems like a good idea, but should it remain generic based on the type of the parameter? Should we have a single "parseArgument" function? Maybe... Should these functions produce the same structural result for every parameter? These things are less clear to me.
These reasons are why I went with the current generic implementation.
Regarding scalability, the struct of arrays or the map will have a statically known
Nelements (or pairs), whereNis the number of parameters for a given root parameter type. (Nis not equal to the total number of token kinds). The largestNwould be is forStaticSamplerswith 13, and then for exampleDescriptorTableClauseit is 5. And we could makeMandatory/Seenjust be twouint32_tbitfields.
The reasons that led you to a generic solution, might be reasons why following existing patterns that Clang uses for order-independent parsing (like attributes), might be the better architectural decision. Maybe we should borrow more from Clang's AST design and have polymorphic returned objects rather than variants.
There was a problem hiding this comment.
I don't think what you've described int he code example here is where the genericness becomes a problem.
I think you draw a false equivalence. You seem to be saying we either do this generically, or we do this like DXC... That's not what I'm saying. The approach that Clang takes generally in parsing is to parse things out, and from the parsed information construct a declaration or statement, which then gets validated during construction.
That is not how DXC's parser for root signatures works, nor is it what you're proposing, but maybe it's the better approach.
I see, then I did misinterpret what the other approach could be.
IIUC in this context, we might have an intermediate object ParsedParamState used to represent parameter values. parseParams would go through and parse the values to this object. Then we can construct our in-memory structs (DescriptorTableClause) from this object and validate the parameter values were specified correctly.
The most recent commit is a prototype of this, expect we allow some validation to be done when parsing.
I think the bigger question is at what layer of abstraction does being generic help and at what layer of abstraction is it a hinderance.
I think a good case to illustrate what level of abstraction we want is to consider parsing the flags = FLAGS parameter, where FLAGS could be a variety of flag types (RootFlags, DescriptorRangeFlags, etc.
Given the root element, (RootCBV, CBV, etc), it will directly imply the expected flag type.
Imo, we should validate it is the expected flag type during parsing instead of validation. Otherwise, we are throwing out that information and are forced to parse the flags in a general manner, only to rederive what the valid type is later.
Similar with register types, if we have CBV we should only parse a b register.
This allows for better localized diag messages by raising an error earlier at the invalid register or flag type.
So, I don't think the abstraction to have all validation after parsing is beneficial. But we can have the ones that make sense to be when constructing the elements (checking which are mandatory), as this is more clear.
Having a
parseRootParamfunction that "generically" parses a root-level parameter seems like a good idea, but should it remain generic based on the type of the parameter? Should we have a single "parseArgument" function? Maybe... Should these functions produce the same structural result for every parameter? These things are less clear to me.
The reasons that led you to a generic solution, might be reasons why following existing patterns that Clang uses for order-independent parsing (like attributes), might be the better architectural decision. Maybe we should borrow more from Clang's AST design and have polymorphic returned objects rather than variants.
I do think that using variants and mapping the variant types to a parseMethod is more confusing than it needs to be. Using a stateful struct is more clear in assigning them and easy to follow. And it removes the need for having to work around having an any type with polymorphic objects or variants.
joaosaffran
approved these changes
Apr 9, 2025
pros: - more explicit mapping to what parse method should be called based on the current keyword/root element - removes complexity of using `std::visit` on the parameter types - allows for validations before the in-memory root element is constructed cons: - we will need to bind the parsed values to the in-memory representations for each element as opposed to just setting it directly. but it makes the validations performed more explicit notes: - this does not enforce that we should do all validations after parsing out, we do allow validations on the current token types (for instance with register types)
|
I'm not entirely convinced the generic Warning - lots of untested and never compiled code follows. Consider an API like so: struct ParsedParam {
std::optional<llvm::hlsl::rootsig::Register> Register;
std::optional<uint32_t> Space;
};
std::optional<ParsedParam> parseParameter(TokenKind RegType);This could then be used in `parseDescriptorClause for the various parameter kinds: DescriptorTableClause Clause;
std::optional<ParsedParam> P;
switch (ParamKind) {
default:
llvm_unreachable("Switch for consumed token was not provided");
case TokenKind::kw_CBV:
Clause.Type = ClauseType::CBuffer;
P = parseParameter(TokenKind::bReg);
break;
// ...
}
if (!P.has_value())
// diagnose
// otherwise we have a parameter!then parseParameter can just can follow the same pattern and just recurse into the various members: std::optional<ParsedParam> parseParameter(TokenKind RegType) {
if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
ParamKind))
return std::nullopt;
ParsedParam Result;
do {
if (tryConsumeExpectedToken(RegType)) {
if (Result.Register.has_value()) {
getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
<< CurToken.TokKind;
return true;
}
if (auto Register = parseRegister(Params.Register))
Result.Register = *Register;
}
if (tryConsumeExpectedToken(RootSignatureToken::Kind::kw_space)) {
if (Result.Space.has_value()) {
getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
<< CurToken.TokKind;
return true;
}
if (auto Space = parseUIntParam())
Result.Register = *Register;
}
} while (tryConsumeExpectedToken(TokenKind::pu_comma));
if (!consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/ParamKind))
return std::nullopt;
if (!Result.Register.has_value()) {
getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
<< ExpectedRegister;
return std::nullopt;
}
return Result;This also makes the functions match the shape of the grammar formulations in the EBNF notation. I suspect that any code duplication we find by doing it this way would be fairly easy to clean up with a few helper functions here and there, and since the functions should stay relatively short I think it keeps it reasonably simple. |
inbelic
force-pushed
the
inbelic/rs-parse-param
branch
from
2df5d6d to
86fde97
Compare
|
I don't have any strong arguments against structuring it like described and have updated accordingly. Since we are changing the calling convention of the Changes to make the calling convention consistent with those methods are in the branch here. I will create the clean up pr once this one merges |
bogner
approved these changes
Apr 17, 2025
inbelic
changed the title
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/204/builds/6836 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/203/builds/8023 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/23055 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/6814 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/17844 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/17377 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/17759 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/21478 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/3703 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/14699 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/99/builds/5985 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 18, 2025
… the descriptor table clause params" (#136252) Reverts llvm/llvm-project#133800 Reverting to resolve the introduce naming collisions.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/92/builds/17375 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Defines
ParseDescriptorTableClauseParamsto establish the pattern of how we will parse parameters in root signatures. Namely, to use recursive descent parsing in a way that follows closely to the EBNF notation definition in the root signature spec.Implements parsing of two param types:
UInt32andRegisterto demonstrate the parsing implementation and allow for unit testingChanges the calling convention to use
std::optionalreturn values instead of boolean error returns and parameters by referencePart two of implementing: #126569