Skip to content

[HLSL][RootSignature] Add parsing of Register in params for RootDescriptors #140148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 22, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions clang/include/clang/Parse/ParseHLSLRootSignature.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,12 @@ class RootSignatureParser {
};
std::optional<ParsedConstantParams> parseRootConstantParams();

struct ParsedRootDescriptorParams {
std::optional<llvm::hlsl::rootsig::Register> Reg;
};
std::optional<ParsedRootDescriptorParams>
parseRootDescriptorParams(RootSignatureToken::Kind RegType);

struct ParsedClauseParams {
std::optional<llvm::hlsl::rootsig::Register> Reg;
std::optional<uint32_t> NumDescriptors;
Expand Down
42 changes: 42 additions & 0 deletions clang/lib/Parse/ParseHLSLRootSignature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -176,20 +176,37 @@ std::optional<RootDescriptor> RootSignatureParser::parseRootDescriptor() {
return std::nullopt;

RootDescriptor Descriptor;
TokenKind ExpectedReg;
switch (DescriptorKind) {
default:
llvm_unreachable("Switch for consumed token was not provided");
case TokenKind::kw_CBV:
Descriptor.Type = DescriptorType::CBuffer;
ExpectedReg = TokenKind::bReg;
break;
case TokenKind::kw_SRV:
Descriptor.Type = DescriptorType::SRV;
ExpectedReg = TokenKind::tReg;
break;
case TokenKind::kw_UAV:
Descriptor.Type = DescriptorType::UAV;
ExpectedReg = TokenKind::uReg;
break;
}

auto Params = parseRootDescriptorParams(ExpectedReg);
if (!Params.has_value())
return std::nullopt;

// Check mandatory parameters were provided
if (!Params->Reg.has_value()) {
getDiags().Report(CurToken.TokLoc, diag::err_hlsl_rootsig_missing_param)
<< ExpectedReg;
return std::nullopt;
}

Descriptor.Reg = Params->Reg.value();

if (consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/TokenKind::kw_RootConstants))
Expand Down Expand Up @@ -398,6 +415,31 @@ RootSignatureParser::parseRootConstantParams() {
return Params;
}

std::optional<RootSignatureParser::ParsedRootDescriptorParams>
RootSignatureParser::parseRootDescriptorParams(TokenKind RegType) {
assert(CurToken.TokKind == TokenKind::pu_l_paren &&
"Expects to only be invoked starting at given token");

ParsedRootDescriptorParams Params;
do {
// ( `b` | `t` | `u`) POS_INT
if (tryConsumeExpectedToken(RegType)) {
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 &&
Expand Down
34 changes: 31 additions & 3 deletions clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,9 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootFlagsTest) {

TEST_F(ParseHLSLRootSignatureTest, ValidParseRootDescriptorsTest) {
const llvm::StringLiteral Source = R"cc(
CBV(),
SRV(),
UAV()
Comment on lines -349 to -351
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test somewhere that we error/reject empty for these now that the required parameters are wired up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are the InvalidMissingParameterTests that check this diagnostic being produced.
However, not on this direct code-path (it is tested for DescriptorTable CBV clauses with the same logic). I will add a quick test here

CBV(b0),
SRV(t42),
UAV(u34893247)
)cc";

TrivialModuleLoader ModLoader;
Expand All @@ -369,14 +369,20 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseRootDescriptorsTest) {
RootElement Elem = Elements[0];
ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem));
ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::CBuffer);
ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::BReg);
ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 0u);

Elem = Elements[1];
ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem));
ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::SRV);
ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::TReg);
ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 42u);

Elem = Elements[2];
ASSERT_TRUE(std::holds_alternative<RootDescriptor>(Elem));
ASSERT_EQ(std::get<RootDescriptor>(Elem).Type, DescriptorType::UAV);
ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.ViewType, RegisterType::UReg);
ASSERT_EQ(std::get<RootDescriptor>(Elem).Reg.Number, 34893247u);

ASSERT_TRUE(Consumer->isSatisfied());
}
Expand Down Expand Up @@ -494,6 +500,28 @@ TEST_F(ParseHLSLRootSignatureTest, InvalidMissingDTParameterTest) {
ASSERT_TRUE(Consumer->isSatisfied());
}

TEST_F(ParseHLSLRootSignatureTest, InvalidMissingRDParameterTest) {
// This test will check that the parsing fails due a mandatory
// parameter (register) not being specified
const llvm::StringLiteral Source = R"cc(
SRV()
)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, InvalidMissingRCParameterTest) {
// This test will check that the parsing fails due a mandatory
// parameter (num32BitConstants) not being specified
Expand Down
1 change: 1 addition & 0 deletions llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ using DescriptorType = llvm::dxil::ResourceClass;
// Models RootDescriptor : CBV | SRV | UAV, by collecting like parameters
struct RootDescriptor {
DescriptorType Type;
Register Reg;
};

// Models the end of a descriptor table and stores its visibility
Expand Down