-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[HLSL] Adding support for Root Constants in LLVM Metadata #135085
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 141 commits
526b34a
e84a8e1
f25fd64
16552f0
a6eb4c0
7d080b3
504527b
2425672
08c17bb
395a536
f93dff9
1c1edb8
5bef775
628545a
c5c2ab6
b9db72c
f4af043
496b922
422578f
b1423eb
b6262b6
16d0e8e
0a9e468
8295031
c8e1e38
434b862
2bfc5ad
479422d
f61ee77
499d879
c4af535
819fa0d
d347a87
d8824ed
25c0384
5eb0ad2
559427d
8ca5b2a
d52cd2c
5930dcb
fc72988
2d1ee0d
92a85fe
979ee91
d089674
980e7d9
04667f3
8ec40aa
b0ac6be
6a36503
f7d2c12
d6c98ed
36746f5
b5208e8
1fd6568
cde4634
4410e7b
cbdb114
667ee17
d0dae8b
b1b28f8
0efd8cc
11256d8
3c5046e
5d4505c
3117530
0af845c
bf49a3a
78826a5
4e689e9
b0d0180
3c6894f
08f6ddc
b232967
1026a8e
00175bf
9ed2adc
e8252ba
4de5c29
fe13b61
767b7d0
8434dc2
d391727
68c7513
23069ab
d14471b
216341c
85f012c
1e2bcf5
0e277d9
5cd0044
7a7c34d
d3fafab
15d1a8c
7485640
17abc82
4b177e2
ec1dd87
eb9d7d3
f2a4f04
7c4236c
2894f96
7b5e9d8
f0e6a46
4fe30df
cbc334a
31bcd73
30098e1
1bf6408
d1b32f3
3b9bf27
89632a4
67da709
3991c5d
cfc6bfb
82a31fa
9b59d01
efc5e52
e6865a7
021976d
102ff4b
39d4b08
7343d95
6986945
c0ac522
cadc296
c4b78d8
8bdc206
1e5eee5
a968d81
8511fa9
330369a
d747bcc
6cef567
246d5d3
bb6c0cf
2203885
7aed7d1
7885eed
f315bb4
193a80b
578faea
860fddd
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 | ||||
---|---|---|---|---|---|---|
|
@@ -40,14 +40,64 @@ static bool reportError(LLVMContext *Ctx, Twine Message, | |||||
return true; | ||||||
} | ||||||
|
||||||
static bool reportValueError(LLVMContext *Ctx, Twine ParamName, uint32_t Value, | ||||||
DiagnosticSeverity Severity = DS_Error) { | ||||||
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. 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. |
||||||
Ctx->diagnose(DiagnosticInfoGeneric( | ||||||
"Invalid value for " + ParamName + ": " + Twine(Value), Severity)); | ||||||
return true; | ||||||
} | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does end up being slightly more verbose, but I think it's slightly clearer to use 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"); |
||||||
|
||||||
static bool parseRootFlags(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | ||||||
MDNode *RootFlagNode) { | ||||||
|
||||||
if (RootFlagNode->getNumOperands() != 2) | ||||||
return reportError(Ctx, "Invalid format for RootFlag Element"); | ||||||
|
||||||
auto *Flag = mdconst::extract<ConstantInt>(RootFlagNode->getOperand(1)); | ||||||
RSD.Flags = Flag->getZExtValue(); | ||||||
if (extractMdIntValue(RSD.Flags, RootFlagNode, 1)) | ||||||
return reportError(Ctx, "Invalid value for RootFlag"); | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
||||||
static bool parseRootConstants(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | ||||||
MDNode *RootConstantNode) { | ||||||
|
||||||
if (RootConstantNode->getNumOperands() != 5) | ||||||
return reportError(Ctx, "Invalid format for RootConstants Element"); | ||||||
|
||||||
mcdxbc::RootParameter NewParameter; | ||||||
NewParameter.Header.ParameterType = | ||||||
llvm::to_underlying(dxbc::RootParameterType::Constants32Bit); | ||||||
|
||||||
uint32_t SV; | ||||||
if (extractMdIntValue(SV, RootConstantNode, 1)) | ||||||
return reportError(Ctx, "Invalid value for ShaderVisibility"); | ||||||
|
||||||
NewParameter.Header.ShaderVisibility = SV; | ||||||
|
||||||
if (extractMdIntValue(NewParameter.Constants.ShaderRegister, RootConstantNode, | ||||||
2)) | ||||||
return reportError(Ctx, "Invalid value for ShaderRegister"); | ||||||
|
||||||
if (extractMdIntValue(NewParameter.Constants.RegisterSpace, RootConstantNode, | ||||||
3)) | ||||||
return reportError(Ctx, "Invalid value for RegisterSpace"); | ||||||
|
||||||
if (extractMdIntValue(NewParameter.Constants.Num32BitValues, RootConstantNode, | ||||||
4)) | ||||||
return reportError(Ctx, "Invalid value for Num32BitValues"); | ||||||
|
||||||
RSD.Parameters.push_back(NewParameter); | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
@@ -62,12 +112,16 @@ static bool parseRootSignatureElement(LLVMContext *Ctx, | |||||
RootSignatureElementKind ElementKind = | ||||||
StringSwitch<RootSignatureElementKind>(ElementText->getString()) | ||||||
.Case("RootFlags", RootSignatureElementKind::RootFlags) | ||||||
.Case("RootConstants", RootSignatureElementKind::RootConstants) | ||||||
.Default(RootSignatureElementKind::Error); | ||||||
|
||||||
switch (ElementKind) { | ||||||
|
||||||
case RootSignatureElementKind::RootFlags: | ||||||
return parseRootFlags(Ctx, RSD, Element); | ||||||
case RootSignatureElementKind::RootConstants: | ||||||
return parseRootConstants(Ctx, RSD, Element); | ||||||
break; | ||||||
case RootSignatureElementKind::Error: | ||||||
return reportError(Ctx, "Invalid Root Signature Element: " + | ||||||
ElementText->getString()); | ||||||
|
@@ -94,10 +148,55 @@ static bool parse(LLVMContext *Ctx, mcdxbc::RootSignatureDesc &RSD, | |||||
|
||||||
static bool verifyRootFlag(uint32_t Flags) { return (Flags & ~0xfff) == 0; } | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this just |
||||||
|
||||||
static bool verifyParameterType(uint32_t Type) { | ||||||
switch (Type) { | ||||||
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): | ||||||
return true; | ||||||
} | ||||||
|
||||||
return false; | ||||||
} | ||||||
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. Same here |
||||||
|
||||||
static bool verifyVersion(uint32_t Version) { | ||||||
return (Version == 1 || Version == 2); | ||||||
} | ||||||
|
||||||
static bool validate(LLVMContext *Ctx, const mcdxbc::RootSignatureDesc &RSD) { | ||||||
|
||||||
if (!verifyVersion(RSD.Version)) { | ||||||
return reportValueError(Ctx, "Version", RSD.Version); | ||||||
} | ||||||
|
||||||
if (!verifyRootFlag(RSD.Flags)) { | ||||||
return reportError(Ctx, "Invalid Root Signature flag value"); | ||||||
return reportValueError(Ctx, "RootFlags", RSD.Flags); | ||||||
} | ||||||
|
||||||
for (const auto &P : RSD.Parameters) { | ||||||
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.
Suggested change
|
||||||
if (!verifyShaderVisibility(P.Header.ShaderVisibility)) | ||||||
return reportValueError(Ctx, "ShaderVisibility", | ||||||
(uint32_t)P.Header.ShaderVisibility); | ||||||
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.
|
||||||
|
||||||
assert(verifyParameterType(P.Header.ParameterType) && | ||||||
"Invalid value for ParameterType"); | ||||||
} | ||||||
|
||||||
return false; | ||||||
} | ||||||
|
||||||
|
@@ -205,14 +304,35 @@ PreservedAnalyses RootSignatureAnalysisPrinter::run(Module &M, | |||||
|
||||||
// start root signature header | ||||||
Space++; | ||||||
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"; | ||||||
OS << indent(Space) << "NumStaticSamplers: " << 0 << ":\n"; | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically a pre-existing bug, but this needs to print |
||||||
OS << indent(Space) << "NumStaticSamplers: " << 0 << "\n"; | ||||||
OS << indent(Space) | ||||||
<< "StaticSamplersOffset: " << RSHSize + RS.Parameters.size_in_bytes() | ||||||
<< ":\n"; | ||||||
<< "\n"; | ||||||
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 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
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 think you took pieces of both of my suggestions. I think we should either:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry, I missed the first point indeed, will fix it. |
||||||
|
||||||
Space++; | ||||||
for (auto const &P : RS.Parameters) { | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple more superfluous casts. |
||||||
<< "\n"; | ||||||
switch (P.Header.ParameterType) { | ||||||
case llvm::to_underlying(dxbc::RootParameterType::Constants32Bit): | ||||||
OS << indent(Space) << "Register Space: " << P.Constants.RegisterSpace | ||||||
<< "\n"; | ||||||
OS << indent(Space) << "Shader Register: " << P.Constants.ShaderRegister | ||||||
<< "\n"; | ||||||
OS << indent(Space) | ||||||
<< "Num 32 Bit Values: " << P.Constants.Num32BitValues << "\n"; | ||||||
break; | ||||||
} | ||||||
} | ||||||
Space--; | ||||||
|
||||||
Space--; | ||||||
// end root signature header | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
; RUN: not opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s | ||
|
||
; CHECK: error: Invalid value for ShaderVisibility: 255 | ||
; CHECK-NOT: Root Signature Definitions | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
define void @main() #0 { | ||
entry: | ||
ret void | ||
} | ||
|
||
attributes #0 = { "hlsl.numthreads"="1,1,1" "hlsl.shader"="compute" } | ||
|
||
inbelic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"RootConstants", i32 255, i32 1, i32 2, i32 3 } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
; RUN: opt -passes='print<dxil-root-signature>' %s -S -o - 2>&1 | FileCheck %s | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
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 = !{ !4, !5 } ; list of root signature elements | ||
!4 = !{ !"RootFlags", i32 1 } ; 1 = allow_input_assembler_input_layout | ||
!5 = !{ !"RootConstants", i32 0, i32 1, i32 2, i32 3 } | ||
|
||
; CHECK-LABEL: Definition for 'main': | ||
; CHECK-NEXT: Flags: 0x000001 | ||
; CHECK-NEXT: Version: 2 | ||
; CHECK-NEXT: NumParameters: 1 | ||
; CHECK-NEXT: RootParametersOffset: 24 | ||
; CHECK-NEXT: NumStaticSamplers: 0 | ||
; CHECK-NEXT: StaticSamplersOffset: 48 | ||
; CHECK-NEXT: Parameter Type: 1 | ||
; CHECK-NEXT: Shader Visibility: 0 | ||
; CHECK-NEXT: Register Space: 2 | ||
; CHECK-NEXT: Shader Register: 1 | ||
; CHECK-NEXT: Num 32 Bit Values: 3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
; 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 Num32BitValues | ||
; CHECK-NOT: Root Signature Definitions | ||
|
||
define void @main() { | ||
entry: | ||
ret void | ||
} | ||
|
||
!dx.rootsignatures = !{!2} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"RootConstants", i32 0, i32 1, i32 2, !"Invalid" } |
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 | ||
; 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 = !{ !"RootConstants", i32 0, i32 1, !"Invalid", 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 ShaderRegister | ||
; 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 = !{ !"RootConstants", i32 0, !"Invalid", i32 2, i32 3 } |
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. The test name implies that we are only testing root constants. Can we either change the name or remove root flags |
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 = !{ !"RootConstants", i32 0, i32 1, i32 2, i32 3 } | ||
|
||
; 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 | ||
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. This has a different value from the test above (0 vs 48). Presumably they should be the same? 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. The difference comes to the fact that 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.
|
||
; DXC-NEXT: Parameters: | ||
; DXC-NEXT: - ParameterType: 1 | ||
; DXC-NEXT: ShaderVisibility: 0 | ||
; DXC-NEXT: Constants: | ||
; DXC-NEXT: Num32BitValues: 3 | ||
; DXC-NEXT: RegisterSpace: 2 | ||
; DXC-NEXT: ShaderRegister: 1 |
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.
Can we add a comment as to why it should be 24
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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