[clang] [llvm] [HLSL][RootSignature] Implement Parsing of Descriptor Tables (PR #122982)

Finn Plummer via cfe-commits Wed, 05 Mar 2025 18:56:42 -0800

https://github.com/inbelic updated 
https://github.com/llvm/llvm-project/pull/122982


>From 9fe6b897b161f7e40e4a5662ab2cad4dceed174e Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Thu, 13 Feb 2025 22:56:52 +0000
Subject: [PATCH 01/15] [HLSL][RootSignature] Parse empty Descriptor Table Root
 Element

- defines the Parser class and an initial set of helper methods to
support consuming tokens. functionality is demonstrated through a simple
empty descriptor table test case
- defines an initial in-memory representation of a DescriptorTable
- implements a test harness that will be used to validate the correct
diagnostics are generated. it will construct a dummy pre-processor with
diagnostics consumer to do so
---
 .../clang/Basic/DiagnosticParseKinds.td       |   2 +
 .../clang/Parse/ParseHLSLRootSignature.h      |  69 ++++++
 clang/lib/Parse/CMakeLists.txt                |   1 +
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 138 +++++++++++
 clang/unittests/CMakeLists.txt                |   1 +
 clang/unittests/Parse/CMakeLists.txt          |  23 ++
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 217 ++++++++++++++++++
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    |  37 +++
 8 files changed, 488 insertions(+)
 create mode 100644 clang/include/clang/Parse/ParseHLSLRootSignature.h
 create mode 100644 clang/lib/Parse/ParseHLSLRootSignature.cpp
 create mode 100644 clang/unittests/Parse/CMakeLists.txt
 create mode 100644 clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
 create mode 100644 llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td 
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index c513dab810d1f..4c6adab80e60e 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1818,4 +1818,6 @@ def ext_hlsl_access_specifiers : ExtWarn<
 def err_hlsl_unsupported_component : Error<"invalid component '%0' used; 
expected 'x', 'y', 'z', or 'w'">;
 def err_hlsl_packoffset_invalid_reg : Error<"invalid resource class specifier 
'%0' for packoffset, expected 'c'">;
 
+// HLSL Root Signature Parser Diagnostics
+def err_hlsl_rootsig_unexpected_token_kind : Error<"got %1 but expected 
%select{|one of}0 the following token kind%select{|s}0: %2">;
 } // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
new file mode 100644
index 0000000000000..fa46bdd663a2c
--- /dev/null
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -0,0 +1,69 @@
+//===--- ParseHLSLRootSignature.h -------------------------------*- C++ 
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines the ParseHLSLRootSignature interface.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_PARSE_PARSEHLSLROOTSIGNATURE_H
+#define LLVM_CLANG_PARSE_PARSEHLSLROOTSIGNATURE_H
+
+#include "clang/Basic/DiagnosticParse.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/LexHLSLRootSignature.h"
+
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+
+#include "llvm/Frontend/HLSL/HLSLRootSignature.h"
+
+namespace clang {
+namespace hlsl {
+
+class RootSignatureParser {
+public:
+  RootSignatureParser(SmallVector<llvm::hlsl::rootsig::RootElement> &Elements,
+                      RootSignatureLexer &Lexer, clang::Preprocessor &PP);
+
+  /// Consumes tokens from the Lexer and constructs the in-memory
+  /// representations of the RootElements. Tokens are consumed until an
+  /// error is encountered or the end of the buffer.
+  ///
+  /// Returns true if a parsing error is encountered.
+  bool Parse();
+
+  DiagnosticsEngine &Diags() { return PP.getDiagnostics(); }
+
+private:
+  // Root Element helpers
+  bool ParseRootElement();
+  bool ParseDescriptorTable();
+
+  /// Invoke the Lexer to consume a token and update CurToken with the result
+  void ConsumeNextToken() { CurToken = Lexer.ConsumeToken(); }
+
+  /// Consumes the next token and report an error if it is not of the expected
+  /// kind.
+  ///
+  /// Returns true if there was an error reported.
+  bool ConsumeExpectedToken(TokenKind Expected);
+  bool ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected);
+
+private:
+  SmallVector<llvm::hlsl::rootsig::RootElement> &Elements;
+  RootSignatureLexer &Lexer;
+
+  clang::Preprocessor &PP;
+
+  RootSignatureToken CurToken;
+};
+
+} // namespace hlsl
+} // namespace clang
+
+#endif // LLVM_CLANG_PARSE_PARSEHLSLROOTSIGNATURE_H
diff --git a/clang/lib/Parse/CMakeLists.txt b/clang/lib/Parse/CMakeLists.txt
index 22e902f7e1bc5..00fde537bb9c6 100644
--- a/clang/lib/Parse/CMakeLists.txt
+++ b/clang/lib/Parse/CMakeLists.txt
@@ -14,6 +14,7 @@ add_clang_library(clangParse
   ParseExpr.cpp
   ParseExprCXX.cpp
   ParseHLSL.cpp
+  ParseHLSLRootSignature.cpp
   ParseInit.cpp
   ParseObjc.cpp
   ParseOpenMP.cpp
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
new file mode 100644
index 0000000000000..aeb33ded4d815
--- /dev/null
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -0,0 +1,138 @@
+#include "clang/Parse/ParseHLSLRootSignature.h"
+
+#include "llvm/Support/raw_ostream.h"
+
+using namespace llvm::hlsl::rootsig;
+
+namespace clang {
+namespace hlsl {
+
+static std::string FormatTokenKinds(ArrayRef<TokenKind> Kinds) {
+  std::string TokenString;
+  llvm::raw_string_ostream Out(TokenString);
+  bool First = true;
+  for (auto Kind : Kinds) {
+    if (!First)
+      Out << ", ";
+    switch (Kind) {
+    case TokenKind::invalid:
+      Out << "invalid identifier";
+      break;
+    case TokenKind::end_of_stream:
+      Out << "end of stream";
+      break;
+    case TokenKind::int_literal:
+      Out << "integer literal";
+      break;
+    case TokenKind::bReg:
+      Out << "b register";
+      break;
+    case TokenKind::tReg:
+      Out << "t register";
+      break;
+    case TokenKind::uReg:
+      Out << "u register";
+      break;
+    case TokenKind::sReg:
+      Out << "s register";
+      break;
+#define PUNCTUATOR(X, Y)                                                       
\
+  case TokenKind::pu_##X:                                                      
\
+    Out << #Y;                                                                 
\
+    break;
+#define KEYWORD(NAME)                                                          
\
+  case TokenKind::kw_##NAME:                                                   
\
+    Out << #NAME;                                                              
\
+    break;
+#define ENUM(NAME, LIT)                                                        
\
+  case TokenKind::en_##NAME:                                                   
\
+    Out << LIT;                                                                
\
+    break;
+#include "clang/Lex/HLSLRootSignatureTokenKinds.def"
+    }
+    First = false;
+  }
+
+  return TokenString;
+}
+
+// Parser Definitions
+
+RootSignatureParser::RootSignatureParser(SmallVector<RootElement> &Elements,
+                                         RootSignatureLexer &Lexer,
+                                         Preprocessor &PP)
+    : Elements(Elements), Lexer(Lexer), PP(PP), CurToken(SourceLocation()) {}
+
+bool RootSignatureParser::Parse() {
+  // Handle edge-case of empty RootSignature()
+  if (Lexer.EndOfBuffer())
+    return false;
+
+  // Iterate as many RootElements as possible
+  while (!ParseRootElement()) {
+    if (Lexer.EndOfBuffer())
+      return false;
+    if (ConsumeExpectedToken(TokenKind::pu_comma))
+      return true;
+  }
+
+  return true;
+}
+
+bool RootSignatureParser::ParseRootElement() {
+  if (ConsumeExpectedToken(TokenKind::kw_DescriptorTable))
+    return true;
+
+  // Dispatch onto the correct parse method
+  switch (CurToken.Kind) {
+  case TokenKind::kw_DescriptorTable:
+    return ParseDescriptorTable();
+  default:
+    llvm_unreachable("Switch for an expected token was not provided");
+  }
+  return true;
+}
+
+bool RootSignatureParser::ParseDescriptorTable() {
+  DescriptorTable Table;
+
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+    return true;
+
+  // Empty case:
+  if (!ConsumeExpectedToken(TokenKind::pu_r_paren)) {
+    Elements.push_back(Table);
+    return false;
+  }
+
+  return true;
+}
+
+// Is given token one of the expected kinds
+static bool IsExpectedToken(TokenKind Kind, ArrayRef<TokenKind> AnyExpected) {
+  for (auto Expected : AnyExpected)
+    if (Kind == Expected)
+      return true;
+  return false;
+}
+
+bool RootSignatureParser::ConsumeExpectedToken(TokenKind Expected) {
+  return ConsumeExpectedToken(ArrayRef{Expected});
+}
+
+bool RootSignatureParser::ConsumeExpectedToken(
+    ArrayRef<TokenKind> AnyExpected) {
+  ConsumeNextToken();
+  if (IsExpectedToken(CurToken.Kind, AnyExpected))
+    return false;
+
+  // Report unexpected token kind error
+  Diags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_unexpected_token_kind)
+      << (unsigned)(AnyExpected.size() != 1)
+      << FormatTokenKinds({CurToken.Kind})
+      << FormatTokenKinds(AnyExpected);
+  return true;
+}
+
+} // namespace hlsl
+} // namespace clang
diff --git a/clang/unittests/CMakeLists.txt b/clang/unittests/CMakeLists.txt
index 85d265426ec80..9b3ce8aa7de73 100644
--- a/clang/unittests/CMakeLists.txt
+++ b/clang/unittests/CMakeLists.txt
@@ -25,6 +25,7 @@ endfunction()
 
 add_subdirectory(Basic)
 add_subdirectory(Lex)
+add_subdirectory(Parse)
 add_subdirectory(Driver)
 if(CLANG_ENABLE_STATIC_ANALYZER)
   add_subdirectory(Analysis)
