-
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 17 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,6 +212,8 @@ 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.Visibility))), | ||||||
|
Builder.getInt32(to_underlying(Sampler.Visibility))), | |
Builder.getInt32(0)), |
I think 0 makes more sense as the temporary measure
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.
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 comment
The 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 Root Descriptor
assert(RSD.Version > 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.
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
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 |
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.