-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[HLSL] Adding support for root descriptors in root signature metadata representation #139781
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
Changes from 31 commits
0abacfc
8b8c02a
7ac9641
c105458
efe76aa
a928e9d
a38f10b
9a7c359
d6c2b55
93e4cf2
b45b1b6
f804a23
15eb6f5
b9d7f07
46cc8c1
1b3e10a
1f31957
e8fbfce
a31e5a5
a394ad0
ad415a7
8ff4845
f875555
4f7f998
3eb5e10
58e1789
81915ad
a515e28
0d54162
7f70dc5
0c570c8
d1ca37d
cb0780b
eeffded
3cbe0cf
92b766b
8732594
fdb8b98
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,14 @@ static std::optional<uint32_t> extractMdIntValue(MDNode *Node, | |
return std::nullopt; | ||
} | ||
|
||
static std::optional<StringRef> extractMdStringValue(MDNode *Node, | ||
unsigned int OpId) { | ||
MDString *NodeText = cast<MDString>(Node->getOperand(OpId)); | ||
if (NodeText == nullptr) | ||
return std::nullopt; | ||
return NodeText->getString(); | ||
} | ||
|
||
static bool parseRootFlags(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | ||
MDNode *RootFlagNode) { | ||
|
||
|
@@ -107,6 +115,59 @@ static bool parseRootConstants(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | |
return false; | ||
} | ||
|
||
static bool parseRootDescriptors(LLVMContext *Ctx, | ||
mcdxbc::RootSignatureDesc &RSD, | ||
MDNode *RootDescriptorNode) { | ||
|
||
if (RootDescriptorNode->getNumOperands() != 5) | ||
return reportError(Ctx, "Invalid format for Root Descriptor Element"); | ||
|
||
std::optional<StringRef> ElementText = | ||
extractMdStringValue(RootDescriptorNode, 0); | ||
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. We should add a check that this is not 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. Thanks, can we also add a test case to cover it? 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. There is this test that covers it already: llvm/test/CodeGen/DirectX/ContainerData/RootSignature-RootDescriptor-Invalid-RegisterSpace.ll 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 don't see how that test would cover this. That test checks that the register space is in range (ie, it tests the |
||
|
||
if (!ElementText.has_value()) | ||
return reportError(Ctx, "Root Descriptor, first element is not a string."); | ||
|
||
dxbc::RTS0::v1::RootParameterHeader Header; | ||
// a default scenario is not needed here. Scenarios where ElementText is | ||
// invalid is previously checked and error handled when this method is called. | ||
Header.ParameterType = | ||
StringSwitch<uint32_t>(*ElementText) | ||
.Case("RootCBV", llvm::to_underlying(dxbc::RootParameterType::CBV)) | ||
.Case("RootSRV", llvm::to_underlying(dxbc::RootParameterType::SRV)) | ||
.Case("RootUAV", llvm::to_underlying(dxbc::RootParameterType::UAV)); | ||
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. Without a default case, this is equivalent to 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 did some research, it seems that it would be undefined behavior, @bogner correct me if I am wrong please. Will update to handle it better 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. Took a look into this, I don't think a default scenario is needed here, this is previously checked and error handled when this method is called. 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. Okay, can we add a comment explaining where it was previously asserted? Or we could assert that here as well? 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. Why don't we pass |
||
|
||
if (std::optional<uint32_t> Val = extractMdIntValue(RootDescriptorNode, 1)) | ||
Header.ShaderVisibility = *Val; | ||
else | ||
return reportError(Ctx, "Invalid value for ShaderVisibility"); | ||
|
||
dxbc::RTS0::v2::RootDescriptor Descriptor; | ||
if (std::optional<uint32_t> Val = extractMdIntValue(RootDescriptorNode, 2)) | ||
Descriptor.ShaderRegister = *Val; | ||
else | ||
return reportError(Ctx, "Invalid value for ShaderRegister"); | ||
|
||
if (std::optional<uint32_t> Val = extractMdIntValue(RootDescriptorNode, 3)) | ||
Descriptor.RegisterSpace = *Val; | ||
else | ||
return reportError(Ctx, "Invalid value for RegisterSpace"); | ||
|
||
if (RSD.Version == 1) { | ||
RSD.ParametersContainer.addParameter(Header, Descriptor); | ||
return false; | ||
} | ||
assert(RSD.Version > 1); | ||
|
||
if (std::optional<uint32_t> Val = extractMdIntValue(RootDescriptorNode, 4)) | ||
Descriptor.Flags = *Val; | ||
else | ||
return reportError(Ctx, "Invalid value for Root Descriptor Flags"); | ||
|
||
RSD.ParametersContainer.addParameter(Header, Descriptor); | ||
return false; | ||
} | ||
|
||
static bool parseRootSignatureElement(LLVMContext *Ctx, | ||
mcdxbc::RootSignatureDesc &RSD, | ||
MDNode *Element) { | ||
|
@@ -118,6 +179,9 @@ static bool parseRootSignatureElement(LLVMContext *Ctx, | |
StringSwitch<RootSignatureElementKind>(ElementText->getString()) | ||
.Case("RootFlags", RootSignatureElementKind::RootFlags) | ||
.Case("RootConstants", RootSignatureElementKind::RootConstants) | ||
.Case("RootCBV", RootSignatureElementKind::RootDescriptors) | ||
.Case("RootSRV", RootSignatureElementKind::RootDescriptors) | ||
.Case("RootUAV", RootSignatureElementKind::RootDescriptors) | ||
.Default(RootSignatureElementKind::Error); | ||
|
||
switch (ElementKind) { | ||
|
@@ -126,7 +190,8 @@ static bool parseRootSignatureElement(LLVMContext *Ctx, | |
return parseRootFlags(Ctx, RSD, Element); | ||
case RootSignatureElementKind::RootConstants: | ||
return parseRootConstants(Ctx, RSD, Element); | ||
break; | ||
case RootSignatureElementKind::RootDescriptors: | ||
return parseRootDescriptors(Ctx, RSD, Element); | ||
case RootSignatureElementKind::Error: | ||
return reportError(Ctx, "Invalid Root Signature Element: " + | ||
ElementText->getString()); | ||
|
@@ -157,6 +222,16 @@ static bool verifyVersion(uint32_t Version) { | |
return (Version == 1 || Version == 2); | ||
} | ||
|
||
static bool verifyRegisterValue(uint32_t RegisterValue) { | ||
return !(RegisterValue == 0xFFFFFFFF); | ||
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. Clearer to use |
||
} | ||
|
||
static bool verifyRegisterSpace(uint32_t RegisterSpace) { | ||
return !(RegisterSpace >= 0xFFFFFFF0 && RegisterSpace <= 0xFFFFFFFF); | ||
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. Can you add a comment about what the condition we're checking here is? Why are specifically the largest 16 values of a uint32 invalid? |
||
} | ||
|
||
static bool verifyDescriptorFlag(uint32_t Flags) { return (Flags & ~0xE) == 0; } | ||
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. self-note: this is only called on v2, so it implicitly handles the different verification based on version. |
||
|
||
static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) { | ||
|
||
if (!verifyVersion(RSD.Version)) { | ||
|
@@ -174,6 +249,28 @@ static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) { | |
|
||
assert(dxbc::isValidParameterType(Info.Header.ParameterType) && | ||
"Invalid value for ParameterType"); | ||
|
||
switch(Info.Header.ParameterType) { | ||
|
||
case llvm::to_underlying(dxbc::RootParameterType::CBV): | ||
case llvm::to_underlying(dxbc::RootParameterType::UAV): | ||
case llvm::to_underlying(dxbc::RootParameterType::SRV): { | ||
const dxbc::RTS0::v2::RootDescriptor &Descriptor = RSD.ParametersContainer.getRootDescriptor(Info.Location); | ||
if (!verifyRegisterValue(Descriptor.ShaderRegister)) | ||
return reportValueError(Ctx, "ShaderRegister", | ||
Descriptor.ShaderRegister); | ||
|
||
if (!verifyRegisterSpace(Descriptor.RegisterSpace)) | ||
return reportValueError(Ctx, "RegisterSpace", | ||
Descriptor.RegisterSpace); | ||
|
||
if(RSD.Version > 1) { | ||
if (!verifyDescriptorFlag(Descriptor.Flags)) | ||
return reportValueError(Ctx, "DescriptorFlag", Descriptor.Flags); | ||
} | ||
break; | ||
} | ||
} | ||
} | ||
|
||
return false; | ||
|
@@ -313,6 +410,20 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M, | |
<< "Shader Register: " << Constants.ShaderRegister << "\n"; | ||
OS << indent(Space + 2) | ||
<< "Num 32 Bit Values: " << Constants.Num32BitValues << "\n"; | ||
break; | ||
} | ||
case llvm::to_underlying(dxbc::RootParameterType::CBV): | ||
case llvm::to_underlying(dxbc::RootParameterType::UAV): | ||
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. Please clang-format, this looks off. |
||
case llvm::to_underlying(dxbc::RootParameterType::SRV): { | ||
const dxbc::RTS0::v2::RootDescriptor &Descriptor = | ||
RS.ParametersContainer.getRootDescriptor(Loc); | ||
OS << indent(Space + 2) | ||
<< "Register Space: " << Descriptor.RegisterSpace << "\n"; | ||
OS << indent(Space + 2) | ||
<< "Shader Register: " << Descriptor.ShaderRegister << "\n"; | ||
if(RS.Version > 1) | ||
OS << indent(Space + 2) << "Flags: " << Descriptor.Flags << "\n"; | ||
break; | ||
} | ||
} | ||
Space--; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
; RUN: not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
; CHECK: error: Invalid value for DescriptorFlag: 3 | ||
; CHECK-NOT: Root Signature Definitions | ||
define void @main() #0 { | ||
entry: | ||
ret void | ||
} | ||
attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"RootCBV", i32 0, i32 1, i32 2, i32 3 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
; RUN: not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
; CHECK: error: Invalid Root Signature Element: Invalid | ||
; CHECK-NOT: Root Signature Definitions | ||
define void @main() #0 { | ||
entry: | ||
ret void | ||
} | ||
attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"Invalid", i32 0, i32 1, i32 2, i32 3 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
; RUN: not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
; CHECK: error: Invalid value for RegisterSpace: 4294967280 | ||
; CHECK-NOT: Root Signature Definitions | ||
define void @main() #0 { | ||
entry: | ||
ret void | ||
} | ||
attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"RootCBV", i32 0, i32 1, i32 4294967280, i32 0 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
; RUN: not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
; CHECK: error: Invalid value for ShaderRegister: 4294967295 | ||
; CHECK-NOT: Root Signature Definitions | ||
define void @main() #0 { | ||
entry: | ||
ret void | ||
} | ||
attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"RootCBV", i32 0, i32 4294967295, i32 2, i32 3 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
; RUN: opt %s -dxil-embed -dxil-globals -S -o - | FileCheck %s | ||
; RUN: llc %s --filetype=obj -o - | obj2yaml | FileCheck %s --check-prefix=DXC | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
; CHECK: @dx.rts0 = private constant [48 x i8] c"{{.*}}", section "RTS0", align 4 | ||
|
||
define void @main() #0 { | ||
entry: | ||
ret void | ||
} | ||
attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"RootCBV", i32 0, i32 1, i32 2, i32 8 } | ||
|
||
; DXC: - Name: RTS0 | ||
; DXC-NEXT: Size: 48 | ||
; DXC-NEXT: RootSignature: | ||
; DXC-NEXT: Version: 2 | ||
; DXC-NEXT: NumRootParameters: 1 | ||
; DXC-NEXT: RootParametersOffset: 24 | ||
; DXC-NEXT: NumStaticSamplers: 0 | ||
; DXC-NEXT: StaticSamplersOffset: 0 | ||
; DXC-NEXT: Parameters: | ||
; DXC-NEXT: - ParameterType: 2 | ||
; DXC-NEXT: ShaderVisibility: 0 | ||
; DXC-NEXT: Descriptor: | ||
; DXC-NEXT: RegisterSpace: 2 | ||
; DXC-NEXT: ShaderRegister: 1 | ||
; DXC-NEXT: DATA_STATIC: true |
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 doesn't look correct.
cast<>
will assert if the type is wrong, not return null, so this condition is unreachable (except maybe ifNode
itself is null?). I think you meant to usedyn_cast
here.I suspect this is why a test case like the following currently crashes:
Uh oh!
There was an error while loading. Please reload this page.
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.
Looks like you fixed the crash here, but could you please add a test?