diff --git a/clang/unittests/Parse/CMakeLists.txt 
b/clang/unittests/Parse/CMakeLists.txt
new file mode 100644
index 0000000000000..eeb58174568cd
--- /dev/null
+++ b/clang/unittests/Parse/CMakeLists.txt
@@ -0,0 +1,23 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+add_clang_unittest(ParseTests
+  ParseHLSLRootSignatureTest.cpp
+  )
+clang_target_link_libraries(ParseTests
+  PRIVATE
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangFrontend
+  clangParse
+  clangSema
+  clangSerialization
+  clangTooling
+  )
+target_link_libraries(ParseTests
+  PRIVATE
+  LLVMTestingAnnotations
+  LLVMTestingSupport
+  clangTesting
+  )
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
new file mode 100644
index 0000000000000..f2ca933b4f39e
--- /dev/null
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -0,0 +1,217 @@
+//=== ParseHLSLRootSignatureTest.cpp - Parse Root Signature tests 
---------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticOptions.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Lex/HeaderSearchOptions.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/ModuleLoader.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
+
+#include "clang/Lex/LexHLSLRootSignature.h"
+#include "clang/Parse/ParseHLSLRootSignature.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace llvm::hlsl::rootsig;
+
+namespace {
+
+// Diagnostic helper for helper tests
+class ExpectedDiagConsumer : public DiagnosticConsumer {
+  virtual void anchor() {}
+
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                        const Diagnostic &Info) override {
+    if (!FirstDiag || !ExpectedDiagID.has_value()) {
+      Satisfied = false;
+      return;
+    }
+    FirstDiag = false;
+
+    Satisfied = ExpectedDiagID.value() == Info.getID();
+  }
+
+  bool FirstDiag = true;
+  bool Satisfied = false;
+  std::optional<unsigned> ExpectedDiagID;
+
+public:
+  void SetNoDiag() {
+    Satisfied = true;
+    ExpectedDiagID = std::nullopt;
+  }
+
+  void SetExpected(unsigned DiagID) {
+    Satisfied = false;
+    ExpectedDiagID = DiagID;
+  }
+
+  bool IsSatisfied() { return Satisfied; }
+};
+
+// The test fixture.
+class ParseHLSLRootSignatureTest : public ::testing::Test {
+protected:
+  ParseHLSLRootSignatureTest()
+      : FileMgr(FileMgrOpts), DiagID(new DiagnosticIDs()),
+        Consumer(new ExpectedDiagConsumer()),
+        Diags(DiagID, new DiagnosticOptions, Consumer),
+        SourceMgr(Diags, FileMgr), TargetOpts(new TargetOptions) {
+    // This is an arbitrarily chosen target triple to create the target info.
+    TargetOpts->Triple = "dxil";
+    Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
+  }
+
+  std::unique_ptr<Preprocessor> CreatePP(StringRef Source,
+                                         TrivialModuleLoader &ModLoader) {
+    std::unique_ptr<llvm::MemoryBuffer> Buf =
+        llvm::MemoryBuffer::getMemBuffer(Source);
+    SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(Buf)));
+
+    HeaderSearch HeaderInfo(std::make_shared<HeaderSearchOptions>(), SourceMgr,
+                            Diags, LangOpts, Target.get());
+    std::unique_ptr<Preprocessor> PP = std::make_unique<Preprocessor>(
+        std::make_shared<PreprocessorOptions>(), Diags, LangOpts, SourceMgr,
+        HeaderInfo, ModLoader,
+        /*IILookup =*/nullptr,
+        /*OwnsHeaderSearch =*/false);
+    PP->Initialize(*Target);
+    PP->EnterMainSourceFile();
+    return PP;
+  }
+
+  FileSystemOptions FileMgrOpts;
+  FileManager FileMgr;
+  IntrusiveRefCntPtr<DiagnosticIDs> DiagID;
+  ExpectedDiagConsumer *Consumer;
+  DiagnosticsEngine Diags;
+  SourceManager SourceMgr;
+  LangOptions LangOpts;
+  std::shared_ptr<TargetOptions> TargetOpts;
+  IntrusiveRefCntPtr<TargetInfo> Target;
+};
+
+// Valid Parser Tests
+
+TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
+  const llvm::StringLiteral Source = R"cc()cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test no diagnostics produced
+  Consumer->SetNoDiag();
+
+  ASSERT_FALSE(Parser.Parse());
+  ASSERT_EQ((int)Elements.size(), 0);
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable()
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test no diagnostics produced
+  Consumer->SetNoDiag();
+
+  ASSERT_FALSE(Parser.Parse());
+  RootElement Elem = Elements[0];
+
+  // Test generated DescriptorTable start has correct default values
+  ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
+  ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 0u);
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
+// Invalid Parser Tests
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedTokenTest) {
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable()
+    space
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced
+  Consumer->SetExpected(diag::err_hlsl_rootsig_unexpected_token_kind);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidParseInvalidTokenTest) {
+  const llvm::StringLiteral Source = R"cc(
+    notAnIdentifier
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced - invalid token
+  Consumer->SetExpected(diag::err_hlsl_rootsig_unexpected_token_kind);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
+TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedEndOfStreamTest) {
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable(
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced - end of stream
+  Consumer->SetExpected(diag::err_hlsl_rootsig_unexpected_token_kind);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
+} // anonymous namespace
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
new file mode 100644
index 0000000000000..9bdd41e6dec7b
--- /dev/null
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -0,0 +1,37 @@
+//===- HLSLRootSignature.h - HLSL Root Signature helper objects 
-----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file This file contains helper objects for working with HLSL Root
+/// Signatures.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_FRONTEND_HLSL_HLSLROOTSIGNATURE_H
+#define LLVM_FRONTEND_HLSL_HLSLROOTSIGNATURE_H
+
+#include <variant>
+
+namespace llvm {
+namespace hlsl {
+namespace rootsig {
+
+// Definitions of the in-memory data layout structures
+
+// Models the end of a descriptor table and stores its visibility
+struct DescriptorTable {
+  uint32_t NumClauses = 0; // The number of clauses in the table
+};
+
+// Models RootElement : DescriptorTable
+using RootElement = std::variant<DescriptorTable>;
+
+} // namespace rootsig
+} // namespace hlsl
+} // namespace llvm
+
+#endif // LLVM_FRONTEND_HLSL_HLSLROOTSIGNATURE_H

>From d444568345ff01da82e9170df3b37690c28a0ce9 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Thu, 13 Feb 2025 23:17:21 +0000
Subject: [PATCH 02/15] add empty Descriptor Table Clause

---
 .../clang/Parse/ParseHLSLRootSignature.h      | 13 ++++
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 69 ++++++++++++++++++-
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 25 +++++++
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    | 11 ++-
 4 files changed, 114 insertions(+), 4 deletions(-)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index fa46bdd663a2c..94acf5b0ecd94 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -43,10 +43,15 @@ class RootSignatureParser {
   // Root Element helpers
   bool ParseRootElement();
   bool ParseDescriptorTable();
+  bool ParseDescriptorTableClause();
 
   /// Invoke the Lexer to consume a token and update CurToken with the result
   void ConsumeNextToken() { CurToken = Lexer.ConsumeToken(); }
 
+  /// Return true if the next token one of the expected kinds
+  bool PeekExpectedToken(TokenKind Expected);
+  bool PeekExpectedToken(ArrayRef<TokenKind> AnyExpected);
+
   /// Consumes the next token and report an error if it is not of the expected
   /// kind.
   ///
@@ -54,6 +59,14 @@ class RootSignatureParser {
   bool ConsumeExpectedToken(TokenKind Expected);
   bool ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected);
 
+  /// Peek if the next token is of the expected kind and if it is then consume
+  /// it.
+  ///
+  /// Returns true if it successfully matches the expected kind and the token
+  /// was consumed.
+  bool TryConsumeExpectedToken(TokenKind Expected);
+  bool TryConsumeExpectedToken(ArrayRef<TokenKind> Expected);
+
 private:
   SmallVector<llvm::hlsl::rootsig::RootElement> &Elements;
   RootSignatureLexer &Lexer;
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index aeb33ded4d815..932a8a6263b05 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -100,12 +100,55 @@ bool RootSignatureParser::ParseDescriptorTable() {
     return true;
 
   // Empty case:
-  if (!ConsumeExpectedToken(TokenKind::pu_r_paren)) {
+  if (TryConsumeExpectedToken(TokenKind::pu_r_paren)) {
     Elements.push_back(Table);
     return false;
   }
 
-  return true;
+  // Iterate through all the defined clauses
+  do {
+    if (ParseDescriptorTableClause())
+      return true;
+    Table.NumClauses++;
+  } while (TryConsumeExpectedToken(TokenKind::pu_comma));
+
+  if (ConsumeExpectedToken(TokenKind::pu_r_paren))
+    return true;
+
+  Elements.push_back(Table);
+  return false;
+}
+
+bool RootSignatureParser::ParseDescriptorTableClause() {
+  if (ConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
+                            TokenKind::kw_UAV, TokenKind::kw_Sampler}))
+    return true;
+
+  DescriptorTableClause Clause;
+  switch (CurToken.Kind) {
+  case TokenKind::kw_CBV:
+    Clause.Type = ClauseType::CBuffer;
+    break;
+  case TokenKind::kw_SRV:
+    Clause.Type = ClauseType::SRV;
+    break;
+  case TokenKind::kw_UAV:
+    Clause.Type = ClauseType::UAV;
+    break;
+  case TokenKind::kw_Sampler:
+    Clause.Type = ClauseType::Sampler;
+    break;
+  default:
+    llvm_unreachable("Switch for an expected token was not provided");
+  }
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+    return true;
+
+  if (ConsumeExpectedToken(TokenKind::pu_r_paren))
+    return true;
+
+  Elements.push_back(Clause);
+  return false;
 }
 
 // Is given token one of the expected kinds
@@ -116,6 +159,15 @@ static bool IsExpectedToken(TokenKind Kind, 
ArrayRef<TokenKind> AnyExpected) {
   return false;
 }
 
+bool RootSignatureParser::PeekExpectedToken(TokenKind Expected) {
+  return PeekExpectedToken(ArrayRef{Expected});
+}
+
+bool RootSignatureParser::PeekExpectedToken(ArrayRef<TokenKind> AnyExpected) {
+  RootSignatureToken Result = Lexer.PeekNextToken();
+  return IsExpectedToken(Result.Kind, AnyExpected);
+}
+
 bool RootSignatureParser::ConsumeExpectedToken(TokenKind Expected) {
   return ConsumeExpectedToken(ArrayRef{Expected});
 }
@@ -134,5 +186,18 @@ bool RootSignatureParser::ConsumeExpectedToken(
   return true;
 }
 
+bool RootSignatureParser::TryConsumeExpectedToken(TokenKind Expected) {
+  return TryConsumeExpectedToken(ArrayRef{Expected});
+}
+
+bool RootSignatureParser::TryConsumeExpectedToken(
+    ArrayRef<TokenKind> AnyExpected) {
+  // If not the expected token just return
+  if (!PeekExpectedToken(AnyExpected))
+    return false;
+  ConsumeNextToken();
+  return true;
+}
+
 } // namespace hlsl
 } // namespace clang
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index f2ca933b4f39e..8e13467f3dfe7 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -128,6 +128,12 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
 
 TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
   const llvm::StringLiteral Source = R"cc(
+    DescriptorTable(
+      CBV(),
+      SRV(),
+      Sampler(),
+      UAV()
+    ),
     DescriptorTable()
   )cc";
 
@@ -144,7 +150,26 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
 
   ASSERT_FALSE(Parser.Parse());
   RootElement Elem = Elements[0];
+  ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer);
+
+  Elem = Elements[1];
+  ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV);
+
+  Elem = Elements[2];
+  ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler);
+
+  Elem = Elements[3];
+  ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV);
+
+  Elem = Elements[4];
+  ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
+  ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, (uint32_t)4);
 
+  Elem = Elements[5];
   // Test generated DescriptorTable start has correct default values
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
   ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 0u);
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 9bdd41e6dec7b..c1b67844c747f 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -14,6 +14,7 @@
 #ifndef LLVM_FRONTEND_HLSL_HLSLROOTSIGNATURE_H
 #define LLVM_FRONTEND_HLSL_HLSLROOTSIGNATURE_H
 
+#include "llvm/Support/DXILABI.h"
 #include <variant>
 
 namespace llvm {
@@ -27,8 +28,14 @@ struct DescriptorTable {
   uint32_t NumClauses = 0; // The number of clauses in the table
 };
 
-// Models RootElement : DescriptorTable
-using RootElement = std::variant<DescriptorTable>;
+// Models DTClause : CBV | SRV | UAV | Sampler, by collecting like parameters
+using ClauseType = llvm::dxil::ResourceClass;
+struct DescriptorTableClause {
+  ClauseType Type;
+};
+
+// Models RootElement : DescriptorTable | DescriptorTableClause
+using RootElement = std::variant<DescriptorTable, DescriptorTableClause>;
 
 } // namespace rootsig
 } // namespace hlsl

>From a7e36bfb21a6db435f8eade235818bf2496a6efd Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Fri, 14 Feb 2025 00:21:28 +0000
Subject: [PATCH 03/15] add Register and implicitly unsigned integers

---
 .../clang/Basic/DiagnosticParseKinds.td       |  1 +
 .../clang/Parse/ParseHLSLRootSignature.h      |  5 ++
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 57 +++++++++++++++++++
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 46 +++++++++++++--
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    |  8 +++
 5 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td 
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index 4c6adab80e60e..a5e19ddc75e02 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1820,4 +1820,5 @@ def err_hlsl_packoffset_invalid_reg : Error<"invalid 
resource class specifier '%
 
 // HLSL Root Signature Parser Diagnostics
 def err_hlsl_rootsig_unexpected_token_kind : Error<"got %1 but expected 
%select{|one of}0 the following token kind%select{|s}0: %2">;
+def err_hlsl_number_literal_overflow : Error<"integer literal is too large to 
be represented as a 32-bit %select{signed |}0 integer type">;
 } // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 94acf5b0ecd94..01d38261f58ee 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -45,6 +45,11 @@ class RootSignatureParser {
   bool ParseDescriptorTable();
   bool ParseDescriptorTableClause();
 
+  /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned
+  /// 32-bit integer
+  bool HandleUIntLiteral(uint32_t &X);
+  bool ParseRegister(llvm::hlsl::rootsig::Register *Reg);
+
   /// 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 932a8a6263b05..f470126f5ed51 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -1,5 +1,7 @@
 #include "clang/Parse/ParseHLSLRootSignature.h"
 
+#include "clang/Lex/LiteralSupport.h"
+
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm::hlsl::rootsig;
@@ -144,6 +146,10 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   if (ConsumeExpectedToken(TokenKind::pu_l_paren))
     return true;
 
+  // Consume mandatory Register paramater
+  if (ParseRegister(&Clause.Register))
+    return true;
+
   if (ConsumeExpectedToken(TokenKind::pu_r_paren))
     return true;
 
@@ -151,6 +157,57 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   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::ParseRegister(Register *Register) {
+  if (ConsumeExpectedToken(
+          {TokenKind::bReg, TokenKind::tReg, TokenKind::uReg, 
TokenKind::sReg}))
+    return true;
+
+  switch (CurToken.Kind) {
+  case TokenKind::bReg:
+    Register->ViewType = RegisterType::BReg;
+    break;
+  case TokenKind::tReg:
+    Register->ViewType = RegisterType::TReg;
+    break;
+  case TokenKind::uReg:
+    Register->ViewType = RegisterType::UReg;
+    break;
+  case TokenKind::sReg:
+    Register->ViewType = RegisterType::SReg;
+    break;
+  default:
+    llvm_unreachable("Switch for an expected token was not provided");
+  }
+
+  if (HandleUIntLiteral(Register->Number))
+    return true; // propogate NumericLiteralParser error
+
+  return false;
+}
+
 // Is given token one of the expected kinds
 static bool IsExpectedToken(TokenKind Kind, ArrayRef<TokenKind> AnyExpected) {
   for (auto Expected : AnyExpected)
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 8e13467f3dfe7..4beff9e6c9a42 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -129,10 +129,10 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
 TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
   const llvm::StringLiteral Source = R"cc(
     DescriptorTable(
-      CBV(),
-      SRV(),
-      Sampler(),
-      UAV()
+      CBV(b0),
+      SRV(t42),
+      Sampler(s987),
+      UAV(u987234)
     ),
     DescriptorTable()
   )cc";
@@ -152,18 +152,33 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
   RootElement Elem = Elements[0];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+            RegisterType::BReg);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 
(uint32_t)0);
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+            RegisterType::TReg);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
+            (uint32_t)42);
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+            RegisterType::SReg);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
+            (uint32_t)987);
 
   Elem = Elements[3];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
+            RegisterType::UReg);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
+            (uint32_t)987234);
 
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
@@ -239,4 +254,27 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidParseUnexpectedEndOfStreamTest) {
   ASSERT_TRUE(Consumer->IsSatisfied());
 }
 
+TEST_F(ParseHLSLRootSignatureTest, InvalidLexOverflowedNumberTest) {
+  // This test will check that the lexing fails due to an integer overflow
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable(
+      CBV(s4294967296)
+    )
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced
+  Consumer->SetExpected(diag::err_hlsl_number_literal_overflow);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
 } // anonymous namespace
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index c1b67844c747f..93f83fc824c6e 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -23,6 +23,13 @@ namespace rootsig {
 
 // Definitions of the in-memory data layout structures
 
+// Models the different registers: bReg | tReg | uReg | sReg
+enum class RegisterType { BReg, TReg, UReg, SReg };
+struct Register {
+  RegisterType ViewType;
+  uint32_t Number;
+};
+
 // Models the end of a descriptor table and stores its visibility
 struct DescriptorTable {
   uint32_t NumClauses = 0; // The number of clauses in the table
@@ -32,6 +39,7 @@ struct DescriptorTable {
 using ClauseType = llvm::dxil::ResourceClass;
 struct DescriptorTableClause {
   ClauseType Type;
+  Register Register;
 };
 
 // Models RootElement : DescriptorTable | DescriptorTableClause

>From e8f60466ec90bde7a4064b873a91a726855c6b59 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Tue, 28 Jan 2025 19:00:29 +0000
Subject: [PATCH 04/15] add support for optional parameters

- use `numDescriptors` as example
---
 .../clang/Basic/DiagnosticParseKinds.td       |  1 +
 .../clang/Parse/ParseHLSLRootSignature.h      | 14 ++++
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 65 +++++++++++++++++++
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 26 +++++++-
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    |  5 ++
 5 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td 
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index a5e19ddc75e02..ce0da28df3976 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1821,4 +1821,5 @@ def err_hlsl_packoffset_invalid_reg : Error<"invalid 
resource class specifier '%
 // HLSL Root Signature Parser Diagnostics
 def err_hlsl_rootsig_unexpected_token_kind : Error<"got %1 but expected 
%select{|one of}0 the following token kind%select{|s}0: %2">;
 def err_hlsl_number_literal_overflow : Error<"integer literal is too large to 
be represented as a 32-bit %select{signed |}0 integer type">;
+def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' 
multiple times">;
 } // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 01d38261f58ee..3587d0acbd739 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -45,10 +45,24 @@ class RootSignatureParser {
   bool ParseDescriptorTable();
   bool ParseDescriptorTableClause();
 
+  /// It is helpful to have a generalized dispatch method so that when we need
+  /// to parse multiple optional parameters in any order, we can invoke this
+  /// method.
+  ///
+  /// Each unique ParamType is expected to define a custom Parse method. This
+  /// function will switch on the ParamType using std::visit and dispatch onto
+  /// the corresponding Parse method
+  bool ParseParam(llvm::hlsl::rootsig::ParamType Ref);
+
+  /// Parses as many optional parameters as possible in any order
+  bool ParseOptionalParams(
+      llvm::SmallDenseMap<TokenKind, llvm::hlsl::rootsig::ParamType> &RefMap);
+
   /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned
   /// 32-bit integer
   bool HandleUIntLiteral(uint32_t &X);
   bool ParseRegister(llvm::hlsl::rootsig::Register *Reg);
+  bool ParseUInt(uint32_t *X);
 
   /// Invoke the Lexer to consume a token and update CurToken with the result
   void ConsumeNextToken() { CurToken = Lexer.ConsumeToken(); }
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index f470126f5ed51..161ad6b71529b 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -150,6 +150,13 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   if (ParseRegister(&Clause.Register))
     return true;
 
+  // Define optional paramaters
+  llvm::SmallDenseMap<TokenKind, ParamType> RefMap = {
+      {TokenKind::kw_numDescriptors, &Clause.NumDescriptors},
+  };
+  if (ParseOptionalParams({RefMap}))
+    return true;
+
   if (ConsumeExpectedToken(TokenKind::pu_r_paren))
     return true;
 
@@ -157,6 +164,52 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   return false;
 }
 
+// Helper struct so that we can use the overloaded notation of std::visit
+template <class... Ts> struct ParseMethods : Ts... {
+  using Ts::operator()...;
+};
+template <class... Ts> ParseMethods(Ts...) -> ParseMethods<Ts...>;
+
+bool RootSignatureParser::ParseParam(ParamType Ref) {
+  if (ConsumeExpectedToken(TokenKind::pu_equal))
+    return true;
+
+  bool Error;
+  std::visit(ParseMethods{
+    [&](uint32_t *X) { Error = ParseUInt(X); },
+  }, Ref);
+
+  return Error;
+}
+
+bool RootSignatureParser::ParseOptionalParams(
+    llvm::SmallDenseMap<TokenKind, ParamType> &RefMap) {
+  SmallVector<TokenKind> ParamKeywords;
+  for (auto RefPair : RefMap)
+    ParamKeywords.push_back(RefPair.first);
+
+  // Keep track of which keywords have been seen to report duplicates
+  llvm::SmallDenseSet<TokenKind> Seen;
+
+  while (TryConsumeExpectedToken(TokenKind::pu_comma)) {
+    if (ConsumeExpectedToken(ParamKeywords))
+      return true;
+
+    TokenKind ParamKind = CurToken.Kind;
+    if (Seen.contains(ParamKind)) {
+      Diags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+          << FormatTokenKinds({ParamKind});
+      return true;
+    }
+    Seen.insert(ParamKind);
+
+    if (ParseParam(RefMap[ParamKind]))
+      return true;
+  }
+
+  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,
@@ -208,6 +261,18 @@ bool RootSignatureParser::ParseRegister(Register 
*Register) {
   return false;
 }
 
+bool RootSignatureParser::ParseUInt(uint32_t *X) {
+  // Treat a postively signed integer as though it is unsigned to match DXC
+  TryConsumeExpectedToken(TokenKind::pu_plus);
+  if (ConsumeExpectedToken(TokenKind::int_literal))
+    return true;
+
+  if (HandleUIntLiteral(*X))
+    return true; // propogate NumericLiteralParser error
+
+  return false;
+}
+
 // Is given token one of the expected kinds
 static bool IsExpectedToken(TokenKind Kind, ArrayRef<TokenKind> AnyExpected) {
   for (auto Expected : AnyExpected)
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 4beff9e6c9a42..88182364ff708 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -130,7 +130,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   const llvm::StringLiteral Source = R"cc(
     DescriptorTable(
       CBV(b0),
-      SRV(t42),
+      SRV(t42, numDescriptors = +4),
       Sampler(s987),
       UAV(u987234)
     ),
@@ -155,6 +155,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
             RegisterType::BReg);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 
(uint32_t)0);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -163,6 +164,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             RegisterType::TReg);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
             (uint32_t)42);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)4);
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -171,6 +173,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             RegisterType::SReg);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
             (uint32_t)987);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
 
   Elem = Elements[3];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -179,6 +182,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             RegisterType::UReg);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
             (uint32_t)987234);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
 
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
@@ -277,4 +281,24 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidLexOverflowedNumberTest) {
   ASSERT_TRUE(Consumer->IsSatisfied());
 }
 
+TEST_F(ParseHLSLRootSignatureTest, InvalidParseRepeatedParamTest) {
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable(
+      CBV(b0, numDescriptors = 3, numDescriptors = 1)
+    )
+  )cc";
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced
+  Consumer->SetExpected(diag::err_hlsl_rootsig_repeat_param);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
 } // anonymous namespace
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 93f83fc824c6e..abc5fe28db32a 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -40,11 +40,16 @@ using ClauseType = llvm::dxil::ResourceClass;
 struct DescriptorTableClause {
   ClauseType Type;
   Register Register;
+  uint32_t NumDescriptors = 1;
 };
 
 // Models RootElement : DescriptorTable | DescriptorTableClause
 using RootElement = std::variant<DescriptorTable, DescriptorTableClause>;
 
+// Models a reference to all assignment parameter types that any RootElement
+// may have. Things of the form: Keyword = Param
+using ParamType = std::variant<uint32_t *>;
+
 } // namespace rootsig
 } // namespace hlsl
 } // namespace llvm

>From a8ddf47e7412179bc2ae8a8ef6f53c27b150fb11 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Tue, 28 Jan 2025 19:23:50 +0000
Subject: [PATCH 05/15] add space optional parameter

- demonstrate can specify in any order
---
 clang/lib/Parse/ParseHLSLRootSignature.cpp           | 1 +
 clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp | 8 ++++++--
 llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h  | 1 +
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 161ad6b71529b..23731439c6766 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -153,6 +153,7 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   // Define optional paramaters
   llvm::SmallDenseMap<TokenKind, ParamType> RefMap = {
       {TokenKind::kw_numDescriptors, &Clause.NumDescriptors},
+      {TokenKind::kw_space, &Clause.Space},
   };
   if (ParseOptionalParams({RefMap}))
     return true;
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 88182364ff708..5ddc068410a88 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -130,8 +130,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   const llvm::StringLiteral Source = R"cc(
     DescriptorTable(
       CBV(b0),
-      SRV(t42, numDescriptors = +4),
-      Sampler(s987),
+      SRV(t42, space = 3, numDescriptors = +4),
+      Sampler(s987, space = 2),
       UAV(u987234)
     ),
     DescriptorTable()
@@ -156,6 +156,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             RegisterType::BReg);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 
(uint32_t)0);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -165,6 +166,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
             (uint32_t)42);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)4);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)3);
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -174,6 +176,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
             (uint32_t)987);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)2);
 
   Elem = Elements[3];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -183,6 +186,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
             (uint32_t)987234);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
 
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index abc5fe28db32a..f3abbb5a917a3 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -41,6 +41,7 @@ struct DescriptorTableClause {
   ClauseType Type;
   Register Register;
   uint32_t NumDescriptors = 1;
+  uint32_t Space = 0;
 };
 
 // Models RootElement : DescriptorTable | DescriptorTableClause

>From 76817892625b9681d46ade8f767681c2284aaa81 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Tue, 28 Jan 2025 19:29:21 +0000
Subject: [PATCH 06/15] define custom paramtype for param parsing - descriptor
 range offset

---
 .../clang/Parse/ParseHLSLRootSignature.h      |  2 ++
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 20 +++++++++++++++++++
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 12 +++++++++--
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    |  9 ++++++++-
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 3587d0acbd739..7e0f190695cb6 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -63,6 +63,8 @@ class RootSignatureParser {
   bool HandleUIntLiteral(uint32_t &X);
   bool ParseRegister(llvm::hlsl::rootsig::Register *Reg);
   bool ParseUInt(uint32_t *X);
+  bool
+  ParseDescriptorRangeOffset(llvm::hlsl::rootsig::DescriptorRangeOffset *X);
 
   /// Invoke the Lexer to consume a token and update CurToken with the result
   void ConsumeNextToken() { CurToken = Lexer.ConsumeToken(); }
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 23731439c6766..b6d6efd8fd2d2 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -154,6 +154,7 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   llvm::SmallDenseMap<TokenKind, ParamType> RefMap = {
       {TokenKind::kw_numDescriptors, &Clause.NumDescriptors},
       {TokenKind::kw_space, &Clause.Space},
+      {TokenKind::kw_offset, &Clause.Offset},
   };
   if (ParseOptionalParams({RefMap}))
     return true;
@@ -178,6 +179,7 @@ bool RootSignatureParser::ParseParam(ParamType Ref) {
   bool Error;
   std::visit(ParseMethods{
     [&](uint32_t *X) { Error = ParseUInt(X); },
+    [&](DescriptorRangeOffset *X) { Error = ParseDescriptorRangeOffset(X); },
   }, Ref);
 
   return Error;
@@ -274,6 +276,24 @@ bool RootSignatureParser::ParseUInt(uint32_t *X) {
   return false;
 }
 
+bool RootSignatureParser::ParseDescriptorRangeOffset(DescriptorRangeOffset *X) 
{
+  if (ConsumeExpectedToken(
+          {TokenKind::int_literal, TokenKind::en_DescriptorRangeOffsetAppend}))
+    return true;
+
+  // Edge case for the offset enum -> static value
+  if (CurToken.Kind == TokenKind::en_DescriptorRangeOffsetAppend) {
+    *X = DescriptorTableOffsetAppend;
+    return false;
+  }
+
+  uint32_t Temp;
+  if (HandleUIntLiteral(Temp))
+    return true; // propogate NumericLiteralParser error
+  *X = DescriptorRangeOffset(Temp);
+  return false;
+}
+
 // Is given token one of the expected kinds
 static bool IsExpectedToken(TokenKind Kind, ArrayRef<TokenKind> AnyExpected) {
   for (auto Expected : AnyExpected)
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 5ddc068410a88..1198a37223b87 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -130,8 +130,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   const llvm::StringLiteral Source = R"cc(
     DescriptorTable(
       CBV(b0),
-      SRV(t42, space = 3, numDescriptors = +4),
-      Sampler(s987, space = 2),
+      SRV(t42, space = 3, offset = 32, numDescriptors = +4),
+      Sampler(s987, space = 2, offset = DESCRIPTOR_RANGE_OFFSET_APPEND),
       UAV(u987234)
     ),
     DescriptorTable()
@@ -157,6 +157,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 
(uint32_t)0);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
+            DescriptorRangeOffset(DescriptorTableOffsetAppend));
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -167,6 +169,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             (uint32_t)42);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)4);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)3);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
+            DescriptorRangeOffset(32));
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -177,6 +181,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             (uint32_t)987);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)2);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
+            DescriptorRangeOffset(DescriptorTableOffsetAppend));
 
   Elem = Elements[3];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -187,6 +193,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
             (uint32_t)987234);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
+            DescriptorRangeOffset(DescriptorTableOffsetAppend));
 
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index f3abbb5a917a3..1b23fed12ea14 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -21,6 +21,10 @@ namespace llvm {
 namespace hlsl {
 namespace rootsig {
 
+// Definition of the various enumerations and flags
+
+enum class DescriptorRangeOffset : uint32_t;
+
 // Definitions of the in-memory data layout structures
 
 // Models the different registers: bReg | tReg | uReg | sReg
@@ -35,6 +39,8 @@ struct DescriptorTable {
   uint32_t NumClauses = 0; // The number of clauses in the table
 };
 
+static const DescriptorRangeOffset DescriptorTableOffsetAppend =
+    DescriptorRangeOffset(0xffffffff);
 // Models DTClause : CBV | SRV | UAV | Sampler, by collecting like parameters
 using ClauseType = llvm::dxil::ResourceClass;
 struct DescriptorTableClause {
@@ -42,6 +48,7 @@ struct DescriptorTableClause {
   Register Register;
   uint32_t NumDescriptors = 1;
   uint32_t Space = 0;
+  DescriptorRangeOffset Offset = DescriptorTableOffsetAppend;
 };
 
 // Models RootElement : DescriptorTable | DescriptorTableClause
@@ -49,7 +56,7 @@ using RootElement = std::variant<DescriptorTable, 
DescriptorTableClause>;
 
 // Models a reference to all assignment parameter types that any RootElement
 // may have. Things of the form: Keyword = Param
-using ParamType = std::variant<uint32_t *>;
+using ParamType = std::variant<uint32_t *, DescriptorRangeOffset *>;
 
 } // namespace rootsig
 } // namespace hlsl

>From c3297af5f026c0d8be6e3c09f87dfb6780f3a25d Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Tue, 28 Jan 2025 19:38:57 +0000
Subject: [PATCH 07/15] add support for parsing enums

- uses Shader Visibility as an example for demonstrated test cases
---
 .../clang/Parse/ParseHLSLRootSignature.h      | 13 +++++
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 48 +++++++++++++++++++
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 28 +++++++++++
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    | 15 +++++-
 4 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 7e0f190695cb6..51048c077836c 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -66,6 +66,19 @@ class RootSignatureParser {
   bool
   ParseDescriptorRangeOffset(llvm::hlsl::rootsig::DescriptorRangeOffset *X);
 
+  /// Method for parsing any type of the ENUM defined token kinds (from
+  /// HLSLRootSignatureTokenKinds.def)
+  ///
+  /// EnumMap provides a mapping from the unique TokenKind to the in-memory
+  /// enum value
+  template <typename EnumType>
+  bool ParseEnum(llvm::SmallDenseMap<TokenKind, EnumType> &EnumMap,
+                 EnumType *Enum);
+
+  /// Helper methods that define the mappings and invoke ParseEnum for
+  /// different enum types
+  bool ParseShaderVisibility(llvm::hlsl::rootsig::ShaderVisibility *Enum);
+
   /// 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 b6d6efd8fd2d2..c5cdf5fa6ce92 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -107,8 +107,23 @@ bool RootSignatureParser::ParseDescriptorTable() {
     return false;
   }
 
+  bool SeenVisibility = false;
   // Iterate through all the defined clauses
   do {
+    // Handle the visibility parameter
+    if (TryConsumeExpectedToken(TokenKind::kw_visibility)) {
+      if (SeenVisibility) {
+        Diags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param)
+            << FormatTokenKinds(CurToken.Kind);
+        return true;
+      }
+      SeenVisibility = true;
+      if (ParseParam(&Table.Visibility))
+        return true;
+      continue;
+    }
+
+    // Otherwise, we expect a clause
     if (ParseDescriptorTableClause())
       return true;
     Table.NumClauses++;
@@ -180,6 +195,7 @@ bool RootSignatureParser::ParseParam(ParamType Ref) {
   std::visit(ParseMethods{
     [&](uint32_t *X) { Error = ParseUInt(X); },
     [&](DescriptorRangeOffset *X) { Error = ParseDescriptorRangeOffset(X); },
+    [&](ShaderVisibility *Enum) { Error = ParseShaderVisibility(Enum); },
   }, Ref);
 
   return Error;
@@ -294,6 +310,38 @@ bool 
RootSignatureParser::ParseDescriptorRangeOffset(DescriptorRangeOffset *X) {
   return false;
 }
 
+template <typename EnumType>
+bool RootSignatureParser::ParseEnum(
+    llvm::SmallDenseMap<TokenKind, EnumType> &EnumMap, EnumType *Enum) {
+  SmallVector<TokenKind> EnumToks;
+  for (auto EnumPair : EnumMap)
+    EnumToks.push_back(EnumPair.first);
+
+  // If invoked we expect to have an enum
+  if (ConsumeExpectedToken(EnumToks))
+    return true;
+
+  // Effectively a switch statement on the token kinds
+  for (auto EnumPair : EnumMap)
+    if (CurToken.Kind == EnumPair.first) {
+      *Enum = EnumPair.second;
+      return false;
+    }
+
+  llvm_unreachable("Switch for an expected token was not provided");
+}
+
+bool RootSignatureParser::ParseShaderVisibility(ShaderVisibility *Enum) {
+  // Define the possible flag kinds
+  llvm::SmallDenseMap<TokenKind, ShaderVisibility> EnumMap = {
+#define SHADER_VISIBILITY_ENUM(NAME, LIT)                                      
\
+  {TokenKind::en_##NAME, ShaderVisibility::NAME},
+#include "clang/Lex/HLSLRootSignatureTokenKinds.def"
+  };
+
+  return ParseEnum(EnumMap, Enum);
+}
+
 // Is given token one of the expected kinds
 static bool IsExpectedToken(TokenKind Kind, ArrayRef<TokenKind> AnyExpected) {
   for (auto Expected : AnyExpected)
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 1198a37223b87..a4b783a7a83fa 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -129,6 +129,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
 TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
   const llvm::StringLiteral Source = R"cc(
     DescriptorTable(
+      visibility = SHADER_VISIBILITY_PIXEL,
       CBV(b0),
       SRV(t42, space = 3, offset = 32, numDescriptors = +4),
       Sampler(s987, space = 2, offset = DESCRIPTOR_RANGE_OFFSET_APPEND),
@@ -199,11 +200,15 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
   ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, (uint32_t)4);
+  ASSERT_EQ(std::get<DescriptorTable>(Elem).Visibility,
+            ShaderVisibility::Pixel);
 
   Elem = Elements[5];
   // Test generated DescriptorTable start has correct default values
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
   ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 0u);
+  ASSERT_EQ(std::get<DescriptorTable>(Elem).Visibility, ShaderVisibility::All);
+
   ASSERT_TRUE(Consumer->IsSatisfied());
 }
 
@@ -313,4 +318,27 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidParseRepeatedParamTest) {
   ASSERT_TRUE(Consumer->IsSatisfied());
 }
 
+TEST_F(ParseHLSLRootSignatureTest, InvalidParseRepeatedVisibilityTest) {
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable(
+      visibility = SHADER_VISIBILITY_GEOMETRY,
+      visibility = SHADER_VISIBILITY_HULL
+    )
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced
+  Consumer->SetExpected(diag::err_hlsl_rootsig_repeat_param);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
 } // anonymous namespace
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index 1b23fed12ea14..a405c75f8c343 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -25,6 +25,17 @@ namespace rootsig {
 
 enum class DescriptorRangeOffset : uint32_t;
 
+enum class ShaderVisibility {
+  All = 0,
+  Vertex = 1,
+  Hull = 2,
+  Domain = 3,
+  Geometry = 4,
+  Pixel = 5,
+  Amplification = 6,
+  Mesh = 7,
+};
+
 // Definitions of the in-memory data layout structures
 
 // Models the different registers: bReg | tReg | uReg | sReg
@@ -36,6 +47,7 @@ struct Register {
 
 // Models the end of a descriptor table and stores its visibility
 struct DescriptorTable {
+  ShaderVisibility Visibility = ShaderVisibility::All;
   uint32_t NumClauses = 0; // The number of clauses in the table
 };
 
@@ -56,7 +68,8 @@ using RootElement = std::variant<DescriptorTable, 
DescriptorTableClause>;
 
 // Models a reference to all assignment parameter types that any RootElement
 // may have. Things of the form: Keyword = Param
-using ParamType = std::variant<uint32_t *, DescriptorRangeOffset *>;
+using ParamType = std::variant<uint32_t *, DescriptorRangeOffset *,
+                               ShaderVisibility *>;
 
 } // namespace rootsig
 } // namespace hlsl

>From 8a902fd5a6f4bb82b8e570ab22eb8cf273ce9723 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Tue, 28 Jan 2025 19:46:34 +0000
Subject: [PATCH 08/15] add support for parsing flags

- use descriptor range flags as an example
---
 .../clang/Basic/DiagnosticParseKinds.td       |  2 +
 .../clang/Parse/ParseHLSLRootSignature.h      | 16 +++++-
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 51 +++++++++++++++++-
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 38 ++++++++++++-
 .../llvm/Frontend/HLSL/HLSLRootSignature.h    | 53 ++++++++++++++++++-
 5 files changed, 155 insertions(+), 5 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td 
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index ce0da28df3976..fe1b0dd0b91cb 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1822,4 +1822,6 @@ def err_hlsl_packoffset_invalid_reg : Error<"invalid 
resource class specifier '%
 def err_hlsl_rootsig_unexpected_token_kind : Error<"got %1 but expected 
%select{|one of}0 the following token kind%select{|s}0: %2">;
 def err_hlsl_number_literal_overflow : Error<"integer literal is too large to 
be represented as a 32-bit %select{signed |}0 integer type">;
 def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' 
multiple times">;
+def err_hlsl_rootsig_non_zero_flag : Error<"specified a non-zero integer as a 
flag">;
+
 } // end of Parser diagnostics
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 51048c077836c..c4fb03a3bc3ed 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -71,7 +71,10 @@ class RootSignatureParser {
   ///
   /// EnumMap provides a mapping from the unique TokenKind to the in-memory
   /// enum value
-  template <typename EnumType>
+  ///
+  /// If AllowZero is true, then the Enum is used as a flag and can also have
+  /// the value of '0' to denote no flag
+  template <bool AllowZero = false, typename EnumType>
   bool ParseEnum(llvm::SmallDenseMap<TokenKind, EnumType> &EnumMap,
                  EnumType *Enum);
 
@@ -79,6 +82,17 @@ class RootSignatureParser {
   /// different enum types
   bool ParseShaderVisibility(llvm::hlsl::rootsig::ShaderVisibility *Enum);
 
+  /// A wrapper method around ParseEnum that will parse an 'or' chain of
+  /// enums, with AllowZero = true
+  template <typename FlagType>
+  bool ParseFlags(llvm::SmallDenseMap<TokenKind, FlagType> &EnumMap,
+                  FlagType *Enum);
+
+  /// Helper methods that define the mappings and invoke ParseFlags for
+  /// different enum types
+  bool
+  ParseDescriptorRangeFlags(llvm::hlsl::rootsig::DescriptorRangeFlags *Enum);
+
   /// 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 c5cdf5fa6ce92..e2564228de897 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -158,6 +158,8 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   default:
     llvm_unreachable("Switch for an expected token was not provided");
   }
+  Clause.SetDefaultFlags();
+
   if (ConsumeExpectedToken(TokenKind::pu_l_paren))
     return true;
 
@@ -170,6 +172,7 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
       {TokenKind::kw_numDescriptors, &Clause.NumDescriptors},
       {TokenKind::kw_space, &Clause.Space},
       {TokenKind::kw_offset, &Clause.Offset},
+      {TokenKind::kw_flags, &Clause.Flags},
   };
   if (ParseOptionalParams({RefMap}))
     return true;
@@ -196,6 +199,7 @@ bool RootSignatureParser::ParseParam(ParamType Ref) {
     [&](uint32_t *X) { Error = ParseUInt(X); },
     [&](DescriptorRangeOffset *X) { Error = ParseDescriptorRangeOffset(X); },
     [&](ShaderVisibility *Enum) { Error = ParseShaderVisibility(Enum); },
+    [&](DescriptorRangeFlags *Flags) { Error = 
ParseDescriptorRangeFlags(Flags); },
   }, Ref);
 
   return Error;
@@ -310,10 +314,12 @@ bool 
RootSignatureParser::ParseDescriptorRangeOffset(DescriptorRangeOffset *X) {
   return false;
 }
 
-template <typename EnumType>
+template <bool AllowZero, typename EnumType>
 bool RootSignatureParser::ParseEnum(
     llvm::SmallDenseMap<TokenKind, EnumType> &EnumMap, EnumType *Enum) {
   SmallVector<TokenKind> EnumToks;
+  if (AllowZero)
+    EnumToks.push_back(TokenKind::int_literal); //  '0' is a valid flag value
   for (auto EnumPair : EnumMap)
     EnumToks.push_back(EnumPair.first);
 
@@ -321,6 +327,20 @@ bool RootSignatureParser::ParseEnum(
   if (ConsumeExpectedToken(EnumToks))
     return true;
 
+  // Handle the edge case when '0' is used to specify None
+  if (CurToken.Kind == TokenKind::int_literal) {
+    uint32_t Temp;
+    if (HandleUIntLiteral(Temp))
+      return true; // propogate NumericLiteralParser error
+    if (Temp != 0) {
+      Diags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_non_zero_flag);
+      return true;
+    }
+    // Set enum to None equivalent
+    *Enum = EnumType(0);
+    return false;
+  }
+
   // Effectively a switch statement on the token kinds
   for (auto EnumPair : EnumMap)
     if (CurToken.Kind == EnumPair.first) {
@@ -342,6 +362,35 @@ bool 
RootSignatureParser::ParseShaderVisibility(ShaderVisibility *Enum) {
   return ParseEnum(EnumMap, Enum);
 }
 
+template <typename FlagType>
+bool RootSignatureParser::ParseFlags(
+    llvm::SmallDenseMap<TokenKind, FlagType> &FlagMap, FlagType *Flags) {
+  // Override the default value to 0 so that we can correctly 'or' the values
+  *Flags = FlagType(0);
+
+  do {
+    FlagType Flag;
+    if (ParseEnum<true>(FlagMap, &Flag))
+      return true;
+    // Store the 'or'
+    *Flags |= Flag;
+  } while (TryConsumeExpectedToken(TokenKind::pu_or));
+
+  return false;
+}
+
+bool RootSignatureParser::ParseDescriptorRangeFlags(
+    DescriptorRangeFlags *Flags) {
+  // Define the possible flag kinds
+  llvm::SmallDenseMap<TokenKind, DescriptorRangeFlags> FlagMap = {
+#define DESCRIPTOR_RANGE_FLAG_ENUM(NAME, LIT, ON)                              
\
+  {TokenKind::en_##NAME, DescriptorRangeFlags::NAME},
+#include "clang/Lex/HLSLRootSignatureTokenKinds.def"
+  };
+
+  return ParseFlags(FlagMap, Flags);
+}
+
 // Is given token one of the expected kinds
 static bool IsExpectedToken(TokenKind Kind, ArrayRef<TokenKind> AnyExpected) {
   for (auto Expected : AnyExpected)
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index a4b783a7a83fa..8fe57244142b0 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -131,9 +131,13 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
     DescriptorTable(
       visibility = SHADER_VISIBILITY_PIXEL,
       CBV(b0),
-      SRV(t42, space = 3, offset = 32, numDescriptors = +4),
+      SRV(t42, space = 3, offset = 32, numDescriptors = +4, flags = 0),
       Sampler(s987, space = 2, offset = DESCRIPTOR_RANGE_OFFSET_APPEND),
-      UAV(u987234)
+      UAV(u987234,
+        flags = Descriptors_Volatile | Data_Volatile
+                      | Data_Static_While_Set_At_Execute | Data_Static
+                      | Descriptors_Static_Keeping_Buffer_Bounds_Checks
+      )
     ),
     DescriptorTable()
   )cc";
@@ -160,6 +164,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(DescriptorTableOffsetAppend));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
+            DescriptorRangeFlags::DataStaticWhileSetAtExecute);
 
   Elem = Elements[1];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -172,6 +178,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)3);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(32));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
+            DescriptorRangeFlags::None);
 
   Elem = Elements[2];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -184,6 +192,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)2);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(DescriptorTableOffsetAppend));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
+            DescriptorRangeFlags::None);
 
   Elem = Elements[3];
   ASSERT_TRUE(std::holds_alternative<DescriptorTableClause>(Elem));
@@ -196,6 +206,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(DescriptorTableOffsetAppend));
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
+            DescriptorRangeFlags::ValidFlags);
 
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
@@ -341,4 +353,26 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidParseRepeatedVisibilityTest) {
   ASSERT_TRUE(Consumer->IsSatisfied());
 }
 
+TEST_F(ParseHLSLRootSignatureTest, InvalidParseNonZeroFlagTest) {
+  const llvm::StringLiteral Source = R"cc(
+    DescriptorTable(
+      CBV(b0, flags = 3)
+    )
+  )cc";
+
+  TrivialModuleLoader ModLoader;
+  auto PP = CreatePP(Source, ModLoader);
+  auto TokLoc = SourceLocation();
+
+  hlsl::RootSignatureLexer Lexer(Source, TokLoc);
+  SmallVector<RootElement> Elements;
+  hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
+
+  // Test correct diagnostic produced
+  Consumer->SetExpected(diag::err_hlsl_rootsig_non_zero_flag);
+  ASSERT_TRUE(Parser.Parse());
+
+  ASSERT_TRUE(Consumer->IsSatisfied());
+}
+
 } // anonymous namespace
diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h 
b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
index a405c75f8c343..f999a0b5eef33 100644
--- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
+++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
@@ -21,10 +21,43 @@ namespace llvm {
 namespace hlsl {
 namespace rootsig {
 
+#define RS_DEFINE_ENUM_CLASS_FLAGS_OPERATORS(Class)                            
\
+  inline Class operator|(Class a, Class b) {                                   
\
+    return static_cast<Class>(llvm::to_underlying(a) |                         
\
+                              llvm::to_underlying(b));                         
\
+  }                                                                            
\
+  inline Class operator&(Class a, Class b) {                                   
\
+    return static_cast<Class>(llvm::to_underlying(a) &                         
\
+                              llvm::to_underlying(b));                         
\
+  }                                                                            
\
+  inline Class operator~(Class a) {                                            
\
+    return static_cast<Class>(~llvm::to_underlying(a));                        
\
+  }                                                                            
\
+  inline Class &operator|=(Class &a, Class b) {                                
\
+    a = a | b;                                                                 
\
+    return a;                                                                  
\
+  }                                                                            
\
+  inline Class &operator&=(Class &a, Class b) {                                
\
+    a = a & b;                                                                 
\
+    return a;                                                                  
\
+  }
+
 // Definition of the various enumerations and flags
 
 enum class DescriptorRangeOffset : uint32_t;
 
+enum class DescriptorRangeFlags : unsigned {
+  None = 0,
+  DescriptorsVolatile = 0x1,
+  DataVolatile = 0x2,
+  DataStaticWhileSetAtExecute = 0x4,
+  DataStatic = 0x8,
+  DescriptorsStaticKeepingBufferBoundsChecks = 0x10000,
+  ValidFlags = 0x1000f,
+  ValidSamplerFlags = DescriptorsVolatile,
+};
+RS_DEFINE_ENUM_CLASS_FLAGS_OPERATORS(DescriptorRangeFlags)
+
 enum class ShaderVisibility {
   All = 0,
   Vertex = 1,
@@ -61,6 +94,24 @@ struct DescriptorTableClause {
   uint32_t NumDescriptors = 1;
   uint32_t Space = 0;
   DescriptorRangeOffset Offset = DescriptorTableOffsetAppend;
+  DescriptorRangeFlags Flags;
+
+  void SetDefaultFlags() {
+    switch (Type) {
+    case ClauseType::CBuffer:
+      Flags = DescriptorRangeFlags::DataStaticWhileSetAtExecute;
+      break;
+    case ClauseType::SRV:
+      Flags = DescriptorRangeFlags::DataStaticWhileSetAtExecute;
+      break;
+    case ClauseType::UAV:
+      Flags = DescriptorRangeFlags::DataVolatile;
+      break;
+    case ClauseType::Sampler:
+      Flags = DescriptorRangeFlags::None;
+      break;
+    }
+  }
 };
 
 // Models RootElement : DescriptorTable | DescriptorTableClause
@@ -69,7 +120,7 @@ using RootElement = std::variant<DescriptorTable, 
DescriptorTableClause>;
 // Models a reference to all assignment parameter types that any RootElement
 // may have. Things of the form: Keyword = Param
 using ParamType = std::variant<uint32_t *, DescriptorRangeOffset *,
-                               ShaderVisibility *>;
+                               DescriptorRangeFlags *, ShaderVisibility *>;
 
 } // namespace rootsig
 } // namespace hlsl

>From 2d763fe285d44c1165366d5104727e71d9da9f3f Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Fri, 14 Feb 2025 19:03:33 +0000
Subject: [PATCH 09/15] clang formatting

---
 .../clang/Parse/ParseHLSLRootSignature.h      |  2 +-
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 25 +++++++++++--------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index c4fb03a3bc3ed..8ba5ceaceac98 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -14,8 +14,8 @@
 #define LLVM_CLANG_PARSE_PARSEHLSLROOTSIGNATURE_H
 
 #include "clang/Basic/DiagnosticParse.h"
-#include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/LexHLSLRootSignature.h"
+#include "clang/Lex/Preprocessor.h"
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index e2564228de897..707a37dbad802 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -185,9 +185,7 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
 }
 
 // Helper struct so that we can use the overloaded notation of std::visit
-template <class... Ts> struct ParseMethods : Ts... {
-  using Ts::operator()...;
-};
+template <class... Ts> struct ParseMethods : Ts... { using Ts::operator()...; 
};
 template <class... Ts> ParseMethods(Ts...) -> ParseMethods<Ts...>;
 
 bool RootSignatureParser::ParseParam(ParamType Ref) {
@@ -195,12 +193,18 @@ bool RootSignatureParser::ParseParam(ParamType Ref) {
     return true;
 
   bool Error;
-  std::visit(ParseMethods{
-    [&](uint32_t *X) { Error = ParseUInt(X); },
-    [&](DescriptorRangeOffset *X) { Error = ParseDescriptorRangeOffset(X); },
-    [&](ShaderVisibility *Enum) { Error = ParseShaderVisibility(Enum); },
-    [&](DescriptorRangeFlags *Flags) { Error = 
ParseDescriptorRangeFlags(Flags); },
-  }, Ref);
+  std::visit(
+      ParseMethods{
+          [&](uint32_t *X) { Error = ParseUInt(X); },
+          [&](DescriptorRangeOffset *X) {
+            Error = ParseDescriptorRangeOffset(X);
+          },
+          [&](ShaderVisibility *Enum) { Error = ParseShaderVisibility(Enum); },
+          [&](DescriptorRangeFlags *Flags) {
+            Error = ParseDescriptorRangeFlags(Flags);
+          },
+      },
+      Ref);
 
   return Error;
 }
@@ -421,8 +425,7 @@ bool RootSignatureParser::ConsumeExpectedToken(
   // Report unexpected token kind error
   Diags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_unexpected_token_kind)
       << (unsigned)(AnyExpected.size() != 1)
-      << FormatTokenKinds({CurToken.Kind})
-      << FormatTokenKinds(AnyExpected);
+      << FormatTokenKinds({CurToken.Kind}) << FormatTokenKinds(AnyExpected);
   return true;
 }
 

>From 1d078242ac1f025f209d97268473c12a75d13ef7 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Fri, 14 Feb 2025 19:04:17 +0000
Subject: [PATCH 10/15] review: remove unreachable return true

---
 clang/lib/Parse/ParseHLSLRootSignature.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 707a37dbad802..e8e637c1b7977 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -89,10 +89,9 @@ bool RootSignatureParser::ParseRootElement() {
   switch (CurToken.Kind) {
   case TokenKind::kw_DescriptorTable:
     return ParseDescriptorTable();
-  default:
-    llvm_unreachable("Switch for an expected token was not provided");
+  default: break;
   }
-  return true;
+  llvm_unreachable("Switch for an expected token was not provided");
 }
 
 bool RootSignatureParser::ParseDescriptorTable() {

>From f78fdfab5d2ce89b24173c51f8ea56cd086eea85 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Fri, 14 Feb 2025 19:08:24 +0000
Subject: [PATCH 11/15] review: clean-up casting of test values

---
 clang/lib/Parse/ParseHLSLRootSignature.cpp    |  3 +-
 .../Parse/ParseHLSLRootSignatureTest.cpp      | 31 +++++++++----------
 2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index e8e637c1b7977..5bbb35875ceb5 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -89,7 +89,8 @@ bool RootSignatureParser::ParseRootElement() {
   switch (CurToken.Kind) {
   case TokenKind::kw_DescriptorTable:
     return ParseDescriptorTable();
-  default: break;
+  default:
+    break;
   }
   llvm_unreachable("Switch for an expected token was not provided");
 }
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 8fe57244142b0..224e16bfa372b 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -121,7 +121,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseEmptyTest) {
   Consumer->SetNoDiag();
 
   ASSERT_FALSE(Parser.Parse());
-  ASSERT_EQ((int)Elements.size(), 0);
+  ASSERT_EQ(Elements.size(), 0u);
 
   ASSERT_TRUE(Consumer->IsSatisfied());
 }
@@ -159,9 +159,9 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::CBuffer);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
             RegisterType::BReg);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 
(uint32_t)0);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 0u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, 1u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(DescriptorTableOffsetAppend));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
@@ -172,10 +172,9 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::SRV);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
             RegisterType::TReg);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
-            (uint32_t)42);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)4);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)3);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 42u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, 4u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 3u);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(32));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
@@ -186,10 +185,9 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::Sampler);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
             RegisterType::SReg);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
-            (uint32_t)987);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)2);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 987u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, 1u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 2u);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(DescriptorTableOffsetAppend));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
@@ -200,10 +198,9 @@ TEST_F(ParseHLSLRootSignatureTest, 
ValidParseDTClausesTest) {
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Type, ClauseType::UAV);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.ViewType,
             RegisterType::UReg);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number,
-            (uint32_t)987234);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, (uint32_t)1);
-  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, (uint32_t)0);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Register.Number, 987234u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).NumDescriptors, 1u);
+  ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u);
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Offset,
             DescriptorRangeOffset(DescriptorTableOffsetAppend));
   ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Flags,
@@ -211,7 +208,7 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) 
{
 
   Elem = Elements[4];
   ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem));
-  ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, (uint32_t)4);
+  ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 4u);
   ASSERT_EQ(std::get<DescriptorTable>(Elem).Visibility,
             ShaderVisibility::Pixel);
 

>From fda500ca8ebe693caa18b434d86ef07c225a90a9 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Fri, 14 Feb 2025 19:18:55 +0000
Subject: [PATCH 12/15] self-review: update doc

---
 clang/include/clang/Parse/ParseHLSLRootSignature.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 8ba5ceaceac98..89a02d88d6dfc 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -40,7 +40,17 @@ class RootSignatureParser {
   DiagnosticsEngine &Diags() { return PP.getDiagnostics(); }
 
 private:
-  // Root Element helpers
+  /// All private Parse.* methods follow a similar pattern:
+  ///   - Each method will expect that the the next token is of a certain kind
+  /// and will invoke `ConsumeExpectedToken`
+  ///   - As such, an error will be raised if the proceeding tokens are not
+  /// what is expected
+  ///   - Therefore, it is the callers responsibility to ensure that you are
+  /// expecting the next element type. Or equivalently, the methods should not
+  /// be called as a way to try and parse an element
+  ///   - All methods return true if a parsing error is encountered. It is the
+  /// callers responsibility to propogate this error up, or deal with it
+  /// otherwise
   bool ParseRootElement();
   bool ParseDescriptorTable();
   bool ParseDescriptorTableClause();

>From d99c599a0b144336243a3788d107bc6e057c7504 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Fri, 14 Feb 2025 19:44:36 +0000
Subject: [PATCH 13/15] self-review: move token spellings to definition

---
 .../clang/Lex/HLSLRootSignatureTokenKinds.def | 22 ++++++------
 .../include/clang/Lex/LexHLSLRootSignature.h  |  2 +-
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 35 ++-----------------
 .../Lex/LexHLSLRootSignatureTest.cpp          |  2 +-
 4 files changed, 16 insertions(+), 45 deletions(-)

diff --git a/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def 
b/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
index e6df763920430..697c6abc54efa 100644
--- a/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
+++ b/clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
@@ -14,16 +14,16 @@
 
//===----------------------------------------------------------------------===//
 
 #ifndef TOK
-#define TOK(X)
+#define TOK(X, SPELLING)
 #endif
 #ifndef PUNCTUATOR
-#define PUNCTUATOR(X,Y) TOK(pu_ ## X)
+#define PUNCTUATOR(X,Y) TOK(pu_ ## X, Y)
 #endif
 #ifndef KEYWORD
-#define KEYWORD(X) TOK(kw_ ## X)
+#define KEYWORD(X) TOK(kw_ ## X, #X)
 #endif
 #ifndef ENUM
-#define ENUM(NAME, LIT) TOK(en_ ## NAME)
+#define ENUM(NAME, LIT) TOK(en_ ## NAME, LIT)
 #endif
 
 // Defines the various types of enum
@@ -49,15 +49,15 @@
 #endif
 
 // General Tokens:
-TOK(invalid)
-TOK(end_of_stream)
-TOK(int_literal)
+TOK(invalid, "invalid identifier")
+TOK(end_of_stream, "end of stream")
+TOK(int_literal, "integer literal")
 
 // Register Tokens:
-TOK(bReg)
-TOK(tReg)
-TOK(uReg)
-TOK(sReg)
+TOK(bReg, "b register")
+TOK(tReg, "t register")
+TOK(uReg, "u register")
+TOK(sReg, "s register")
 
 // Punctuators:
 PUNCTUATOR(l_paren, '(')
diff --git a/clang/include/clang/Lex/LexHLSLRootSignature.h 
b/clang/include/clang/Lex/LexHLSLRootSignature.h
index 21c44e0351d9e..b82237411b2ab 100644
--- a/clang/include/clang/Lex/LexHLSLRootSignature.h
+++ b/clang/include/clang/Lex/LexHLSLRootSignature.h
@@ -24,7 +24,7 @@ namespace hlsl {
 
 struct RootSignatureToken {
   enum Kind {
-#define TOK(X) X,
+#define TOK(X, SPELLING) X,
 #include "clang/Lex/HLSLRootSignatureTokenKinds.def"
   };
 
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 5bbb35875ceb5..40d6921037f9e 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -17,38 +17,9 @@ static std::string FormatTokenKinds(ArrayRef<TokenKind> 
Kinds) {
     if (!First)
       Out << ", ";
     switch (Kind) {
-    case TokenKind::invalid:
-      Out << "invalid identifier";
-      break;
-    case TokenKind::end_of_stream:
-      Out << "end of stream";
-      break;
-    case TokenKind::int_literal:
-      Out << "integer literal";
-      break;
-    case TokenKind::bReg:
-      Out << "b register";
-      break;
-    case TokenKind::tReg:
-      Out << "t register";
-      break;
-    case TokenKind::uReg:
-      Out << "u register";
-      break;
-    case TokenKind::sReg:
-      Out << "s register";
-      break;
-#define PUNCTUATOR(X, Y)                                                       
\
-  case TokenKind::pu_##X:                                                      
\
-    Out << #Y;                                                                 
\
-    break;
-#define KEYWORD(NAME)                                                          
\
-  case TokenKind::kw_##NAME:                                                   
\
-    Out << #NAME;                                                              
\
-    break;
-#define ENUM(NAME, LIT)                                                        
\
-  case TokenKind::en_##NAME:                                                   
\
-    Out << LIT;                                                                
\
+#define TOK(X, SPELLING)                                                       
\
+  case TokenKind::X:                                                           
\
+    Out << SPELLING;                                                           
\
     break;
 #include "clang/Lex/HLSLRootSignatureTokenKinds.def"
     }
diff --git a/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp 
b/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
index 0576f08c4c276..1e014f6c41c57 100644
--- a/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
@@ -113,7 +113,7 @@ TEST_F(LexHLSLRootSignatureTest, ValidLexAllTokensTest) {
 
   SmallVector<hlsl::RootSignatureToken> Tokens;
   SmallVector<hlsl::TokenKind> Expected = {
-#define TOK(NAME) hlsl::TokenKind::NAME,
+#define TOK(NAME, SPELLING) hlsl::TokenKind::NAME,
 #include "clang/Lex/HLSLRootSignatureTokenKinds.def"
   };
 

>From 3146ebad1269b5580d5e2fd268c6dc5075aaff6c Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Wed, 5 Mar 2025 18:17:45 +0000
Subject: [PATCH 14/15] review: add liscence

---
 clang/lib/Parse/ParseHLSLRootSignature.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index 40d6921037f9e..b68bc935f3581 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -1,3 +1,11 @@
+//=== ParseHLSLRootSignature.cpp - Parse Root Signature 
-------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
 #include "clang/Parse/ParseHLSLRootSignature.h"
 
 #include "clang/Lex/LiteralSupport.h"

>From f457b38e8bc4bc342e12e81fb4ad1123b92b97f5 Mon Sep 17 00:00:00 2001
From: Finn Plummer <canadienf...@gmail.com>
Date: Wed, 5 Mar 2025 22:20:52 +0000
Subject: [PATCH 15/15] review prototype: allow for more context to
 ConsumeNextToken error

---
 .../clang/Basic/DiagnosticParseKinds.td       |  2 +-
 .../clang/Parse/ParseHLSLRootSignature.h      |  8 ++-
 clang/lib/Parse/ParseHLSLRootSignature.cpp    | 70 +++++++++++++------
 .../Parse/ParseHLSLRootSignatureTest.cpp      |  6 +-
 4 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td 
b/clang/include/clang/Basic/DiagnosticParseKinds.td
index fe1b0dd0b91cb..f2a5eaba48952 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -1819,7 +1819,7 @@ def err_hlsl_unsupported_component : Error<"invalid 
component '%0' used; expecte
 def err_hlsl_packoffset_invalid_reg : Error<"invalid resource class specifier 
'%0' for packoffset, expected 'c'">;
 
 // HLSL Root Signature Parser Diagnostics
-def err_hlsl_rootsig_unexpected_token_kind : Error<"got %1 but expected 
%select{|one of}0 the following token kind%select{|s}0: %2">;
+def err_hlsl_expected : Error<"expected %0 as %1">;
 def err_hlsl_number_literal_overflow : Error<"integer literal is too large to 
be represented as a 32-bit %select{signed |}0 integer type">;
 def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' 
multiple times">;
 def err_hlsl_rootsig_non_zero_flag : Error<"specified a non-zero integer as a 
flag">;
diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h 
b/clang/include/clang/Parse/ParseHLSLRootSignature.h
index 89a02d88d6dfc..9d141b0ebfd4f 100644
--- a/clang/include/clang/Parse/ParseHLSLRootSignature.h
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -114,8 +114,12 @@ class RootSignatureParser {
   /// kind.
   ///
   /// Returns true if there was an error reported.
-  bool ConsumeExpectedToken(TokenKind Expected);
-  bool ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected);
+  bool ConsumeExpectedToken(TokenKind Expected,
+                            unsigned DiagID = diag::err_expected,
+                            StringRef DiagMsg = "");
+  bool ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected,
+                            unsigned DiagID = diag::err_expected,
+                            StringRef DiagMsg = "");
 
   /// Peek if the next token is of the expected kind and if it is then consume
   /// it.
diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp 
b/clang/lib/Parse/ParseHLSLRootSignature.cpp
index b68bc935f3581..732d37d310750 100644
--- a/clang/lib/Parse/ParseHLSLRootSignature.cpp
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -53,7 +53,8 @@ bool RootSignatureParser::Parse() {
   while (!ParseRootElement()) {
     if (Lexer.EndOfBuffer())
       return false;
-    if (ConsumeExpectedToken(TokenKind::pu_comma))
+    if (ConsumeExpectedToken(TokenKind::pu_comma, diag::err_expected_either,
+                             "end of root signature string"))
       return true;
   }
 
@@ -61,7 +62,8 @@ bool RootSignatureParser::Parse() {
 }
 
 bool RootSignatureParser::ParseRootElement() {
-  if (ConsumeExpectedToken(TokenKind::kw_DescriptorTable))
+  if (ConsumeExpectedToken(TokenKind::kw_DescriptorTable,
+                           diag::err_hlsl_expected, "root element"))
     return true;
 
   // Dispatch onto the correct parse method
@@ -77,7 +79,8 @@ bool RootSignatureParser::ParseRootElement() {
 bool RootSignatureParser::ParseDescriptorTable() {
   DescriptorTable Table;
 
-  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
+                           "DescriptorTable"))
     return true;
 
   // Empty case:
@@ -108,7 +111,8 @@ bool RootSignatureParser::ParseDescriptorTable() {
     Table.NumClauses++;
   } while (TryConsumeExpectedToken(TokenKind::pu_comma));
 
-  if (ConsumeExpectedToken(TokenKind::pu_r_paren))
+  if (ConsumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
+                           "descriptor table clauses"))
     return true;
 
   Elements.push_back(Table);
@@ -117,7 +121,8 @@ bool RootSignatureParser::ParseDescriptorTable() {
 
 bool RootSignatureParser::ParseDescriptorTableClause() {
   if (ConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
-                            TokenKind::kw_UAV, TokenKind::kw_Sampler}))
+                            TokenKind::kw_UAV, TokenKind::kw_Sampler},
+                           diag::err_hlsl_expected, "descriptor table clause"))
     return true;
 
   DescriptorTableClause Clause;
@@ -139,7 +144,8 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   }
   Clause.SetDefaultFlags();
 
-  if (ConsumeExpectedToken(TokenKind::pu_l_paren))
+  if (ConsumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
+                           FormatTokenKinds({CurToken.Kind})))
     return true;
 
   // Consume mandatory Register paramater
