Skip to content

[HLSL][RootSignature] Add parsing infastructure for StaticSampler #140180

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

Open
wants to merge 2 commits into
base: users/inbelic/pr-140152
Choose a base branch
from
Open
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
1 change: 1 addition & 0 deletions clang/include/clang/Lex/HLSLRootSignatureTokenKinds.def
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ KEYWORD(RootSignature) // used only for diagnostic messaging
KEYWORD(RootFlags)
KEYWORD(DescriptorTable)
KEYWORD(RootConstants)
KEYWORD(StaticSampler)

// RootConstants Keywords:
KEYWORD(num32BitConstants)
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Parse/ParseHLSLRootSignature.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class RootSignatureParser {
std::optional<llvm::hlsl::rootsig::DescriptorTable> parseDescriptorTable();
std::optional<llvm::hlsl::rootsig::DescriptorTableClause>
parseDescriptorTableClause();
std::optional<llvm::hlsl::rootsig::StaticSampler> parseStaticSampler();

/// Parameter arguments (eg. `bReg`, `space`, ...) can be specified in any
/// order and only exactly once. The following methods define a
Expand Down Expand Up @@ -108,6 +109,11 @@ class RootSignatureParser {
std::optional<ParsedClauseParams>
parseDescriptorTableClauseParams(RootSignatureToken::Kind RegType);

struct ParsedStaticSamplerParams {
std::optional<llvm::hlsl::rootsig::Register> Reg;
};
std::optional<ParsedStaticSamplerParams> parseStaticSamplerParams();

// Common parsing methods
std::optional<uint32_t> parseUIntParam();
std::optional<llvm::hlsl::rootsig::Register> parseRegister();
Expand Down
62 changes: 62 additions & 0 deletions clang/lib/Parse/ParseHLSLRootSignature.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ bool RootSignatureParser::parse() {
return true;
Elements.push_back(*RootParam);
}

if (tryConsumeExpectedToken(TokenKind::kw_StaticSampler)) {
auto Sampler = parseStaticSampler();
if (!Sampler.has_value())
return true;
Elements.push_back(*Sampler);
}
} while (tryConsumeExpectedToken(TokenKind::pu_comma));

return consumeExpectedToken(TokenKind::end_of_stream,
Expand Down Expand Up @@ -348,6 +355,37 @@ RootSignatureParser::parseDescriptorTableClause() {
return Clause;
}

std::optional<StaticSampler> RootSignatureParser::parseStaticSampler() {
assert(CurToken.TokKind == TokenKind::kw_StaticSampler &&
"Expects to only be invoked starting at given keyword");

if (consumeExpectedToken(TokenKind::pu_l_paren, diag::err_expected_after,
CurToken.TokKind))
return std::nullopt;

StaticSampler Sampler;

auto Params = parseStaticSamplerParams();
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)
<< TokenKind::sReg;
return std::nullopt;
}

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

if (consumeExpectedToken(TokenKind::pu_r_paren,
diag::err_hlsl_unexpected_end_of_params,
/*param of=*/TokenKind::kw_StaticSampler))
return std::nullopt;

return Sampler;
}

// 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.
Expand Down Expand Up @@ -606,6 +644,30 @@ RootSignatureParser::parseDescriptorTableClauseParams(TokenKind RegType) {
return Params;
}

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

ParsedStaticSamplerParams Params;
do {
// `s` POS_INT
if (tryConsumeExpectedToken(TokenKind::sReg)) {
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())
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this case look like? Why is this not a parsing error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case would occur, for instance, when the register number would overflow an u32. Something like s4294967296.

It will have already reported the error (as part of handleUIntLiteral invoked by parseRegister) at this point and we are just propagating the error up.

return std::nullopt;
Params.Reg = Reg;
}
} while (tryConsumeExpectedToken(TokenKind::pu_comma));
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this parsing code allows stuff of the form 'StaticSampler(,,,,,,,,,,,,,,,,,)', and from looking at a DXC example it seems that is allowed https://godbolt.org/z/j7qP11h9W. It is amusing this is so permissive, and I wonder if there is a reason for that.


return Params;
}

std::optional<uint32_t> RootSignatureParser::parseUIntParam() {
assert(CurToken.TokKind == TokenKind::pu_equal &&
"Expects to only be invoked starting at given keyword");
Expand Down
2 changes: 1 addition & 1 deletion clang/unittests/Lex/LexHLSLRootSignatureTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ TEST_F(LexHLSLRootSignatureTest, ValidLexAllTokensTest) {

RootSignature

RootFlags DescriptorTable RootConstants
RootFlags DescriptorTable RootConstants StaticSampler

num32BitConstants

Expand Down
28 changes: 28 additions & 0 deletions clang/unittests/Parse/ParseHLSLRootSignatureTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,34 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) {
ASSERT_TRUE(Consumer->isSatisfied());
}

TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you add a test where the parsing is unsuccessful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted-out to reduce redundancy, as there is already testing for the errors that are encountered, albeit from a different code path.

const llvm::StringLiteral Source = R"cc(
StaticSampler(s0)
)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(Elements.size(), 1u);

RootElement Elem = Elements[0];
ASSERT_TRUE(std::holds_alternative<StaticSampler>(Elem));
ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.ViewType, RegisterType::SReg);
ASSERT_EQ(std::get<StaticSampler>(Elem).Reg.Number, 0u);

ASSERT_TRUE(Consumer->isSatisfied());
}

TEST_F(ParseHLSLRootSignatureTest, ValidSamplerFlagsTest) {
// This test will checks we can set the valid enum for Sampler descriptor
// range flags
Expand Down
16 changes: 11 additions & 5 deletions llvm/include/llvm/Frontend/HLSL/HLSLRootSignature.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,23 +158,29 @@ struct DescriptorTableClause {
void dump(raw_ostream &OS) const;
};

struct StaticSampler {
Register Reg;
};

/// Models RootElement : RootFlags | RootConstants | RootParam
/// | DescriptorTable | DescriptorTableClause
/// | DescriptorTable | DescriptorTableClause | StaticSampler
///
/// A Root Signature is modeled in-memory by an array of RootElements. These
/// aim to map closely to their DSL grammar reprsentation defined in the spec.
///
/// Each optional parameter has its default value defined in the struct, and,
/// each mandatory parameter does not have a default initialization.
///
/// For the variants RootFlags, RootConstants and DescriptorTableClause: each
/// data member maps directly to a parameter in the grammar.
/// For the variants RootFlags, RootConstants, RootParam, StaticSampler and
/// DescriptorTableClause: each data member maps directly to a parameter in the
/// grammar.
///
/// The DescriptorTable is modelled by having its Clauses as the previous
/// RootElements in the array, and it holds a data member for the Visibility
/// parameter.
using RootElement = std::variant<RootFlags, RootConstants, RootParam,
DescriptorTable, DescriptorTableClause>;
using RootElement =
std::variant<RootFlags, RootConstants, RootParam, DescriptorTable,
DescriptorTableClause, StaticSampler>;
void dumpRootElements(raw_ostream &OS, ArrayRef<RootElement> Elements);

class MetadataBuilder {
Expand Down