-
Notifications
You must be signed in to change notification settings - Fork 825
[SPIR-V] Add support for D3D12 descriptor heaps #6650
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
Conversation
c059911 to
e64b584
Compare
s-perron
left a comment
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 implementation looks pretty good. But we need to work on how the bindings and descriptor sets are assigned. There does seem to be some misunderstanding. We also need to consider the interaction with -fspv-flatten-resource-arrays.
|
Resolving comments around bindings as the design changed since the last version.
|
|
@s-perron I think this is ready for another review. I modified the PR description and the documentation to match the new binding allocation behavior. |
s-perron
left a comment
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.
Looks good.
I have one more case to consider. I expect we would want to disallow passing the heaps as function parameters. As far as I know, it cannot be done without templates because we do not even know the type.
This commit adds support for Sampler/Resource descriptor heaps in DXC.
Support for those heaps on the SPIR-V side requires no other extension
than SPV_EXT_descriptor_indexing.
On the Vulkan side, the VK_EXT_mutable_descriptor_type will be required
as multiple descriptor types must be allowed on the same binding.
The way this is implemented is by reserving 2 descriptor sets:
- one for resources/samplers
- one for counters.
This means the sampler & descriptor heaps are both sharing the same
descriptor set.
Counters are stored in their own set, so their index can match the
associated resource index:
```hlsl
RWStructuredBuffer a = ResourceDescriptorHeap[2];
// descriptorSet 0, binding 2 stores the buffer.
// descriptorSet 1, binding 2 stores the counter.
```
The choice to share descriptor across resource/sampler it because I
felt this was simpler to use on the API side.
The choice to allocate a second descriptor and share the bindings
indices between the resource and the counter was done because it felt
the most intuitive method to use them.
```hlsl
RWStructuredBuffer a = ResourceDescriptorHeap[0];
RWStructuredBuffer b = ResourceDescriptorHeap[1];
```
```hlsl
RWStructuredBuffer a = ResourceDescriptorHeap[0];
a.IncrementCounter();
// Not A needs 2 bindings, so the second resource needs to be
shifted.
RWStructuredBuffer b = ResourceDescriptorHeap[2];
```
For now, descriptor set indices are fixed: 0 for heaps, 1 for counters.
I think this is fine for now, but we might want to add a flag to change
those values, in case some engine wants to specific descriptors.
Also, I decided to forbid usage of non-explicit bindings when heaps are
in use:
```hlsl
Texture2D textureA; // Forbidden. If the binding set=0, b=0 or set=2, b=0?
float4 main() : SV_Target {
Texture2D textureB = ResourceDescriptorHeap[0];
```
```hlsl
[[vk::binding(0, 0)]]
Texture2D textureA; // Allowed
float4 main() : SV_Target {
Texture2D textureB = ResourceDescriptorHeap[0];
```
The reason is usability: I'd assume if you use explicit bindings, you
know what you are doing. Meaning you might want to alias a resource in
the heap using a specific bindings, or declare resources in other
descriptor sets.
But when using the default behavior, the sudden change in behavior might
not be wanted: shall the declared resource alias resources from the
heap? Or shall a 3rd descriptor set be used for those?
This could also be solved through a flag to define the default set, and
allow mixed usage of heaps and global resources. But until there is a
clear ask for this, I wanted to err on the safe side.
Fixes microsoft#4255
Signed-off-by: Nathan Gauër <brioche@google.com>
Signed-off-by: Nathan Gauër <brioche@google.com>
Signed-off-by: Nathan Gauër <brioche@google.com>
|
Added the error + test, and rebased on main. |
Signed-off-by: Nathan Gauër <brioche@google.com>
sudonatalie
left a comment
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.
LGTM!
This commit adds support for Sampler/Resource descriptor heaps in DXC.
Support for those heaps on the SPIR-V side requires no other extension
than SPV_EXT_descriptor_indexing.
On the Vulkan side, the VK_EXT_mutable_descriptor_type will be required
as multiple descriptor types must be allowed on the same binding.
When loading a type from a heap, DXC generates a new OpRuntimeArray of
the correct type, and binds it to `set=0,
binding=<BindingNumberOfTheHeap>`.
This means multiple OpRuntimeArrays will share the same binding. This is
why VK_EXT_mutable_descriptor_type is required.
This implementation uses at most 3 bindings:
- N OpRuntimeArray as binding A for the ResourceDescriptorHeap
- N OpRuntimeArray as binding B for the SamplerDescriptorHeap
- 1 OpRuntimeArray %counter_type for the ResourceDescriptorHeap
counters.
The bindings are only allocated if used. If only the
SamplerDescriptorHeap is used, a single binding is required.
The binding allocation logic is:
1. allocate bindings for every resources, excluding heaps.
2. If ResourceDescriptorHeap is used, find the first unused binding in
set=0 and use it.
3. Same for the SamplerDescriptorHeap
4. Same for the counters.
UAV counters are not always created, only if used.
When used, they are stored in an OpRuntimeArray. The index of a counter
in that array
is equal to the index of the associated resource in its own
OpRuntimeArray.
```hlsl
RWStructuredBuffer a = ResourceDescriptorHeap[2];
a.IncrementCounter();
// buffer in descriptorSet 0, binding 0, OpRuntimeArray[index=2]
// counter in descriptorSet 0, binding 1, OpRuntimeArray[index=2]
```
As-is, this PR doesn't allow resource heaps to alias regular resources,
or to overlap.
A follow-up PR will add 3 flags to override each binding/set pairs:
- 'fvk-bind-resource-heap <set> <binding>'
- 'fvk-bind-sampler-heap <set> <binding>'
- 'fvk-bind-counter-heap <set> <binding>'
---------
Signed-off-by: Nathan Gauër <brioche@google.com>
This commit adds support for Sampler/Resource descriptor heaps in DXC. Support for those heaps on the SPIR-V side requires no other extension than SPV_EXT_descriptor_indexing.
On the Vulkan side, the VK_EXT_mutable_descriptor_type will be required as multiple descriptor types must be allowed on the same binding.
When loading a type from a heap, DXC generates a new OpRuntimeArray of the correct type, and binds it to
set=0, binding=<BindingNumberOfTheHeap>.This means multiple OpRuntimeArrays will share the same binding. This is why VK_EXT_mutable_descriptor_type is required.
This implementation uses at most 3 bindings:
The bindings are only allocated if used. If only the SamplerDescriptorHeap is used, a single binding is required.
The binding allocation logic is:
UAV counters are not always created, only if used.
When used, they are stored in an OpRuntimeArray. The index of a counter in that array
is equal to the index of the associated resource in its own OpRuntimeArray.
As-is, this PR doesn't allow resource heaps to alias regular resources, or to overlap.
A follow-up PR will add 3 flags to override each binding/set pairs: