[HLSL] Adding support for Root Constants in LLVM Metadata #135085
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
joaosaffran
commented
Apr 9, 2025
joaosaffran
commented
- Closes #126637
added 30 commits
February 19, 2025 21:38
joaosaffran
force-pushed
the
metadata/root-constants
branch
from
02c2d6f to
7aed7d1
Compare
inbelic
reviewed
Apr 17, 2025
| @@ -25,7 +25,7 @@ struct RootSignatureDesc { | |||
|
|
|||
| uint32_t Version = 2U; | |||
| uint32_t Flags = 0U; | |||
| uint32_t RootParameterOffset = 0U; | |||
| uint32_t RootParameterOffset = 24U; | |||
There was a problem hiding this comment.
Can we add a comment as to why it should be 24
There was a problem hiding this comment.
Clang emits the root signature data in dxcontainer following a specific sequence, first the header, then the root parameters, the header is always 24 bytes long, so this is why we have 24 here.
There was a problem hiding this comment.
Sounds good thanks. Can we add this to the code as well so it doesn't get lost
There was a problem hiding this comment.
Given that the root parameter offset doesn't actually have to be 24 it feels awkward to set it like it's a constant here. I think it's more logical to leave this as zero and fill it in when we create these from the metadata, much like we'll have to do for StaticSamplersOffset
llvm/test/CodeGen/DirectX/ContainerData/RootSignature-Parameters-Validation-Error.ll
Show resolved
Hide resolved
bogner
reviewed
Apr 24, 2025
| @@ -25,7 +25,7 @@ struct RootSignatureDesc { | |||
|
|
|||
| uint32_t Version = 2U; | |||
| uint32_t Flags = 0U; | |||
| uint32_t RootParameterOffset = 0U; | |||
| uint32_t RootParameterOffset = 24U; | |||
There was a problem hiding this comment.
Given that the root parameter offset doesn't actually have to be 24 it feels awkward to set it like it's a constant here. I think it's more logical to leave this as zero and fill it in when we create these from the metadata, much like we'll have to do for StaticSamplersOffset
Comment on lines
43
to
44
| static bool reportValueError(LLVMContext *Ctx, Twine ParamName, uint32_t Value, | ||
| DiagnosticSeverity Severity = DS_Error) { |
There was a problem hiding this comment.
Given that the function is called reportValueError it seems weird for there to be a Severity parameter. We should either drop that parameter or rename this.
Comment on lines
50
to
58
| static bool extractMdIntValue(uint32_t &Value, MDNode *Node, | ||
| unsigned int OpId) { | ||
| auto *CI = mdconst::dyn_extract<ConstantInt>(Node->getOperand(OpId).get()); | ||
| if (CI == nullptr) | ||
| return true; | ||
|
|
||
| Value = CI->getZExtValue(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
It does end up being slightly more verbose, but I think it's slightly clearer to use std::optional here rather than a boolean return value and an out parameter, like
static std::optional<uint32_t> extractMdIntValue(MDNode *Node,
unsigned int OpId) {
if (auto *CI =
mdconst::dyn_extract<ConstantInt>(Node->getOperand(OpId).get()))
return CI->getZExtValue();
return std::nullopt;
}then:
if (std::optional<uint32_t> Val = extractMdIntValue(RootConstantNode, 1))
NewParameter.Header.ShaderVisibility = *Val;
else
return reportError(Ctx, "Invalid value for ShaderVisibility");
Comment on lines
151
to
166
| static bool verifyShaderVisibility(uint32_t Flags) { | ||
| switch (Flags) { | ||
|
|
||
| case llvm::to_underlying(dxbc::ShaderVisibility::All): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Vertex): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Hull): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Domain): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Geometry): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Pixel): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Amplification): | ||
| case llvm::to_underlying(dxbc::ShaderVisibility::Mesh): | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Isn't this just dxbc::isValidShaderVisibility()?
Comment on lines
168
to
175
| static bool verifyParameterType(uint32_t Type) { | ||
| switch (Type) { | ||
| case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } |
| for (const auto &P : RSD.Parameters) { | ||
| if (!verifyShaderVisibility(P.Header.ShaderVisibility)) | ||
| return reportValueError(Ctx, "ShaderVisibility", | ||
| (uint32_t)P.Header.ShaderVisibility); |
There was a problem hiding this comment.
P.Header.ShaderVisibility is already uint32_t. What's this cast for?
Comment on lines
318
to
321
| OS << indent(Space) | ||
| << "Parameter Type: " << (uint32_t)P.Header.ParameterType << "\n"; | ||
| OS << indent(Space) | ||
| << "Shader Visibility: " << (uint32_t)P.Header.ShaderVisibility |
There was a problem hiding this comment.
A couple more superfluous casts.
| OS << indent(Space) | ||
| << "StaticSamplersOffset: " << RSHSize + RS.Parameters.size_in_bytes() | ||
| << ":\n"; | ||
| << "\n"; |
There was a problem hiding this comment.
Should there be some kind of header to indicate that we're printing the root parameters here, or at least an empty line? The way this reads currently it looks almost like these are associated with the static samplers since that's what printed right before.
We could also consider printing these jinline near the parts about root parameters. Something like
Definition for 'main':
Flags: 0x000001
Version: 2
RootParametersOffset: 24
NumParameters: 2
- Parameter Type: 1
Shader Visibility: 0
Register Space: 2
Shader Register: 1
Num 32 Bit Values: 3
- Parameter Type: ...
...
NumStaticSamplers: 0
StaticSamplersOffset: 48
There was a problem hiding this comment.
I think you took pieces of both of my suggestions. I think we should either:
- Put the list of parameters inline after the num parameters / offset fields
- Print out a header that says "Root Parameters:" before the list of parameters
As it is now, we print the parameters immediately after the "static samplers offset", and it isn't clear that it's a list of parameters.
There was a problem hiding this comment.
Oh sorry, I missed the first point indeed, will fix it.
| OS << indent(Space) << "Flags: " << format_hex(RS.Flags, 8) << "\n"; | ||
| OS << indent(Space) << "Version: " << RS.Version << "\n"; | ||
| OS << indent(Space) << "NumParameters: " << RS.Parameters.size() << "\n"; | ||
| OS << indent(Space) << "RootParametersOffset: " << RSHSize << "\n"; |
There was a problem hiding this comment.
Technically a pre-existing bug, but this needs to print RS.RootParametersOffset, not RSHSize
| ; DXC-NEXT: RootSignature: | ||
| ; DXC-NEXT: Version: 2 | ||
| ; DXC-NEXT: NumStaticSamplers: 0 | ||
| ; DXC-NEXT: StaticSamplersOffset: 0 |
There was a problem hiding this comment.
RootSignatureAnalysis also doesn't have support for static samplers yet, and the 48 comes from the fact that it prints RSHSize + RS.Parameters.size_in_bytes() rather than StaticSamplersOffset, which is arguably a bug. That will all need to be fixed as part of the implementation for static samplers though.
bogner
reviewed
Apr 24, 2025
| // Clang emits the root signature data in dxcontainer following a specific | ||
| // sequence. First the header, then the root parameters. The header is | ||
| // always 24 bytes long, this is why we have 24 here. | ||
| RSD.RootParameterOffset = 24U; |
There was a problem hiding this comment.
Is sizeof(dxbc::RootSignatureHeader) clearer?
| OS << indent(Space) | ||
| << "StaticSamplersOffset: " << RSHSize + RS.Parameters.size_in_bytes() | ||
| << ":\n"; | ||
| << "\n"; |
There was a problem hiding this comment.
I think you took pieces of both of my suggestions. I think we should either:
- Put the list of parameters inline after the num parameters / offset fields
- Print out a header that says "Root Parameters:" before the list of parameters
As it is now, we print the parameters immediately after the "static samplers offset", and it isn't clear that it's a list of parameters.
| } | ||
|
|
||
| for (const auto &P : RSD.Parameters) { |
There was a problem hiding this comment.
Suggested change
| for (const auto &P : RSD.Parameters) { | |
| for (const RootParameter &P : RSD.Parameters) { |
bogner
approved these changes
Apr 24, 2025
inbelic
approved these changes
Apr 24, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16602 Here is the relevant piece of the build log for the reference |
joaosaffran
added a commit
to joaosaffran/llvm-project
that referenced
this pull request
Apr 24, 2025
- Closes [llvm#126637](llvm#126637) --------- Co-authored-by: joaosaffran <[email protected]>
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Apr 24, 2025
…vm#135085)" This reverts commit 23186d1.
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Apr 24, 2025
…vm#135085)" This reverts commit 23186d1.
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
- Closes [llvm#126637](llvm#126637) --------- Co-authored-by: joaosaffran <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
- Closes [llvm#126637](llvm#126637) --------- Co-authored-by: joaosaffran <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
- Closes [llvm#126637](llvm#126637) --------- Co-authored-by: joaosaffran <[email protected]>
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
May 9, 2025
- Closes [llvm#126637](llvm#126637) --------- Co-authored-by: joaosaffran <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.