Skip to content
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

Request for a generic descriptor indexing intrinsic #4351

Open
dubiousconst282 opened this issue Jun 12, 2024 · 16 comments · May be fixed by #4389
Open

Request for a generic descriptor indexing intrinsic #4351

dubiousconst282 opened this issue Jun 12, 2024 · 16 comments · May be fixed by #4389
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang kind:enhancement a desirable new feature, option, or behavior

Comments

@dubiousconst282
Copy link

Descriptor indexing allows shaders to access resources in a natural and flexible way, while further eliminating the complexity of managing and updating descriptor sets. This feature can already be used today in Slang, but it requires that global bindings be declared for every accessed resource type and format beforehand, which can become a bit tedious and makes it difficult for users to implement abstractions on top.

Following from #4150, it is not possible to define bindings inside a generic type due to design constraints in the compiler. What I propose instead, is for Slang to provide an intrinsic type that allows indexing over arbitrary resource types, similarly to ResourceDescriptorHeap in HLSL. This way, only one binding variable would need to be defined per descriptor set and binding index:

// Provided by Slang
__magic_type(...)
struct __RuntimeDescriptorArray {
    __intrinsic_op(...)
    T Get<T>(uint index);
}; 

// User code
[vk::binding(0, 0)] 
__RuntimeDescriptorArray g_TextureDescriptors;  // treated as opaque / unbounded array by layout and reflection

let texture = g_TextureDescriptors.Get<Texture2D<uint2>>(...);

The SPIRV backend would then emit the appropriate OpVariable definitions as it finds the specialized Get() calls. Potentially, this could also be used to implement the HLSL global heaps in a relatively portable way, although I'm not familiar with their binding model.

If this gets approved, I would be willing to work on an implementation if that is preferred.

@csyonghe
Copy link
Collaborator

We should provide something similar to ResourceDescriptorHeap and SamplerDescriptorHeap in HLSL.

Since D3D12/HLSL use separate descriptor heap for resources and samplers, we likely will need to keep them separate in Slang so we have a way to map to HLSL.

We would be happy to take patches for this implementation. The interface you proposed is fine, my only comment is that we should separate ResourceDescriptorArray and SamplerDescriptorArray to make hlsl easy to support.

@csyonghe
Copy link
Collaborator

We need to figure out how this DescirptorArray is reported by reflection API, and how to assign bindings for it.

In our layout rules, we should make it so that a DescriptorArray occupies a whole space, and is reflected as an unsized array of DescriptorTableSlot resource type.

@dubiousconst282
Copy link
Author

Thanks for your comments.

I agree about separating the resource and sampler heaps, but I'm a bit confused about whether these should be implemented with two different types or with the proposed type but in different globals for the (future) HLSL target. A constrained generic type in the struct could also work, although there doesn't seem to be a common interface for buffers yet.

The layout rules and reflection types make sense, I had not thought much of them at first so that is nice info to have :)

@csyonghe
Copy link
Collaborator

I quickly tried prototyping this in our current type system, and it seems something like this will work:
image

@jkwak-work
Copy link
Collaborator

I will be happy to be wrong but it may fall in to a known limitation described in #3804.
The second get shows red underline and that would be hard to resolve, I think.

@csyonghe
Copy link
Collaborator

csyonghe commented Jun 12, 2024

The red underline there means the type system is working as intended. A Sampler2D is not a Resource kind, so you shouldn't be allowed to get a Sampler2D from a array that contains only resources.

@natevm
Copy link
Contributor

natevm commented Jun 12, 2024

Can the "Resource" and "Sampler" integers there be integers evaluated at runtime?

Eg,

ResourceArray<Resource> resArray;
int loc = resArray.get<int>(0);
Texture2D t = resArray.get<Texture2D>(loc);

@natevm
Copy link
Contributor

natevm commented Jun 12, 2024

The red underline there means the type system is working as intended. A Sampler2D is not a Resource kind, so you shouldn't be allowed to get a Sampler2D from a array that contains only resources.

Isn't the motivation for such a feature exactly this though? It should be possible to dynamically have different types bound to different offsets within the same descriptor array.

This feature can already be used today in Slang, but it requires that global bindings be declared for every accessed resource type and format beforehand, which can become a bit tedious and makes it difficult for users to implement abstractions on top.

@csyonghe
Copy link
Collaborator

We can have different type of Resources in the ResourceArray, but you can't get a Resource handle from a Sampler array.

This is because HLSL/D3D distinguishes a Sampler descriptor heap from a resource descriptor heap, unlike in Vulkan everything can be placed in a single descriptor set allocated from a single descriptor heap.

@csyonghe
Copy link
Collaborator

Can the "Resource" and "Sampler" integers there be integers evaluated at runtime?

Eg,

ResourceArray<Resource> resArray;
int loc = resArray.get<int>(0);
Texture2D t = resArray.get<Texture2D>(loc);

You mean the index passsed to get method being a runtime value. Yes this is the intended use of this type.

@natevm
Copy link
Contributor

natevm commented Jun 12, 2024

For some context, this is what I'm kinda forced into doing at the moment with Slang. I force users to include some code which inlines the below descriptor arrays, which then I can sorta manage myself in the host code.

[[vk::binding(0, 1)]] SamplerState samplers[];
[[vk::binding(0, 2)]] Texture1D texture1Ds[];
[[vk::binding(0, 3)]] Texture2D texture2Ds[];
[[vk::binding(0, 4)]] Texture3D texture3Ds[];
[[vk::binding(0, 5)]] RWByteAddressBuffer buffers[];

But this is also very constraining. For example, I can't distinguish between different buffer types. At the same time, I can't just enumerate all the different types of descriptors and make bindings for each one.

@natevm
Copy link
Contributor

natevm commented Jun 12, 2024

It's also very frustrating to need to include texture1Ds and texture3Ds when more often than not, folks using my framework don't use them. I have to add in these placeholder descriptors for each new type, eg a volume of 1 voxel, texture with 1 pixel, a buffer with one integer. It would be really nice if I didn't have to do that...

@natevm
Copy link
Contributor

natevm commented Jun 12, 2024

We can have different type of Resources in the ResourceArray, but you can't get a Resource handle from a Sampler array.

This is because HLSL/D3D distinguishes a Sampler descriptor heap from a resource descriptor heap, unlike in Vulkan everything can be placed in a single descriptor set allocated from a single descriptor heap.

I see. I'm coming from a Vulkan user's perspective. I'm probably ok to just separate out the samplers into their own dedicated array. Just so long as the N other resource types could all be put together.

@swoods-nv
Copy link
Collaborator

@dubiousconst282 Can you let us know if this is something you'll be working on in the near term? Thank you!

@swoods-nv swoods-nv added kind:enhancement a desirable new feature, option, or behavior goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang Needs reporter feedback Bugs awaiting more information from the reporter labels Jun 13, 2024
@dubiousconst282
Copy link
Author

Yes, I am working on a draft and will be ready to submit a PR shortly (hopefully before the end of the week).

@dubiousconst282 dubiousconst282 linked a pull request Jun 14, 2024 that will close this issue
@jkwak-work
Copy link
Collaborator

It appears that I misunderstood what was asked. Thanks for the link at the very beginning.
What Yong suggested now makes all sense to me.
I will review the draft tomorrow.

@bmillsNV bmillsNV removed the Needs reporter feedback Bugs awaiting more information from the reporter label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang kind:enhancement a desirable new feature, option, or behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants