Skip to content

Conversation

@Keenuts
Copy link
Collaborator

@Keenuts Keenuts commented May 27, 2024

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.

        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 '
  • 'fvk-bind-sampler-heap '
  • 'fvk-bind-counter-heap '

@Keenuts Keenuts force-pushed the fix-4255 branch 3 times, most recently from c059911 to e64b584 Compare May 27, 2024 15:39
@Keenuts Keenuts requested review from cassiebeckley and s-perron May 27, 2024 15:40
@Keenuts Keenuts marked this pull request as ready for review May 27, 2024 15:40
@Keenuts Keenuts requested a review from a team as a code owner May 27, 2024 15:40
Copy link
Collaborator

@s-perron s-perron left a 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.

@Keenuts
Copy link
Collaborator Author

Keenuts commented Jul 3, 2024

Resolving comments around bindings as the design changed since the last version.
Binding logic to implement now:

  • All other resources follow the same rules as today (implicit & explicit bindings)
  • Then, we find the 3 first unused bindings (not necessarily consecutive, starting at set 0, binding 0)
  • Take 1 binding for resource heaps, 1 for sample heaps, and 1 for UAV counters.
  • index in the binding for UAV counter matches the index of the associated UAV in the resource heap binding.
  • 3 flags will be added: -fvk-bind-[resource,sampler,counter]-heap <set> <binding>, which allow the user to fix the binding/set pair, allowing aliasing resources.

@Keenuts
Copy link
Collaborator Author

Keenuts commented Jul 3, 2024

@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.
This PR does not include the new flags as I find it already quite big.

@Keenuts Keenuts requested a review from s-perron July 3, 2024 14:35
Copy link
Collaborator

@s-perron s-perron left a 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.

Keenuts added 5 commits July 10, 2024 13:27
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>
Signed-off-by: Nathan Gauër <brioche@google.com>
@Keenuts
Copy link
Collaborator Author

Keenuts commented Jul 10, 2024

Added the error + test, and rebased on main.

@s-perron s-perron self-requested a review July 16, 2024 00:22
Signed-off-by: Nathan Gauër <brioche@google.com>
Copy link
Collaborator

@sudonatalie sudonatalie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

SjMxr233 pushed a commit to ShaderHelper/DirectXShaderCompiler that referenced this pull request Jul 24, 2025
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants