[HLSL][RootSignature] Implement parsing of a DescriptorTable with empty clauses #133302
+620
−13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-hlsl @llvm/pr-subscribers-clang Author: Finn Plummer (inbelic) Changes
Patch is 22.98 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133302.diff 10 Files Affected:
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/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h
new file mode 100644
index 0000000000000..bd593fa3e8873
--- /dev/null
+++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h
@@ -0,0 +1,110 @@
+//===--- 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/LexHLSLRootSignature.h"
+#include "clang/Lex/Preprocessor.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();
+
+private:
+ DiagnosticsEngine &Diags() { return PP.getDiagnostics(); }
+
+ // All private Parse.* methods follow a similar pattern:
+ // - Each method will start with an assert to denote what the CurToken is
+ // expected to be and will parse from that token forward
+ //
+ // - Therefore, it is the callers responsibility to ensure that you are
+ // at the correct CurToken. This should be done with the pattern of:
+ //
+ // if (TryConsumeExpectedToken(TokenKind))
+ // if (Parse.*())
+ // return true;
+ //
+ // or,
+ //
+ // if (ConsumeExpectedToken(TokenKind, ...))
+ // return true;
+ // if (Parse.*())
+ // return true;
+ //
+ // - 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
+ //
+ // - An error will be raised if the proceeding tokens are not what is
+ // expected, or, there is a lexing error
+
+ /// Root Element parse methods:
+ 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.
+ ///
+ /// Returns true if there was an error reported.
+ bool ConsumeExpectedToken(TokenKind Expected,
+ unsigned DiagID = diag::err_expected,
+ TokenKind Context = TokenKind::invalid);
+ bool ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected,
+ unsigned DiagID = diag::err_expected,
+ TokenKind Context = TokenKind::invalid);
+
+ /// 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;
+
+ 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..2dd01167eaf83
--- /dev/null
+++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp
@@ -0,0 +1,188 @@
+#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) {
+#define TOK(X, SPELLING) \
+ case TokenKind::X: \
+ Out << SPELLING; \
+ 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() {
+ // Iterate as many RootElements as possible
+ while (TryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) {
+ bool Error = false;
+ // Dispatch onto parser method.
+ // We guard against the unreachable here as we just ensured that CurToken
+ // will be one of the kinds in the while condition
+ switch (CurToken.Kind) {
+ case TokenKind::kw_DescriptorTable:
+ Error = ParseDescriptorTable();
+ break;
+ default:
+ llvm_unreachable("Switch for consumed token was not provided");
+ }
+
+ if (Error)
+ return true;
+
+ if (!TryConsumeExpectedToken(TokenKind::pu_comma))
+ break;
+ }
+
+ return ConsumeExpectedToken(TokenKind::end_of_stream, diag::err_expected);
+}
+
+bool RootSignatureParser::ParseDescriptorTable() {
+ assert(CurToken.Kind == TokenKind::kw_DescriptorTable &&
+ "Expects to only be invoked starting at given keyword");
+
+ DescriptorTable Table;
+
+ if (ConsumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
+ CurToken.Kind))
+ return true;
+
+ // Iterate as many Clauses as possible
+ while (TryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV,
+ TokenKind::kw_UAV, TokenKind::kw_Sampler})) {
+ if (ParseDescriptorTableClause())
+ return true;
+
+ Table.NumClauses++;
+
+ if (!TryConsumeExpectedToken(TokenKind::pu_comma))
+ break;
+ }
+
+ if (ConsumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
+ CurToken.Kind))
+ return true;
+
+ Elements.push_back(Table);
+ return false;
+}
+
+bool RootSignatureParser::ParseDescriptorTableClause() {
+ assert((CurToken.Kind == TokenKind::kw_CBV ||
+ CurToken.Kind == TokenKind::kw_SRV ||
+ CurToken.Kind == TokenKind::kw_UAV ||
+ CurToken.Kind == TokenKind::kw_Sampler) &&
+ "Expects to only be invoked starting at given keyword");
+
+ DescriptorTableClause Clause;
+ switch (CurToken.Kind) {
+ default:
+ break; // Unreachable given Try + assert pattern
+ 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;
+ }
+
+ if (ConsumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
+ CurToken.Kind))
+ return true;
+
+ if (ConsumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after,
+ CurToken.Kind))
+ return true;
+
+ Elements.push_back(Clause);
+ return false;
+}
+
+// Returns true when given token is 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::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,
+ unsigned DiagID,
+ TokenKind Context) {
+ return ConsumeExpectedToken(ArrayRef{Expected}, DiagID, Context);
+}
+
+bool RootSignatureParser::ConsumeExpectedToken(ArrayRef<TokenKind> AnyExpected,
+ unsigned DiagID,
+ TokenKind Context) {
+ if (TryConsumeExpectedToken(AnyExpected))
+ return false;
+
+ // Report unexpected token kind error
+ DiagnosticBuilder DB = Diags().Report(CurToken.TokLoc, DiagID);
+ switch (DiagID) {
+ case diag::err_expected:
+ DB << FormatTokenKinds(AnyExpected);
+ break;
+ case diag::err_expected_either:
+ case diag::err_expected_after:
+ DB << FormatTokenKinds(AnyExpected) << FormatTokenKinds({Context});
+ break;
+ default:
+ break;
+ }
+ 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/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/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"
};
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..d5c3d9778987f
--- /dev/null
+++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
@@ -0,0 +1,244 @@
+//=== 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(
+ CBV(),
+ SRV(),
+ Sampler(),
+ UAV()
+ ),
+ 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());
+
+ // First Descriptor Table with 4 elements
+ 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);
+
+ // Empty Descriptor Table
+ Elem = Elements[5];
+ 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 produc...
[truncated]
|
- 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
inbelic
force-pushed
the
inbelic/rs-parse-empty
branch
from
13e5c91 to
d87707f
Compare
inbelic
commented
Mar 27, 2025
| return TokenString; | ||
| } | ||
|
|
||
| // Parser Definitions |
joaosaffran
reviewed
Mar 27, 2025
|
|
||
| bool RootSignatureParser::Parse() { | ||
| // Iterate as many RootElements as possible | ||
| while (TryConsumeExpectedToken(TokenKind::kw_DescriptorTable)) { |
There was a problem hiding this comment.
nit: Something that makes me confuse with that approach is, how will that look once you add more RootElements? Will that become something like:
while (
TryConsumeExpectedToken(TokenKind::kw_DescriptorTable) ||
TryConsumeExpectedToken(TokenKind::kw_RootDescriptor) ||
TryConsumeExpectedToken(TokenKind::kw_RootConstant)) {
...
}My expectation for that code would be something like:
do{
// Parsing logic here
} while(TryConsumeExpectedToken(TokenKind::pu_comma));There was a problem hiding this comment.
Correct, it will look like how you mentioned in the other comment:
while (TryConsumeExpectedToken({TokenKind::kw_DescriptorTable, TokenKind::kw_RootConstant, ... }) {
...
}
| return true; | ||
|
|
||
| // Iterate as many Clauses as possible | ||
| while (TryConsumeExpectedToken({TokenKind::kw_CBV, TokenKind::kw_SRV, |
There was a problem hiding this comment.
nit: I imagine the above loop would be something like this, once you add more Root Elements?
| }; | ||
|
|
||
| // Models RootElement : DescriptorTable | DescriptorTableClause | ||
| using RootElement = std::variant<DescriptorTable, DescriptorTableClause>; |
There was a problem hiding this comment.
I feel like the DescriptorTableClause should be a member of DescriptorTable and not a RootElement, since those are connected in the metadata representation, it should be easier that way, IMHO.
There was a problem hiding this comment.
I agree that would be intuitive, but I would argue that each of these clauses will have their own metadata node that this representation maps well to.
There is also an implementation hurdle to have these as a vector on DescriptorTable which is that we need to explicitly allocate all the memory onto the AST. Practically this means that after this function returns in SemaHLSL we would need to go through and reallocate all these vectors as ArrayRef.
Or maybe we can pass down an allocator? I am not sure, either way I think it would introduce complexity rather than remove it
inbelic
commented
Mar 27, 2025
bogner
reviewed
Mar 27, 2025
| namespace clang { | ||
| namespace hlsl { | ||
|
|
||
| static std::string FormatTokenKinds(ArrayRef<TokenKind> Kinds) { |
There was a problem hiding this comment.
You can simplify this while also making it more efficient by implementing a TokenFormatter class with an operator<< for DiagnosticBuilder. The class should be okay to hold an ArrayRef as a member since it will always be used as a temporary, like DB << TokenFormatter(Tokens);
There was a problem hiding this comment.
Updated to define the const operator<< for TokenKind and removed the api to use consumeExpectedToken(ArrayRef)
added 7 commits
March 28, 2025 20:24
- using `consumeExpectedToken` with multiple expected tokens leads to poor handling of diagnostic messaging and would largely be used in cases where more specific errors could be done - instead we should limit the api to just using `consumeExpectedToken` with only a specific token and to only be used when it is very clear that only one specific token should follow after another
bogner
approved these changes
Mar 31, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/76/builds/8327 Here is the relevant piece of the build log for the reference |
inbelic
added a commit
to inbelic/llvm-project
that referenced
this pull request
Mar 31, 2025
…ty clauses (llvm#133302) - 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 Implements the first part of llvm#126569
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/6149 Here is the relevant piece of the build log for the reference |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 31, 2025
…iptorTable with empty clauses" (#133790) Reverts llvm/llvm-project#133302 Reverting to inspect build failures that were introduced from use of the `clang::Preprocessor` in unit testing, as well as, the warning about an unused declaration. See linked issue for failures.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2619 Here is the relevant piece of the build log for the reference |
inbelic
added a commit
to inbelic/llvm-project
that referenced
this pull request
Apr 1, 2025
…ty clauses (llvm#133302) - 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 Implements the first part of llvm#126569
inbelic
added a commit
that referenced
this pull request
Apr 1, 2025
…with empty clauses" (#133958) This pr relands #133302. It resolves two issues: - Linking error during build, [here](#133302 (comment)). There was a missing dependency for `clangLex` for the `ParseHLSLRootSignatureTest.cpp` unit testing. This library was added to the dependencies to resolve the error. It wasn't caught previously as the library was transitively linked in most build environments - Warning of unused declaration, [here](#133302 (comment)). There was a usability line in `LexHLSLRootSignature.h` of the form `using TokenKind = enum RootSignatureToken::Kind` which causes this error. The declaration is removed from the header file to be used locally in the `.cpp` files that use it. Notably, the original pr would also exposed `clang::hlsl::TokenKind` to everywhere it was included, which had a name clash with `tok::TokenKind`. This is another motivation to change to the proposed resolution. --------- Co-authored-by: Finn Plummer <finnplummer@microsoft.com>
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 1, 2025
…iptorTable with empty clauses" (#133958) This pr relands llvm/llvm-project#133302. It resolves two issues: - Linking error during build, [here](llvm/llvm-project#133302 (comment)). There was a missing dependency for `clangLex` for the `ParseHLSLRootSignatureTest.cpp` unit testing. This library was added to the dependencies to resolve the error. It wasn't caught previously as the library was transitively linked in most build environments - Warning of unused declaration, [here](llvm/llvm-project#133302 (comment)). There was a usability line in `LexHLSLRootSignature.h` of the form `using TokenKind = enum RootSignatureToken::Kind` which causes this error. The declaration is removed from the header file to be used locally in the `.cpp` files that use it. Notably, the original pr would also exposed `clang::hlsl::TokenKind` to everywhere it was included, which had a name clash with `tok::TokenKind`. This is another motivation to change to the proposed resolution. --------- Co-authored-by: Finn Plummer <finnplummer@microsoft.com>
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
Apr 2, 2025
…with empty clauses" (llvm#133958) This pr relands llvm#133302. It resolves two issues: - Linking error during build, [here](llvm#133302 (comment)). There was a missing dependency for `clangLex` for the `ParseHLSLRootSignatureTest.cpp` unit testing. This library was added to the dependencies to resolve the error. It wasn't caught previously as the library was transitively linked in most build environments - Warning of unused declaration, [here](llvm#133302 (comment)). There was a usability line in `LexHLSLRootSignature.h` of the form `using TokenKind = enum RootSignatureToken::Kind` which causes this error. The declaration is removed from the header file to be used locally in the `.cpp` files that use it. Notably, the original pr would also exposed `clang::hlsl::TokenKind` to everywhere it was included, which had a name clash with `tok::TokenKind`. This is another motivation to change to the proposed resolution. --------- Co-authored-by: Finn Plummer <finnplummer@microsoft.com>
inbelic
added a commit
that referenced
this pull request
Apr 4, 2025
#134136) - when developing the RootSignatureLexer library, we are creating new files so we should set the standard to adhere to the coding conventions for function naming - this was missed in the initial review but caught in the review of the parser pr [here](#133302 (comment)) Co-authored-by: Finn Plummer <finnplummer@microsoft.com>
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Apr 4, 2025
… conventions (#134136) - when developing the RootSignatureLexer library, we are creating new files so we should set the standard to adhere to the coding conventions for function naming - this was missed in the initial review but caught in the review of the parser pr [here](llvm/llvm-project#133302 (comment)) Co-authored-by: Finn Plummer <finnplummer@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the first part of #126569