review: remove abstraction of parsing arbitrary params

Author: Finn Plummer

clang/include/clang/Parse/ParseHLSLRootSignature.h

@@ -69,59 +69,13 @@ class RootSignatureParser {
   bool parseDescriptorTable();
   bool parseDescriptorTableClause();
 
-  /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
-  /// order and only exactly once. `ParsedParamState` provides a common
-  /// stateful structure to facilitate a common abstraction over collecting
-  /// parameters. By having a common return type we can follow clang's pattern
-  /// of first parsing out the values and then validating when we construct
-  /// the corresponding in-memory RootElements.
-  struct ParsedParamState {
-    // Parameter state to hold the parsed values. The value is only guarentted
-    // to be correct if one of its keyword bits is set in `Seen`.
-    // Eg) if any of the seen bits for `bReg, tReg, uReg, sReg` are set, then
-    // Register will have an initialized value
-    llvm::hlsl::rootsig::Register Register;
-    uint32_t Space;
-
-    // Seen retains whether or not the corresponding keyword has been
-    // seen
-    uint32_t Seen = 0u;
-
-    // Valid keywords for the different parameter types and its corresponding
-    // parsed value
-    ArrayRef<RootSignatureToken::Kind> Keywords;
-
-    // Context of which RootElement is retained to dispatch on the correct
-    // parse method.
-    // Eg) If we encounter kw_flags, it will depend on Context
-    // we are parsing to determine which `parse*Flags` method to call
-    RootSignatureToken::Kind Context;
-
-    // Retain start of params for reporting diagnostics
-    SourceLocation StartLoc;
-
-    // Must provide the starting context of the parameters
-    ParsedParamState(ArrayRef<RootSignatureToken::Kind> Keywords,
-                     RootSignatureToken::Kind Context, SourceLocation StartLoc)
-        : Keywords(Keywords), Context(Context), StartLoc(StartLoc) {}
-
-    // Helper functions to interact with Seen
-    size_t getKeywordIdx(RootSignatureToken::Kind Keyword);
-    bool checkAndSetSeen(RootSignatureToken::Kind Keyword);
-    bool checkAndClearSeen(RootSignatureToken::Kind Keyword);
+  struct ParsedParams {
+    std::optional<llvm::hlsl::rootsig::Register> Register;
+    std::optional<uint32_t> Space;
   };
-
-  /// Root signature parameters follow the form of `keyword` `=` `value`, or,
-  /// are a register. Given a keyword and the context of which `RootElement`
-  /// type we are parsing, we can dispatch onto the correct parseMethod to
-  /// parse a value into the `ParsedParamState`.
-  ///
-  /// This function implements the dispatch onto the correct parse method.
-  bool parseParam(ParsedParamState &Params);
-
-  /// Parses out a `ParsedParamState` for the caller to use for construction
+  /// Parses out a `ParsedParams` for the caller to use for construction
   /// of the in-memory representation of a Root Element.
-  bool parseParams(ParsedParamState &Params);
+  bool parseDescriptorTableClauseParams(ParsedParams &Params, RootSignatureToken::Kind RegType);
 
   /// Parameter parse methods corresponding to a ParamType
   bool parseUIntParam(uint32_t &X);

clang/lib/Parse/ParseHLSLRootSignature.cpp

@@ -117,26 +117,21 @@ bool RootSignatureParser::parseDescriptorTableClause() {
                            ParamKind))
     return true;
 
-  TokenKind Keywords[2] = {
-      ExpectedRegister,
-      TokenKind::kw_space,
-  };
-  ParsedParamState Params(Keywords, ParamKind, CurToken.TokLoc);
-  if (parseParams(Params))
+  ParsedParams Result;
+  if (parseDescriptorTableClauseParams(Result, ExpectedRegister))
     return true;
 
-  // Mandatory parameters:
-  if (!Params.checkAndClearSeen(ExpectedRegister)) {
+  // Check mandatory parameters were provided
+  if (!Result.Register.has_value()) {
     getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
         << ExpectedRegister;
     return true;
   }
 
-  Clause.Register = Params.Register;
+  Clause.Register = *Result.Register;
 
-  // Optional parameters:
-  if (Params.checkAndClearSeen(TokenKind::kw_space))
-    Clause.Space = Params.Space;
+  if (Result.Space)
+    Clause.Space = *Result.Space;
 
   if (consumeExpectedToken(TokenKind::pu_r_paren,
                            diag::err_hlsl_unexpected_end_of_params,
@@ -147,66 +142,36 @@ bool RootSignatureParser::parseDescriptorTableClause() {
   return false;
 }
 
-size_t RootSignatureParser::ParsedParamState::getKeywordIdx(
-    RootSignatureToken::Kind Keyword) {
-  ArrayRef KeywordRef = Keywords;
-  auto It = llvm::find(KeywordRef, Keyword);
-  assert(It != KeywordRef.end() && "Did not provide a valid param keyword");
-  return std::distance(KeywordRef.begin(), It);
-}
-
-bool RootSignatureParser::ParsedParamState::checkAndSetSeen(
-    RootSignatureToken::Kind Keyword) {
-  size_t Idx = getKeywordIdx(Keyword);
-  bool WasSeen = Seen & (1 << Idx);
-  Seen |= 1u << Idx;
-  return WasSeen;
-}
-
-bool RootSignatureParser::ParsedParamState::checkAndClearSeen(
-    RootSignatureToken::Kind Keyword) {
-  size_t Idx = getKeywordIdx(Keyword);
-  bool WasSeen = Seen & (1 << Idx);
-  Seen &= ~(1u << Idx);
-  return WasSeen;
-}
-
-bool RootSignatureParser::parseParam(ParsedParamState &Params) {
-  TokenKind Keyword = CurToken.TokKind;
-  if (Keyword == TokenKind::bReg || Keyword == TokenKind::tReg ||
-      Keyword == TokenKind::uReg || Keyword == TokenKind::sReg) {
-    return parseRegister(Params.Register);
-  }
-
-  if (consumeExpectedToken(TokenKind::pu_equal, diag::err_expected_after,
-                           Keyword))
-    return true;
-
-  switch (Keyword) {
-  case RootSignatureToken::Kind::kw_space:
-    return parseUIntParam(Params.Space);
-  default:
-    llvm_unreachable("Switch for consumed keyword was not provided");
-  }
-}
-
-bool RootSignatureParser::parseParams(ParsedParamState &Params) {
+bool RootSignatureParser::parseDescriptorTableClauseParams(ParsedParams &Params, TokenKind RegType) {
   assert(CurToken.TokKind == TokenKind::pu_l_paren &&
          "Expects to only be invoked starting at given token");
 
-  while (tryConsumeExpectedToken(Params.Keywords)) {
-    if (Params.checkAndSetSeen(CurToken.TokKind)) {
-      getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+  do {
+    if (tryConsumeExpectedToken(RegType)) {
+      if (Params.Register.has_value()) {
+        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
           << CurToken.TokKind;
-      return true;
+        return true;
+      }
+      Register Reg;
+      if (parseRegister(Reg))
+        return true;
+      Params.Register = Reg;
     }
-
-    if (parseParam(Params))
-      return true;
-
-    if (!tryConsumeExpectedToken(TokenKind::pu_comma))
-      break;
-  }
+    if (tryConsumeExpectedToken(TokenKind::kw_space)) {
+      if (Params.Space.has_value()) {
+        getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+          << CurToken.TokKind;
+        return true;
+      }
+      if (consumeExpectedToken(TokenKind::pu_equal))
+        return true;
+      uint32_t Space;
+      if (parseUIntParam(Space))
+        return true;
+      Params.Space = Space;
+    }
+  } while (tryConsumeExpectedToken(TokenKind::pu_comma));
 
   return false;
 }

llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h

@@ -46,12 +46,6 @@ struct DescriptorTableClause {
 // 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