-
Notifications
You must be signed in to change notification settings - Fork 812
[DXIL] Add -fhlsl-unused-resource-bindings=reserve-all to ensure consistent binding assignments #7643
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
base: main
Are you sure you want to change the base?
[DXIL] Add -fhlsl-unused-resource-bindings=reserve-all to ensure consistent binding assignments #7643
Changes from 11 commits
9887b09
8454274
f60c594
74283d1
6f5689d
26a7f54
b63cdd2
8d7dc86
298ff34
28b24f1
cb9f345
777a50e
3fc2f2e
f5c545b
ea760bf
9169ce0
6a15db9
895724d
a633dc6
316e493
68f20d6
6d0531b
75f9cc0
c2b24ed
d018f94
c8efa7b
5c96b53
77275a4
ba1b43b
2f482a9
4492c44
118363f
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 |
|---|---|---|
|
|
@@ -550,7 +550,8 @@ class DxilLowerCreateHandleForLib : public ModulePass { | |
| ResourceRegisterAllocator.GatherReservedRegisters(DM); | ||
|
|
||
| // Remove unused resources. | ||
| DM.RemoveResourcesWithUnusedSymbols(); | ||
| if (!DM.GetConsistentBindings()) | ||
| DM.RemoveResourcesWithUnusedSymbols(); | ||
|
|
||
| unsigned newResources = DM.GetCBuffers().size() + DM.GetUAVs().size() + | ||
| DM.GetSRVs().size() + DM.GetSamplers().size(); | ||
|
|
@@ -571,6 +572,9 @@ class DxilLowerCreateHandleForLib : public ModulePass { | |
|
|
||
| bChanged |= ResourceRegisterAllocator.AllocateRegisters(DM); | ||
|
|
||
| if (DM.GetConsistentBindings()) | ||
| DM.RemoveResourcesWithUnusedSymbols(); | ||
|
||
|
|
||
| // Fill in top-level CBuffer variable usage bit | ||
| UpdateCBufferUsage(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| // RUN: %dxc -T lib_6_3 -auto-binding-space 0 -consistent-bindings %s | %D3DReflect %s | FileCheck %s | ||
|
|
||
| RWByteAddressBuffer output1; | ||
| RWByteAddressBuffer output2; | ||
| RWByteAddressBuffer output3 : register(u0); | ||
| RWByteAddressBuffer output4 : register(space1); | ||
| RWByteAddressBuffer output5 : SEMA; | ||
| RWByteAddressBuffer output6; | ||
| RWByteAddressBuffer output7 : register(u1); | ||
| RWByteAddressBuffer output8[12] : register(u3); | ||
| RWByteAddressBuffer output9[12]; | ||
| RWByteAddressBuffer output10[33] : register(space1); | ||
| RWByteAddressBuffer output11[33] : register(space2); | ||
| RWByteAddressBuffer output12[33] : register(u0, space2); | ||
|
|
||
| StructuredBuffer<float> test5; | ||
| ByteAddressBuffer input13 : SEMA; | ||
| ByteAddressBuffer input14; | ||
| ByteAddressBuffer input15 : register(t0); | ||
| ByteAddressBuffer input16[12] : register(t3); | ||
| ByteAddressBuffer input17[2] : register(space1); | ||
| ByteAddressBuffer input18[12] : register(t1, space1); | ||
| ByteAddressBuffer input19[3] : register(space1); | ||
| ByteAddressBuffer input20 : register(space1); | ||
| Texture2D tex; | ||
|
|
||
| SamplerState sampler0; | ||
| SamplerState sampler1; | ||
| SamplerState sampler2 : register(s0); | ||
| SamplerState sampler3 : register(space1); | ||
| SamplerState sampler4 : register(s0, space1); | ||
|
|
||
| cbuffer test : register(b0) { float a; }; | ||
| cbuffer test2 { float b; }; | ||
| cbuffer test3 : register(space1) { float c; }; | ||
| cbuffer test4 : register(space1) { float d; }; | ||
|
|
||
| float e; //$Global is always the first to receive bindings before cbuffers without register annotation | ||
|
|
||
| [shader("compute")] | ||
| [numthreads(16, 16, 1)] | ||
| void main(uint id : SV_DispatchThreadID) { | ||
| output1.Store(0, test5[0]); | ||
| output2.Store(id * 4, a * b * c * d * e); //Only use 1 output, but this won't result into output2 receiving wrong bindings | ||
| output3.Store(0, input13.Load(0)); | ||
| output4.Store(0, input14.Load(0)); | ||
| output5.Store(0, input15.Load(0)); | ||
| output6.Store(0, input16[0].Load(0)); | ||
| output7.Store(0, input17[0].Load(0)); | ||
| output8[0].Store(0, input18[0].Load(0)); | ||
| output9[0].Store(0, input19[0].Load(0)); | ||
| output10[0].Store(0, input20.Load(0)); | ||
| output11[0].Store(0, tex.SampleLevel(sampler0, 0.xx, 0)); | ||
| output12[0].Store(0, tex.SampleLevel(sampler1, 0.xx, 0) * tex.SampleLevel(sampler2, 0.xx, 0) * tex.SampleLevel(sampler3, 0.xx, 0) * tex.SampleLevel(sampler4, 0.xx, 0)); | ||
| } | ||
|
|
||
| // CHECK: ID3D12LibraryReflection: | ||
| // CHECK: D3D12_LIBRARY_DESC: | ||
| // CHECK: FunctionCount: 1 | ||
| // CHECK: ID3D12FunctionReflection: | ||
| // CHECK: D3D12_FUNCTION_DESC: Name: main | ||
| // CHECK: Shader Version: Compute 6.3 | ||
| // CHECK: BoundResources: 32 | ||
| // CHECK: Bound Resources: | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: $Globals | ||
| // CHECK: Type: D3D_SIT_CBUFFER | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: test | ||
| // CHECK: Type: D3D_SIT_CBUFFER | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: test2 | ||
| // CHECK: Type: D3D_SIT_CBUFFER | ||
| // CHECK: BindPoint: 2 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: test3 | ||
| // CHECK: Type: D3D_SIT_CBUFFER | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: test4 | ||
| // CHECK: Type: D3D_SIT_CBUFFER | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: sampler0 | ||
| // CHECK: Type: D3D_SIT_SAMPLER | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: sampler1 | ||
| // CHECK: Type: D3D_SIT_SAMPLER | ||
| // CHECK: BindPoint: 2 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: sampler2 | ||
| // CHECK: Type: D3D_SIT_SAMPLER | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: sampler3 | ||
| // CHECK: Type: D3D_SIT_SAMPLER | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: sampler4 | ||
| // CHECK: Type: D3D_SIT_SAMPLER | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: test5 | ||
| // CHECK: Type: D3D_SIT_STRUCTURED | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input13 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 2 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input14 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 15 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input15 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input16 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 3 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input17 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 13 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input18 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input19 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 15 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: input20 | ||
| // CHECK: Type: D3D_SIT_BYTEADDRESS | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: tex | ||
| // CHECK: BindPoint: 16 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output1 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 2 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output2 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 15 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output3 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output4 | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output5 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 16 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output6 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 17 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output7 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output8 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 3 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output9 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 18 | ||
| // CHECK: Space: 0 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output10 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 1 | ||
| // CHECK: Space: 1 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output11 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 33 | ||
| // CHECK: Space: 2 | ||
| // CHECK: D3D12_SHADER_INPUT_BIND_DESC: Name: output12 | ||
| // CHECK: Type: D3D_SIT_UAV_RWBYTEADDRESS | ||
| // CHECK: BindPoint: 0 | ||
| // CHECK: Space: 2 |
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.
We need an IR-based pass test for changes that impact this pass.
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.
Where do I add that? Any examples? (Same with the comment on DxilGenerationPass.cpp)
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.
Regarding these IR-based tests, there's a helper script:
utils/hct/ExtractIRForPassTest.py -hfor option help.You pass in the desired pass and it stops at that point and generates the test output with the needed RUN line. You'll want to add
-hlsl-dxilemitto the end of the pass list to emit the metadata.However, as I was anticipating questions related to how to target this specific area, I tried crafting a test myself. Along the way, I ran into a bug in the pass, so I put a PR up to fix the bug, along with a couple pass tests.
Here's the PR: #7934
You could copy and modify the
legalize-resource-phi.llpass test for the purposes of testing changes to thishlsl-dxil-lower-handle-for-libpass.Notice how the metadata indicates unbound resources, then it checks the binding. For this change, this should check several things:
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.
Will do in a bit. Already pulled your PR into mine locally.
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.
@tex3d I have a bit of trouble trying to wrap my head around this process. So if I simplify it in steps maybe you can see where it goes wrong.
python3 utils\hct\ExtractIRForPassTest.py D:\programming\repos\DirectXShaderCompiler\tools\clang\test\HLSLFileCheck\d3dreflect\consistent_bindings_simple.hlsl -p hlsl-dxil-lower-handle-for-lib -o test.txt -- -T cs_6_3 -E mainB -auto-binding-space 0 -fhlsl-unused-resource-bindings=reserve-allSo a few questions: