https://github.com/inbelic updated https://github.com/llvm/llvm-project/pull/138002
>From 61931241a2d66161b00ea5478d02f7fe3842ae8c Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Wed, 30 Apr 2025 17:53:11 +0000 Subject: [PATCH 1/4] [HLSL][RootSignature] Add mandatory parameters for RootConstants - defines the `parseRootConstantParams` function and adds handling for the mandatory arguments of `num32BitConstants` and `bReg` - adds corresponding unit tests Part two of implementing --- .../clang/Parse/ParseHLSLRootSignature.h | 10 ++- clang/lib/Parse/ParseHLSLRootSignature.cpp | 68 ++++++++++++++++++- .../Parse/ParseHLSLRootSignatureTest.cpp | 14 +++- .../llvm/Frontend/HLSL/HLSLRootSignature.h | 5 +- 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/clang/include/clang/Parse/ParseHLSLRootSignature.h b/clang/include/clang/Parse/ParseHLSLRootSignature.h index efa735ea03d94..0f05b05ed4df6 100644 --- a/clang/include/clang/Parse/ParseHLSLRootSignature.h +++ b/clang/include/clang/Parse/ParseHLSLRootSignature.h @@ -77,8 +77,14 @@ class RootSignatureParser { parseDescriptorTableClause(); /// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any - /// order and only exactly once. `ParsedClauseParams` denotes the current - /// state of parsed params + /// order and only exactly once. The following methods define a + /// `Parsed.*Params` struct to denote the current state of parsed params + struct ParsedConstantParams { + std::optional<llvm::hlsl::rootsig::Register> Reg; + std::optional<uint32_t> Num32BitConstants; + }; + std::optional<ParsedConstantParams> parseRootConstantParams(); + struct ParsedClauseParams { std::optional<llvm::hlsl::rootsig::Register> Reg; std::optional<uint32_t> NumDescriptors; diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 48d3e38b0519d..2ce8e6e5cca98 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -57,6 +57,27 @@ std::optional<RootConstants> RootSignatureParser::parseRootConstants() { RootConstants Constants; + auto Params = parseRootConstantParams(); + if (!Params.has_value()) + return std::nullopt; + + // Check mandatory parameters were provided + if (!Params->Num32BitConstants.has_value()) { + getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param) + << TokenKind::kw_num32BitConstants; + return std::nullopt; + } + + Constants.Num32BitConstants = Params->Num32BitConstants.value(); + + if (!Params->Reg.has_value()) { + getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param) + << TokenKind::bReg; + return std::nullopt; + } + + Constants.Reg = Params->Reg.value(); + if (consumeExpectedToken(TokenKind::pu_r_paren, diag::err_hlsl_unexpected_end_of_params, /*param of=*/TokenKind::kw_RootConstants)) @@ -187,14 +208,55 @@ RootSignatureParser::parseDescriptorTableClause() { return Clause; } +// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any +// order and only exactly once. The following methods will parse through as +// many arguments as possible reporting an error if a duplicate is seen. +std::optional<RootSignatureParser::ParsedConstantParams> +RootSignatureParser::parseRootConstantParams() { + assert(CurToken.TokKind == TokenKind::pu_l_paren && + "Expects to only be invoked starting at given token"); + + ParsedConstantParams Params; + do { + // `num32BitConstants` `=` POS_INT + if (tryConsumeExpectedToken(TokenKind::kw_num32BitConstants)) { + if (Params.Num32BitConstants.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 Num32BitConstants = parseUIntParam(); + if (!Num32BitConstants.has_value()) + return std::nullopt; + Params.Num32BitConstants = Num32BitConstants; + } + + // `b` POS_INT + if (tryConsumeExpectedToken(TokenKind::bReg)) { + if (Params.Reg.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.Reg = Reg; + } + } while (tryConsumeExpectedToken(TokenKind::pu_comma)); + + return Params; +} + 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 diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 0a7d8ac86cc5f..336868b579866 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -254,7 +254,8 @@ TEST_F(ParseHLSLRootSignatureTest, ValidSamplerFlagsTest) { TEST_F(ParseHLSLRootSignatureTest, ValidParseRootConsantsTest) { const llvm::StringLiteral Source = R"cc( - RootConstants() + RootConstants(num32BitConstants = 1, b0), + RootConstants(b42, num32BitConstants = 4294967295) )cc"; TrivialModuleLoader ModLoader; @@ -270,10 +271,19 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootConsantsTest) { ASSERT_FALSE(Parser.parse()); - ASSERT_EQ(Elements.size(), 1u); + ASSERT_EQ(Elements.size(), 2u); RootElement Elem = Elements[0]; ASSERT_TRUE(std::holds_alternative<RootConstants>(Elem)); + ASSERT_EQ(std::get<RootConstants>(Elem).Num32BitConstants, 1u); + ASSERT_EQ(std::get<RootConstants>(Elem).Reg.ViewType, RegisterType::BReg); + ASSERT_EQ(std::get<RootConstants>(Elem).Reg.Number, 0u); + + Elem = Elements[1]; + ASSERT_TRUE(std::holds_alternative<RootConstants>(Elem)); + ASSERT_EQ(std::get<RootConstants>(Elem).Num32BitConstants, 4294967295u); + ASSERT_EQ(std::get<RootConstants>(Elem).Reg.ViewType, RegisterType::BReg); + ASSERT_EQ(std::get<RootConstants>(Elem).Reg.Number, 42u); ASSERT_TRUE(Consumer->isSatisfied()); } diff --git a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h index 05735fa75b318..a3f98a9f1944f 100644 --- a/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h +++ b/llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h @@ -55,7 +55,10 @@ struct Register { }; // Models the parameter values of root constants -struct RootConstants {}; +struct RootConstants { + uint32_t Num32BitConstants; + Register Reg; +}; // Models the end of a descriptor table and stores its visibility struct DescriptorTable { >From 21d31c50d098ec186fef1d464b2ffeee8675bc1c Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Thu, 8 May 2025 15:32:08 +0000 Subject: [PATCH 2/4] review: add repeated param error testcase --- .../Parse/ParseHLSLRootSignatureTest.cpp | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 336868b579866..10c78eaaecefa 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -401,7 +401,7 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidMissingParameterTest) { ASSERT_TRUE(Consumer->isSatisfied()); } -TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryParameterTest) { +TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryDTParameterTest) { // This test will check that the parsing fails due the same mandatory // parameter being specified multiple times const llvm::StringLiteral Source = R"cc( @@ -425,6 +425,28 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryParameterTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidRepeatedMandatoryRCParameterTest) { + // This test will check that the parsing fails due the same mandatory + // parameter being specified multiple times + const llvm::StringLiteral Source = R"cc( + RootConstants(num32BitConstants = 32, num32BitConstants = 24) + )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 >From 2467c34e5506429bc38727f7aa60945b12ebc906 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Thu, 8 May 2025 15:34:22 +0000 Subject: [PATCH 3/4] review: add missing param error testcase --- .../Parse/ParseHLSLRootSignatureTest.cpp | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp index 10c78eaaecefa..63b5a65fd3c38 100644 --- a/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp +++ b/clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp @@ -377,7 +377,7 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidParseUnexpectedEndOfStreamTest) { ASSERT_TRUE(Consumer->isSatisfied()); } -TEST_F(ParseHLSLRootSignatureTest, InvalidMissingParameterTest) { +TEST_F(ParseHLSLRootSignatureTest, InvalidMissingDTParameterTest) { // This test will check that the parsing fails due a mandatory // parameter (register) not being specified const llvm::StringLiteral Source = R"cc( @@ -401,6 +401,28 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidMissingParameterTest) { ASSERT_TRUE(Consumer->isSatisfied()); } +TEST_F(ParseHLSLRootSignatureTest, InvalidMissingRCParameterTest) { + // This test will check that the parsing fails due a mandatory + // parameter (num32BitConstants) not being specified + const llvm::StringLiteral Source = R"cc( + RootConstants(b0) + )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, InvalidRepeatedMandatoryDTParameterTest) { // This test will check that the parsing fails due the same mandatory // parameter being specified multiple times >From ccb45bdf2c0ed0858aacbc6db09824b516960c95 Mon Sep 17 00:00:00 2001 From: Finn Plummer <[email protected]> Date: Fri, 9 May 2025 09:15:03 -0700 Subject: [PATCH 4/4] review: fix typo Co-authored-by: Ashley Coleman <[email protected]> --- clang/lib/Parse/ParseHLSLRootSignature.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/Parse/ParseHLSLRootSignature.cpp b/clang/lib/Parse/ParseHLSLRootSignature.cpp index 2ce8e6e5cca98..d392e9017ce85 100644 --- a/clang/lib/Parse/ParseHLSLRootSignature.cpp +++ b/clang/lib/Parse/ParseHLSLRootSignature.cpp @@ -61,7 +61,7 @@ std::optional<RootConstants> RootSignatureParser::parseRootConstants() { if (!Params.has_value()) return std::nullopt; - // Check mandatory parameters were provided + // Check mandatory parameters where provided if (!Params->Num32BitConstants.has_value()) { getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param) << TokenKind::kw_num32BitConstants; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
