Author: Finn Plummer Date: 2025-04-17T20:58:16-07:00 New Revision: c134e99a2adf6ac3f0d7a5f15a2f384f174c80a5
URL: https://github.com/llvm/llvm-project/commit/c134e99a2adf6ac3f0d7a5f15a2f384f174c80a5 DIFF: https://github.com/llvm/llvm-project/commit/c134e99a2adf6ac3f0d7a5f15a2f384f174c80a5.diff LOG: Revert "[HLSL][RootSignature] Implement initial parsing of the descriptor table clause params" (#136252) Reverts llvm/llvm-project#133800 Reverting to resolve the introduce naming collisions. Added: Modified: clang/include/clang/Basic/DiagnosticParseKinds.td clang/include/clang/Parse/ParseHLSLRootSignature.h clang/lib/Parse/ParseHLSLRootSignature.cpp clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td index 72e765bcb800d..9975520f4f9ff 100644 --- a/clang/include/clang/Basic/DiagnosticParseKinds.td +++ b/clang/include/clang/Basic/DiagnosticParseKinds.td @@ -1836,11 +1836,8 @@ def err_hlsl_virtual_function def err_hlsl_virtual_inheritance : Error<"virtual inheritance is unsupported in HLSL">; -// HLSL Root Signature Parser Diagnostics +// HLSL Root Siganture diagnostic messages def err_hlsl_unexpected_end_of_params : Error<"expected %0 to denote end of parameters, or, another valid parameter of %1">; -def err_hlsl_rootsig_repeat_param : Error<"specified the same parameter '%0' multiple times">; -def err_hlsl_rootsig_missing_param : Error<"did not specify mandatory parameter '%0'">; -def err_hlsl_number_literal_overflow : Error<"integer literal is too large to be represented as a 32-bit %select{signed |}0 integer type">; } // end of Parser diagnostics diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index e17d6584622ba..a8dd6b02501ae 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -40,31 +40,26 @@ class RootSignatureParser { private: DiagnosticsEngine &getDiags() { return PP.getDiagnostics(); } - // All private parse.* methods follow a similar pattern: + // 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(RootSignatureToken::Kind)) { - // auto ParsedObject = parse.*(); - // if (!ParsedObject.has_value()) - // return std::nullopt; - // ... - // } + // if (TryConsumeExpectedToken(RootSignatureToken::Kind)) + // if (Parse.*()) + // return true; // // or, // - // if (consumeExpectedToken(RootSignatureToken::Kind, ...)) - // return std::nullopt; - // auto ParsedObject = parse.*(); - // if (!ParsedObject.has_value()) - // return std::nullopt; - // ... + // if (ConsumeExpectedToken(RootSignatureToken::Kind, ...)) + // return true; + // if (Parse.*()) + // return true; // - // - All methods return std::nullopt if a parsing error is encountered. It - // is the callers responsibility to propogate this error up, or deal with it + // - 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 @@ -74,23 +69,6 @@ class RootSignatureParser { bool parseDescriptorTable(); bool parseDescriptorTableClause(); - /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any - /// order and only exactly once. `ParsedClauseParams` denotes the current - /// state of parsed params - struct ParsedClauseParams { - std::optional<llvm::hlsl::rootsig::Register> Register; - std::optional<uint32_t> Space; - }; - std::optional<ParsedClauseParams> - parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType); - - std::optional<uint32_t> parseUIntParam(); - std::optional<llvm::hlsl::rootsig::Register> parseRegister(); - - /// Use NumericLiteralParser to convert CurToken.NumSpelling into a unsigned - /// 32-bit integer - std::optional<uint32_t> handleUIntLiteral(); - /// 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 a13bee9750e2f..3513ef454f750 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -8,8 +8,6 @@ #include "clang/Parse/ParseHLSLRootSignature.h" -#include "clang/Lex/LiteralSupport.h" - #include "llvm/Support/raw_ostream.h" using namespace llvm::hlsl::rootsig; @@ -43,11 +41,12 @@ bool RootSignatureParser::parse() { break; } - if (consumeExpectedToken(TokenKind::end_of_stream, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/TokenKind::kw_RootSignature)) + if (!tryConsumeExpectedToken(TokenKind::end_of_stream)) { + getDiags().Report(CurToken.TokLoc, diag::err_hlsl_unexpected_end_of_params) + << /*expected=*/TokenKind::end_of_stream + << /*param of=*/TokenKind::kw_RootSignature; return true; - + } return false; } @@ -73,10 +72,12 @@ bool RootSignatureParser::parseDescriptorTable() { break; } - if (consumeExpectedToken(TokenKind::pu_r_paren, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/TokenKind::kw_DescriptorTable)) + if (!tryConsumeExpectedToken(TokenKind::pu_r_paren)) { + getDiags().Report(CurToken.TokLoc, diag::err_hlsl_unexpected_end_of_params) + << /*expected=*/TokenKind::pu_r_paren + << /*param of=*/TokenKind::kw_DescriptorTable; return true; + } Elements.push_back(Table); return false; @@ -89,170 +90,36 @@ bool RootSignatureParser::parseDescriptorTableClause() { CurToken.TokKind == TokenKind::kw_Sampler) && "Expects to only be invoked starting at given keyword"); - TokenKind ParamKind = CurToken.TokKind; - - if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after, - CurToken.TokKind)) - return true; - DescriptorTableClause Clause; - TokenKind ExpectedReg; - switch (ParamKind) { + switch (CurToken.TokKind) { default: llvm_unreachable("Switch for consumed token was not provided"); case TokenKind::kw_CBV: Clause.Type = ClauseType::CBuffer; - ExpectedReg = TokenKind::bReg; break; case TokenKind::kw_SRV: Clause.Type = ClauseType::SRV; - ExpectedReg = TokenKind::tReg; break; case TokenKind::kw_UAV: Clause.Type = ClauseType::UAV; - ExpectedReg = TokenKind::uReg; break; case TokenKind::kw_Sampler: Clause.Type = ClauseType::Sampler; - ExpectedReg = TokenKind::sReg; break; } - auto Params = parseDescriptorTableClauseParams(ExpectedReg); - if (!Params.has_value()) - return true; - - // Check mandatory parameters were provided - if (!Params->Register.has_value()) { - getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param) - << ExpectedReg; + if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after, + CurToken.TokKind)) return true; - } - - Clause.Register = Params->Register.value(); - - // Fill in optional values - if (Params->Space.has_value()) - Clause.Space = Params->Space.value(); - if (consumeExpectedToken(TokenKind::pu_r_paren, - diag::err_hlsl_unexpected_end_of_params, - /*param of=*/ParamKind)) + if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_expected_after, + CurToken.TokKind)) return true; Elements.push_back(Clause); return false; } -std::optional<RootSignatureParser::ParsedClauseParams> -RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) { - assert(CurToken.TokKind == TokenKind::pu_l_paren && - "Expects to only be invoked starting at given token"); - - // Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any - // order and only exactly once. Parse through as many arguments as possible - // reporting an error if a duplicate is seen. - ParsedClauseParams Params; - do { - // ( `b` | `t` | `u` | `s`) POS_INT - if (tryConsumeExpectedToken(RegType)) { - if (Params.Register.has_value()) { - getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param) - << CurToken.TokKind; - return std::nullopt; - } - auto Reg = parseRegister(); - if (!Reg.has_value()) - return std::nullopt; - Params.Register = Reg; - } - - // `space` `=` POS_INT - if (tryConsumeExpectedToken(TokenKind::kw_space)) { - if (Params.Space.has_value()) { - getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_repeat_param) - << CurToken.TokKind; - return std::nullopt; - } - - if (consumeExpectedToken(TokenKind::pu_equal)) - return std::nullopt; - - auto Space = parseUIntParam(); - if (!Space.has_value()) - return std::nullopt; - Params.Space = Space; - } - } while (tryConsumeExpectedToken(TokenKind::pu_comma)); - - return Params; -} - -std::optional<uint32_t> RootSignatureParser::parseUIntParam() { - assert(CurToken.TokKind == TokenKind::pu_equal && - "Expects to only be invoked starting at given keyword"); - tryConsumeExpectedToken(TokenKind::pu_plus); - if (consumeExpectedToken(TokenKind::int_literal, diag::err_expected_after, - CurToken.TokKind)) - return std::nullopt; - return handleUIntLiteral(); -} - -std::optional<Register> RootSignatureParser::parseRegister() { - assert((CurToken.TokKind == TokenKind::bReg || - CurToken.TokKind == TokenKind::tReg || - CurToken.TokKind == TokenKind::uReg || - CurToken.TokKind == TokenKind::sReg) && - "Expects to only be invoked starting at given keyword"); - - Register Register; - switch (CurToken.TokKind) { - default: - llvm_unreachable("Switch for consumed token was not provided"); - case TokenKind::bReg: - Register.ViewType = RegisterType::BReg; - break; - case TokenKind::tReg: - Register.ViewType = RegisterType::TReg; - break; - case TokenKind::uReg: - Register.ViewType = RegisterType::UReg; - break; - case TokenKind::sReg: - Register.ViewType = RegisterType::SReg; - break; - } - - auto Number = handleUIntLiteral(); - if (!Number.has_value()) - return std::nullopt; // propogate NumericLiteralParser error - - Register.Number = *Number; - return Register; -} - -std::optional<uint32_t> RootSignatureParser::handleUIntLiteral() { - // 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 std::nullopt; - } - - return Val.getExtValue(); -} - bool RootSignatureParser::peekExpectedToken(TokenKind Expected) { return peekExpectedToken(ArrayRef{Expected}); } @@ -274,7 +141,6 @@ bool RootSignatureParser::consumeExpectedToken(TokenKind Expected, case diag::err_expected: DB << Expected; break; - case diag::err_hlsl_unexpected_end_of_params: case diag::err_expected_either: case diag::err_expected_after: DB << Expected << Context; diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 56ee5d1183ace..19d5b267f310a 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(b0), - SRV(space = 3, t42), - Sampler(s987, space = +2), - UAV(u4294967294) + CBV(), + SRV(), + Sampler(), + UAV() ), DescriptorTable() )cc"; @@ -154,34 +154,18 @@ 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, 0u); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u); 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, 42u); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 3u); 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, 987u); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 2u); 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, 4294967294u); - ASSERT_EQ(std::get<DescriptorTableClause>(Elem).Space, 0u); Elem = Elements[4]; ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem)); @@ -191,32 +175,6 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { Elem = Elements[5]; ASSERT_TRUE(std::holds_alternative<DescriptorTable>(Elem)); ASSERT_EQ(std::get<DescriptorTable>(Elem).NumClauses, 0u); - - ASSERT_TRUE(Consumer->isSatisfied()); -} - -TEST_F(ParseHLSLRootSignatureTest, ValidTrailingCommaTest) { - // This test will checks we can handling trailing commas ',' - const llvm::StringLiteral Source = R"cc( - DescriptorTable( - CBV(b0, ), - SRV(t42), - ) - )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_TRUE(Consumer->isSatisfied()); } @@ -278,102 +236,6 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedEndOfStreamTest) { // Test correct diagnostic produced - end of stream Consumer->setExpected(diag::err_expected_after); - - ASSERT_TRUE(Parser.parse()); - - ASSERT_TRUE(Consumer->isSatisfied()); -} - -TEST_F(ParseHLSLRootSignatureTest, InvalidMissingParameterTest) { - // This test will check that the parsing fails due a mandatory - // parameter (register) not being specified - const llvm::StringLiteral Source = R"cc( - DescriptorTable( - CBV() - ) - )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_missing_param); - ASSERT_TRUE(Parser.parse()); - - ASSERT_TRUE(Consumer->isSatisfied()); -} - -TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryParameterTest) { - // This test will check that the parsing fails due the same mandatory - // parameter being specified multiple times - const llvm::StringLiteral Source = R"cc( - DescriptorTable( - CBV(b32, b84) - ) - )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()); -} - -TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedOptionalParameterTest) { - // This test will check that the parsing fails due the same optional - // parameter being specified multiple times - const llvm::StringLiteral Source = R"cc( - DescriptorTable( - CBV(space = 2, space = 0) - ) - )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()); -} - -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(b4294967296) - ) - )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()); diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h index 6a35c114100b9..c1b67844c747f 100644 --- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h +++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h @@ -23,13 +23,6 @@ namespace rootsig { // Definitions of the in-memory data layout structures -// Models the diff erent 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 @@ -39,8 +32,6 @@ struct DescriptorTable { using ClauseType = llvm::dxil::ResourceClass; struct DescriptorTableClause { ClauseType Type; - Register Register; - uint32_t Space = 0; }; // Models RootElement : DescriptorTable | DescriptorTableClause _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
