-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[DirectX] Updating Root Signature Metadata to contain Static Sampler flags #160210
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 26 commits
787c776
57fd710
d72adfe
23803b7
fefd58c
13d945d
1d0cbd3
e530bcc
79ea587
367ac8c
a08c05e
159388d
905d5d3
b73bf24
6b191d0
cc05d4d
f02d18e
ddf9945
284901c
73e6bb5
159cd57
ae969d4
b7f07aa
c25fee8
b5ecf7f
6187125
8d017fa
efcd873
9c2c1d3
97e733b
0796e45
066c113
f38b4c3
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 | ||
---|---|---|---|---|
|
@@ -212,7 +212,7 @@ MDNode *MetadataBuilder::BuildStaticSampler(const StaticSampler &Sampler) { | |||
ConstantAsMetadata::get(Builder.getInt32(Sampler.Space)), | ||||
ConstantAsMetadata::get( | ||||
Builder.getInt32(to_underlying(Sampler.Visibility))), | ||||
}; | ||||
ConstantAsMetadata::get(Builder.getInt32(0))}; | ||||
return MDNode::get(Ctx, Operands); | ||||
} | ||||
|
||||
|
@@ -411,7 +411,7 @@ Error MetadataParser::parseDescriptorTable(mcdxbc::RootSignatureDesc &RSD, | |||
|
||||
Error MetadataParser::parseStaticSampler(mcdxbc::RootSignatureDesc &RSD, | ||||
MDNode *StaticSamplerNode) { | ||||
if (StaticSamplerNode->getNumOperands() != 14) | ||||
if (StaticSamplerNode->getNumOperands() != 15) | ||||
return make_error<InvalidRSMetadataFormat>("Static Sampler"); | ||||
|
||||
mcdxbc::StaticSampler Sampler; | ||||
|
@@ -495,6 +495,17 @@ Error MetadataParser::parseStaticSampler(mcdxbc::RootSignatureDesc &RSD, | |||
return Error(std::move(E)); | ||||
Sampler.ShaderVisibility = *Visibility; | ||||
|
||||
if (RSD.Version < 3) { | ||||
RSD.StaticSamplers.push_back(Sampler); | ||||
return Error::success(); | ||||
} | ||||
assert(RSD.Version >= 3); | ||||
Comment on lines
+505
to
+509
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 metadata always has a value now, so this early exit isn't necessary 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 metadata always had a value, this version exists just to ignore it if we are using the incorrect version and prevent the Flags to be set in the wrong version. We did the same logic for other flags that got added in different versions, 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. As per our offline discussion, I've filed an issue to throw an error when we notice the flags are being set in an unsupported version: #161579 |
||||
|
||||
if (std::optional<uint32_t> Val = extractMdIntValue(StaticSamplerNode, 14)) | ||||
Sampler.Flags = *Val; | ||||
else | ||||
return make_error<InvalidRSMetadataValue>("Static Sampler Flags"); | ||||
|
||||
RSD.StaticSamplers.push_back(Sampler); | ||||
return Error::success(); | ||||
} | ||||
|
@@ -651,6 +662,13 @@ Error MetadataParser::validateRootSignature( | |||
joinErrors(std::move(DeferredErrs), | ||||
make_error<RootSignatureValidationError<uint32_t>>( | ||||
"RegisterSpace", Sampler.RegisterSpace)); | ||||
|
||||
if (!hlsl::rootsig::verifyStaticSamplerFlags( | ||||
RSD.Version, dxbc::StaticSamplerFlags(Sampler.Flags))) | ||||
DeferredErrs = | ||||
joinErrors(std::move(DeferredErrs), | ||||
make_error<RootSignatureValidationError<uint32_t>>( | ||||
"Static Sampler Flag", Sampler.Flags)); | ||||
} | ||||
|
||||
return DeferredErrs; | ||||
|
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 | ||
|
||
target triple = "dxil-unknown-shadermodel6.0-compute" | ||
|
||
|
||
; CHECK: error: Invalid value for Version: 4 | ||
; 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, !3, !4, !5} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !6, i32 1 } ; function, root signature | ||
!3 = !{ ptr @main, !6, i32 4 } ; function, root signature | ||
!4 = !{ ptr @main, !6, i32 2 } ; function, root signature | ||
!5 = !{ ptr @main, !6, i32 3 } ; function, root signature | ||
!6 = !{ } ; list of root signature elements |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
; 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 ShaderVisibility: 666 | ||
; 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, i32 2 } ; function, root signature | ||
!3 = !{ !5 } ; list of root signature elements | ||
!5 = !{ !"StaticSampler", i32 4, i32 2, i32 3, i32 5, float 0x3FF6CCCCC0000000, i32 9, i32 3, i32 2, float -1.280000e+02, float 1.280000e+02, i32 42, i32 0, i32 666 } |
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 is a temporary fix, I am working on the frontend changes in a different PR.
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.
Note: we should probably stack the frontend changes to go in before this so that we avoid this temporary state
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.
I think that merge with this temporary fix is better. Since, if we update the frontend first, it is going to break the backend, since it still expect StaticSampler MD Nodes to contain 14 values and not 15. @bogner what do you think is the best direction here?
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.
There are a few ways to stage this, but I agree that if we're doing either the backend or the frontend first we run into similar problems. However, I do share @inbelic's concern that this is a bit of an awkward temporary state.
It might be worthwhile to just wire up enough of the frontend in this PR that it's explicitly writing out the empty flags. That is, update the StaticSampler definition to include the flags now, but just initialize it to zero in the frontend. This way the metadata stays consistent between the frontend and the backend.
Either way, this deserves a comment in the code saying what's going on.