@@ -156,7 +162,8 @@ bool RootSignatureParser::ParseDescriptorTableClause() {
   if (ParseOptionalParams({RefMap}))
     return true;
 
-  if (ConsumeExpectedToken(TokenKind::pu_r_paren))
+  if (ConsumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
+                           "clause parameters"))
     return true;
 
   Elements.push_back(Clause);
@@ -168,7 +175,8 @@ template <class... Ts> struct ParseMethods : Ts... { using 
Ts::operator()...; };
 template <class... Ts> ParseMethods(Ts...) -> ParseMethods<Ts...>;
 
 bool RootSignatureParser::ParseParam(ParamType Ref) {
-  if (ConsumeExpectedToken(TokenKind::pu_equal))
+  if (ConsumeExpectedToken(TokenKind::pu_equal, diag::err_expected_after,
+                           FormatTokenKinds(CurToken.Kind)))
     return true;
 
   bool Error;
@@ -198,7 +206,8 @@ bool RootSignatureParser::ParseOptionalParams(
   llvm::SmallDenseSet<TokenKind> Seen;
 
   while (TryConsumeExpectedToken(TokenKind::pu_comma)) {
-    if (ConsumeExpectedToken(ParamKeywords))
+    if (ConsumeExpectedToken(ParamKeywords, diag::err_hlsl_expected,
+                             "optional parameter"))
       return true;
 
     TokenKind ParamKind = CurToken.Kind;
@@ -241,7 +250,8 @@ bool RootSignatureParser::HandleUIntLiteral(uint32_t &X) {
 
 bool RootSignatureParser::ParseRegister(Register *Register) {
   if (ConsumeExpectedToken(
-          {TokenKind::bReg, TokenKind::tReg, TokenKind::uReg, 
TokenKind::sReg}))
+          {TokenKind::bReg, TokenKind::tReg, TokenKind::uReg, TokenKind::sReg},
+          diag::err_hlsl_expected, "a register"))
     return true;
 
   switch (CurToken.Kind) {
@@ -270,7 +280,8 @@ bool RootSignatureParser::ParseRegister(Register *Register) 
{
 bool RootSignatureParser::ParseUInt(uint32_t *X) {
   // Treat a postively signed integer as though it is unsigned to match DXC
   TryConsumeExpectedToken(TokenKind::pu_plus);
-  if (ConsumeExpectedToken(TokenKind::int_literal))
+  if (ConsumeExpectedToken(TokenKind::int_literal, diag::err_hlsl_expected,
+                           "unsigned integer"))
     return true;
 
   if (HandleUIntLiteral(*X))
@@ -281,7 +292,8 @@ bool RootSignatureParser::ParseUInt(uint32_t *X) {
 
 bool RootSignatureParser::ParseDescriptorRangeOffset(DescriptorRangeOffset *X) 
{
   if (ConsumeExpectedToken(
-          {TokenKind::int_literal, TokenKind::en_DescriptorRangeOffsetAppend}))
+          {TokenKind::int_literal, TokenKind::en_DescriptorRangeOffsetAppend},
+          diag::err_hlsl_expected, "descriptor range offset"))
     return true;
 
   // Edge case for the offset enum -> static value
@@ -307,7 +319,8 @@ bool RootSignatureParser::ParseEnum(
     EnumToks.push_back(EnumPair.first);
 
   // If invoked we expect to have an enum
-  if (ConsumeExpectedToken(EnumToks))
+  if (ConsumeExpectedToken(EnumToks, diag::err_hlsl_expected,
+                           "parameter value"))
     return true;
 
   // Handle the edge case when '0' is used to specify None
@@ -391,20 +404,33 @@ bool 
RootSignatureParser::PeekExpectedToken(ArrayRef<TokenKind> AnyExpected) {
   return IsExpectedToken(Result.Kind, AnyExpected);
 }
 
-bool RootSignatureParser::ConsumeExpectedToken(TokenKind Expected) {
-  return ConsumeExpectedToken(ArrayRef{Expected});
+bool RootSignatureParser::ConsumeExpectedToken(TokenKind Expected,
+                                               unsigned DiagID,
+                                               StringRef DiagMsg) {
+  return ConsumeExpectedToken(ArrayRef{Expected}, DiagID, DiagMsg);
 }
 
-bool RootSignatureParser::ConsumeExpectedToken(
-    ArrayRef<TokenKind> AnyExpected) {
-  ConsumeNextToken();
-  if (IsExpectedToken(CurToken.Kind, AnyExpected))
+bool RootSignatureParser::ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected,
+                                               unsigned DiagID,
+                                               StringRef DiagMsg) {
+  if (TryConsumeExpectedToken(AnyExpected))
     return false;
 
   // Report unexpected token kind error
-  Diags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_unexpected_token_kind)
-      << (unsigned)(AnyExpected.size() != 1)
-      << FormatTokenKinds({CurToken.Kind}) << FormatTokenKinds(AnyExpected);
+  DiagnosticBuilder DB = Diags().Report(CurToken.TokLoc, DiagID);
+  switch (DiagID) {
+  case diag::err_expected:
+    DB << FormatTokenKinds(AnyExpected);
+    break;
+  case diag::err_hlsl_expected:
+  case diag::err_expected_either:
+  case diag::err_expected_after:
+    DB << FormatTokenKinds(AnyExpected) << DiagMsg;
+    break;
+  default:
+    DB << DiagMsg;
+    break;
+  }
   return true;
 }
 
diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp 
b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
index 224e16bfa372b..35ad4bfff14d0 100644
--- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -238,7 +238,7 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidParseUnexpectedTokenTest) {
   hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
 
   // Test correct diagnostic produced
-  Consumer->SetExpected(diag::err_hlsl_rootsig_unexpected_token_kind);
+  Consumer->SetExpected(diag::err_expected_either);
   ASSERT_TRUE(Parser.Parse());
 
   ASSERT_TRUE(Consumer->IsSatisfied());
@@ -258,7 +258,7 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidParseInvalidTokenTest) {
   hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
 
   // Test correct diagnostic produced - invalid token
-  Consumer->SetExpected(diag::err_hlsl_rootsig_unexpected_token_kind);
+  Consumer->SetExpected(diag::err_hlsl_expected);
   ASSERT_TRUE(Parser.Parse());
 
   ASSERT_TRUE(Consumer->IsSatisfied());
@@ -278,7 +278,7 @@ TEST_F(ParseHLSLRootSignatureTest, 
InvalidParseUnexpectedEndOfStreamTest) {
   hlsl::RootSignatureParser Parser(Elements, Lexer, *PP);
 
   // Test correct diagnostic produced - end of stream
-  Consumer->SetExpected(diag::err_hlsl_rootsig_unexpected_token_kind);
+  Consumer->SetExpected(diag::err_hlsl_expected);
   ASSERT_TRUE(Parser.Parse());
 
   ASSERT_TRUE(Consumer->IsSatisfied());

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [HLSL][RootSignature] Implement Parsing of Descriptor Tables (PR #122982)

Reply via email to