Skip to content

[HLSL][RootSignature] Implement initial parsing of the descriptor table clause params #133800

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Apr 18, 2025
Merged

[HLSL][RootSignature] Implement initial parsing of the descriptor table clause params #133800

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9ff87eb
[HLSL][RootSignature] Add infastructure to parse parameters
Mar 31, 2025
180c33c
define ParseUInt and ParseRegister to plug into parameters
Mar 27, 2025
ebab0ca
self-review: use consumeExpectedToken api to report unexpected_end_of…
Mar 31, 2025
cd5c142
self-review: fix-up comments
Apr 2, 2025
490e6b9
rebase changes: update to build issue fixes
Apr 2, 2025
8155c60
clang-formatting
Apr 2, 2025
3a598fd
self-review: clang-format fix up
Apr 2, 2025
1d2bd91
review: prototype using a stateful struct for parsed params
Apr 11, 2025
5d1ec2f
review: remove abstraction of parsing arbitrary params
Apr 17, 2025
86fde97
NFC review: update calling conventions to use std::optional instead o…
Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion clang/include/clang/Basic/DiagnosticParseKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
40 changes: 40 additions & 0 deletions clang/include/clang/Parse/ParseHLSLRootSignature.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,46 @@ class RootSignatureParser {
bool parseDescriptorTable();
bool parseDescriptorTableClause();

/// Each unique ParamType will have a custom parse method defined that can be
/// invoked to set a value to the referenced paramtype.
///
/// 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<RootSignatureToken::Kind, ParamType> Params = {
/// RootSignatureToken::Kind::bReg, &Clause.Register,
/// RootSignatureToken::Kind::kw_space, &Clause.Space
/// };
/// SmallDenseSet<RootSignatureToken::Kind> Mandatory = {
/// RootSignatureToken::Kind::bReg
/// };
///
/// 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<RootSignatureToken::Kind,
llvm::hlsl::rootsig::ParamType> &Params,
llvm::SmallDenseSet<RootSignatureToken::Kind> &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(); }

Expand Down
169 changes: 155 additions & 14 deletions clang/lib/Parse/ParseHLSLRootSignature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -41,12 +43,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;
}

Expand All @@ -72,12 +73,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;
Expand All @@ -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<TokenKind, ParamType> Params = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@inbelic inbelic Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 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.

{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 ParseParamTypeMethods : Ts... {
using Ts::operator()...;
};
template <class... Ts>
ParseParamTypeMethods(Ts...) -> ParseParamTypeMethods<Ts...>;

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<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.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(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});
}
Expand All @@ -141,6 +281,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;
Expand Down
Loading