-
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 all 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 | ||
---|---|---|---|---|
|
@@ -218,6 +218,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(to_underlying(Sampler.Flags))), | ||||
}; | ||||
return MDNode::get(Ctx, Operands); | ||||
} | ||||
|
@@ -417,7 +418,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; | ||||
|
@@ -501,6 +502,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(); | ||||
} | ||||
|
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 Static Sampler Flag: 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} ; list of function/root signature pairs | ||
!2 = !{ ptr @main, !3, i32 3 } ; 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 0, i32 4 } |
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 we also have testing of an invalid value? I think it makes sense to include the basic verifications in this pr |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
; 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 [248 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, i32 3 } ; function, root signature | ||
!3 = !{ !5, !6, !7, !8 } ; 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 0, i32 1 } | ||
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 we should have a test for the other valid flag and also when their or'd value is specified. Is it allowed to have both flags set at the same time? 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've added other scenarios for all flags below. |
||
!6 = !{ !"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 43, i32 0, i32 0, i32 2 } | ||
!7 = !{ !"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 44, i32 0, i32 0, i32 0 } | ||
!8 = !{ !"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 45, i32 0, i32 0, i32 3 } | ||
|
||
; DXC: - Name: RTS0 | ||
; DXC-NEXT: Size: 248 | ||
; DXC-NEXT: RootSignature: | ||
; DXC-NEXT: Version: 3 | ||
; DXC-NEXT: NumRootParameters: 0 | ||
; DXC-NEXT: RootParametersOffset: 24 | ||
; DXC-NEXT: NumStaticSamplers: 4 | ||
; DXC-NEXT: StaticSamplersOffset: 24 | ||
; DXC-NEXT: Parameters: [] | ||
; DXC-NEXT: Samplers: | ||
; DXC-LABEL: ShaderRegister: 42 | ||
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. nit: I think 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 am avoiding the need to check all the other values in the static samplers, otherwise this test would become really big and would need to check all the other samplers parameters other than the flags 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.
Disregard - I guess this is sort of CHECK-LABEL behaviour, I saw it out of context. |
||
; DXC: SAMPLER_FLAG_UINT_BORDER_COLOR: true | ||
; DXC-LABEL: ShaderRegister: 43 | ||
; DXC: SAMPLER_FLAG_NON_NORMALIZED_COORDINATES: true | ||
; DXC-LABEL: ShaderRegister: 44 | ||
; DXC-NOT: SAMPLER_FLAG_NON_NORMALIZED_COORDINATES: | ||
; DXC-NOT: SAMPLER_FLAG_UINT_BORDER_COLOR: | ||
; DXC-LABEL: ShaderRegister: 45 | ||
; DXC: SAMPLER_FLAG_UINT_BORDER_COLOR: true | ||
; DXC-NEXT: SAMPLER_FLAG_NON_NORMALIZED_COORDINATES: 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 is the minimum change required to make the frontend emit valid metadata.