-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[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
base: users/inbelic/pr-140152
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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. | ||
|
@@ -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()) | ||
return std::nullopt; | ||
Params.Reg = Reg; | ||
} | ||
} while (tryConsumeExpectedToken(TokenKind::pu_comma)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -223,6 +223,34 @@ TEST_F(ParseHLSLRootSignatureTest, ValidParseDTClausesTest) { | |
ASSERT_TRUE(Consumer->isSatisfied()); | ||
} | ||
|
||
TEST_F(ParseHLSLRootSignatureTest, ValidParseStaticSamplerTest) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you add a test where the parsing is unsuccessful? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 byparseRegister
) at this point and we are just propagating the error up.