[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
inbelic wrote: 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 `parse.*` methods to use `std::optional` I had originally also updated the `parseDescriptorTable` and `parseDescriptorTableClause` as well. But it made the diff difficult to read. Changes to make the calling convention consistent with those methods are in the branch [here](https://github.com/inbelic/llvm-project/tree/inbelic/rs-update-call-convs). I will create the clean up pr once this one merges https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/bogner approved this pull request. https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic edited https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic updated
https://github.com/llvm/llvm-project/pull/133800
>From 9ff87eb37437dc92a554d1d89b236e9a13249694 Mon Sep 17 00:00:00 2001
From: Finn Plummer
Date: Mon, 31 Mar 2025 18:29:26 +
Subject: [PATCH 01/10] [HLSL][RootSignature] Add infastructure to parse
parameters
- 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
---
.../clang/Basic/DiagnosticParseKinds.td | 4 +-
.../clang/Parse/ParseHLSLRootSignature.h | 32
clang/lib/Parse/ParseHLSLRootSignature.cpp| 51 +++
.../llvm/Frontend/HLSL/HLSLRootSignature.h| 6 +++
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 954f538e15026..2a0bf81c68e44 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1830,8 +1830,10 @@ 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'">;
} // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 18cc2c6692551..55d84f91b8834 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -69,6 +69,38 @@ 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 Params = {
+ ///TokenKind::bReg, &Clause.Register,
+ ///TokenKind::kw_space, &Clause.Space
+ /// };
+ /// SmallDenseSet 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 &Params,
+ llvm::SmallDenseSet &Mandatory);
+
/// 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 93a9689ebdf72..54c51e56b84dd 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -120,6 +120,57 @@ bool RootSignatureParser::parseDescriptorTableClause() {
return false;
}
+// Helper struct so that we can use the overloaded notation of std::visit
+template struct ParseMethods : Ts... { using Ts::operator()...;
};
+template ParseMethods(Ts...) -> ParseMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ bool Error = true;
+ std::visit(ParseMethods{}, Ref);
+
+ return Error;
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet 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 = S
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic updated
https://github.com/llvm/llvm-project/pull/133800
>From 9ff87eb37437dc92a554d1d89b236e9a13249694 Mon Sep 17 00:00:00 2001
From: Finn Plummer
Date: Mon, 31 Mar 2025 18:29:26 +
Subject: [PATCH 01/11] [HLSL][RootSignature] Add infastructure to parse
parameters
- 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
---
.../clang/Basic/DiagnosticParseKinds.td | 4 +-
.../clang/Parse/ParseHLSLRootSignature.h | 32
clang/lib/Parse/ParseHLSLRootSignature.cpp| 51 +++
.../llvm/Frontend/HLSL/HLSLRootSignature.h| 6 +++
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 954f538e15026..2a0bf81c68e44 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1830,8 +1830,10 @@ 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'">;
} // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 18cc2c6692551..55d84f91b8834 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -69,6 +69,38 @@ 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 Params = {
+ ///TokenKind::bReg, &Clause.Register,
+ ///TokenKind::kw_space, &Clause.Space
+ /// };
+ /// SmallDenseSet 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 &Params,
+ llvm::SmallDenseSet &Mandatory);
+
/// 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 93a9689ebdf72..54c51e56b84dd 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -120,6 +120,57 @@ bool RootSignatureParser::parseDescriptorTableClause() {
return false;
}
+// Helper struct so that we can use the overloaded notation of std::visit
+template struct ParseMethods : Ts... { using Ts::operator()...;
};
+template ParseMethods(Ts...) -> ParseMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ bool Error = true;
+ std::visit(ParseMethods{}, Ref);
+
+ return Error;
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet 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 = S
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
bogner wrote:
I'm not entirely convinced the generic `ParsedParamState` is the right level of
abstraction. In some ways the declarative nature is nice, but I think it's
quite a bit more complex than a simple recursive descent here.
Warning - lots of untested and never compiled code follows. Consider an API
like so:
```c++
struct ParsedParam {
std::optional Register;
std::optional Space;
};
std::optional parseParameter(TokenKind RegType);
```
This could then be used in `parseDescriptorClause for the various parameter
kinds:
```c++
DescriptorTableClause Clause;
std::optional 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:
```c++
std::optional 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.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -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;
+// 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;
inbelic wrote:
Can be removed completely now if we use the struct approach
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
inbelic wrote:
> 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 `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.
> 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.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic updated
https://github.com/llvm/llvm-project/pull/133800
>From 9ff87eb37437dc92a554d1d89b236e9a13249694 Mon Sep 17 00:00:00 2001
From: Finn Plummer
Date: Mon, 31 Mar 2025 18:29:26 +
Subject: [PATCH 1/8] [HLSL][RootSignature] Add infastructure to parse
parameters
- 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
---
.../clang/Basic/DiagnosticParseKinds.td | 4 +-
.../clang/Parse/ParseHLSLRootSignature.h | 32
clang/lib/Parse/ParseHLSLRootSignature.cpp| 51 +++
.../llvm/Frontend/HLSL/HLSLRootSignature.h| 6 +++
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 954f538e15026..2a0bf81c68e44 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1830,8 +1830,10 @@ 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'">;
} // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 18cc2c6692551..55d84f91b8834 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -69,6 +69,38 @@ 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 Params = {
+ ///TokenKind::bReg, &Clause.Register,
+ ///TokenKind::kw_space, &Clause.Space
+ /// };
+ /// SmallDenseSet 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 &Params,
+ llvm::SmallDenseSet &Mandatory);
+
/// 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 93a9689ebdf72..54c51e56b84dd 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -120,6 +120,57 @@ bool RootSignatureParser::parseDescriptorTableClause() {
return false;
}
+// Helper struct so that we can use the overloaded notation of std::visit
+template struct ParseMethods : Ts... { using Ts::operator()...;
};
+template ParseMethods(Ts...) -> ParseMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ bool Error = true;
+ std::visit(ParseMethods{}, Ref);
+
+ return Error;
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet 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 = See
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
llvm-beanz wrote:
> 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](https://github.com/microsoft/DirectXShaderCompiler/blob/c940161bb3398ff988fafc343ed1623d4a3fad6c/tools/clang/lib/Parse/HLSLRootSignature.cpp#L1301),
> 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 `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.
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.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic edited https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic edited https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
inbelic wrote:
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() {
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;
...
}
```
- We can contrast that with DXC,
[here](https://github.com/microsoft/DirectXShaderCompiler/blob/c940161bb3398ff988fafc343ed1623d4a3fad6c/tools/clang/lib/Parse/HLSLRootSignature.cpp#L1301),
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 4. And we could make `Mandatory/Seen`
just be two `uint32_t` bitfields.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
llvm-beanz wrote:
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.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/joaosaffran approved this pull request. LGTM, not an expert, though. https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet 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 struct ParseParamTypeMethods : Ts... {
+ using Ts::operator()...;
+};
+template
+ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseParamTypeMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+return consumeExpectedToken(TokenKind::pu_equal,
+diag::err_expected_after,
+CurToken.TokKind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+if (Seen.contains(CurToken.TokKind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.TokKind;
+ return true;
+}
+Seen.insert(CurToken.TokKind);
+
+if (parseParam(Params[CurToken.TokKind]))
+ 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.TokKind == 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.TokKind) ||
+ handleUIntLiteral(X);
+}
+
+bool RootSignatureParser::parseRegister(Register *Register) {
+ assert((CurToken.TokKind == TokenKind::bReg ||
+ CurToken.TokKind == TokenKind::tReg ||
+ CurToken.TokKind == TokenKind::uReg ||
+ CurToken.TokKind == TokenKind::sReg) &&
+ "Expects to only be invoked starting at given keyword");
+
+ switch (CurToken.TokKind) {
+ default:
+llvm_unreachable("Switch for consumed token was not provided");
+ 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;
+ }
+
+ if (handleUIntLiteral(&Register->Number))
+return true; // propogate NumericLiteralParser error
+
+ return false;
+}
+
+bool RootSignatureParser::handleUIntLiteral(uin
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
inbelic wrote:
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
struct ParamMap {
TokenKind Kinds[N];
ParamType Types[N];
bool Mandatory[N];
bool Seen[N];
}
```
WDYT?
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
llvm-beanz wrote:
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?
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet 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 struct ParseParamTypeMethods : Ts... {
+ using Ts::operator()...;
+};
+template
+ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseParamTypeMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+return consumeExpectedToken(TokenKind::pu_equal,
+diag::err_expected_after,
+CurToken.TokKind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+if (Seen.contains(CurToken.TokKind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.TokKind;
+ return true;
+}
+Seen.insert(CurToken.TokKind);
+
+if (parseParam(Params[CurToken.TokKind]))
+ 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.TokKind == 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.TokKind) ||
+ handleUIntLiteral(X);
+}
+
+bool RootSignatureParser::parseRegister(Register *Register) {
+ assert((CurToken.TokKind == TokenKind::bReg ||
+ CurToken.TokKind == TokenKind::tReg ||
+ CurToken.TokKind == TokenKind::uReg ||
+ CurToken.TokKind == TokenKind::sReg) &&
+ "Expects to only be invoked starting at given keyword");
+
+ switch (CurToken.TokKind) {
+ default:
+llvm_unreachable("Switch for consumed token was not provided");
+ 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;
+ }
+
+ if (handleUIntLiteral(&Register->Number))
+return true; // propogate NumericLiteralParser error
+
+ return false;
+}
+
+bool RootSignatureParser::handleUIntLiteral(uin
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet 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 struct ParseParamTypeMethods : Ts... {
+ using Ts::operator()...;
+};
+template
+ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseParamTypeMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+return consumeExpectedToken(TokenKind::pu_equal,
+diag::err_expected_after,
+CurToken.TokKind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+if (Seen.contains(CurToken.TokKind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.TokKind;
+ return true;
+}
+Seen.insert(CurToken.TokKind);
+
+if (parseParam(Params[CurToken.TokKind]))
+ 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.TokKind == TokenKind::pu_equal &&
inbelic wrote:
The assert is there to communicate that it should only be called after
consuming the preceding `=` symbol, when specifying a parameter value `space =
u32`, `numDescriptors = u32`.
Maybe you are confused why it is restricted to that case? If so, we can modify
the assert to include other valid tokens that come before this token, but keep
a clear communication about what is allowed.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/inbelic edited https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/joaosaffran edited https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/joaosaffran commented: Few questions to help me understand better the changes being introduced here. https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet 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 struct ParseParamTypeMethods : Ts... {
+ using Ts::operator()...;
+};
+template
+ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseParamTypeMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+return consumeExpectedToken(TokenKind::pu_equal,
+diag::err_expected_after,
+CurToken.TokKind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+if (Seen.contains(CurToken.TokKind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.TokKind;
+ return true;
+}
+Seen.insert(CurToken.TokKind);
+
+if (parseParam(Params[CurToken.TokKind]))
+ 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.TokKind == 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.TokKind) ||
+ handleUIntLiteral(X);
+}
+
+bool RootSignatureParser::parseRegister(Register *Register) {
+ assert((CurToken.TokKind == TokenKind::bReg ||
+ CurToken.TokKind == TokenKind::tReg ||
+ CurToken.TokKind == TokenKind::uReg ||
+ CurToken.TokKind == TokenKind::sReg) &&
+ "Expects to only be invoked starting at given keyword");
+
+ switch (CurToken.TokKind) {
+ default:
+llvm_unreachable("Switch for consumed token was not provided");
+ 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;
+ }
+
+ if (handleUIntLiteral(&Register->Number))
+return true; // propogate NumericLiteralParser error
+
+ return false;
+}
+
+bool RootSignatureParser::handleUIntLiteral(uin
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -89,37 +88,178 @@ bool RootSignatureParser::parseDescriptorTableClause() {
CurToken.TokKind == TokenKind::kw_UAV ||
CurToken.TokKind == TokenKind::kw_Sampler) &&
"Expects to only be invoked starting at given keyword");
+ TokenKind ParamKind = CurToken.TokKind; // retain for diagnostics
DescriptorTableClause Clause;
- switch (CurToken.TokKind) {
+ 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.TokKind))
+ ParamKind))
return true;
- if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
- CurToken.TokKind))
+ llvm::SmallDenseMap Params = {
+ {ExpectedRegister, &Clause.Register},
+ {TokenKind::kw_space, &Clause.Space},
+ };
+ llvm::SmallDenseSet 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 struct ParseParamTypeMethods : Ts... {
+ using Ts::operator()...;
+};
+template
+ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods;
+
+bool RootSignatureParser::parseParam(ParamType Ref) {
+ return std::visit(
+ ParseParamTypeMethods{
+ [this](Register *X) -> bool { return parseRegister(X); },
+ [this](uint32_t *X) -> bool {
+return consumeExpectedToken(TokenKind::pu_equal,
+diag::err_expected_after,
+CurToken.TokKind) ||
+ parseUIntParam(X);
+ },
+ },
+ Ref);
+}
+
+bool RootSignatureParser::parseParams(
+llvm::SmallDenseMap &Params,
+llvm::SmallDenseSet &Mandatory) {
+
+ // Initialize a vector of possible keywords
+ SmallVector Keywords;
+ for (auto Pair : Params)
+Keywords.push_back(Pair.first);
+
+ // Keep track of which keywords have been seen to report duplicates
+ llvm::SmallDenseSet Seen;
+
+ while (tryConsumeExpectedToken(Keywords)) {
+if (Seen.contains(CurToken.TokKind)) {
+ getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+ << CurToken.TokKind;
+ return true;
+}
+Seen.insert(CurToken.TokKind);
+
+if (parseParam(Params[CurToken.TokKind]))
+ 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.TokKind == TokenKind::pu_equal &&
joaosaffran wrote:
This made me confused. The only place this is called is parsing
`TokenKind::pu_equal` before calling it, so it seems confusing, IMHO, that this
assert is here.
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
https://github.com/joaosaffran edited https://github.com/llvm/llvm-project/pull/133800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [HLSL][RootSignature] Add infastructure to parse parameters (PR #133800)
@@ -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;
+// 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;
inbelic wrote:
This is probably better moved to `ParseHLSLRootSignature.h` since it is
specific to there
https://github.com/llvm/llvm-project/pull/133800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
