================
@@ -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 = {
----------------
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
