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

[Feature Request] Allow resource overlap within same space #353

Open
Agrael1 opened this issue Nov 17, 2024 · 3 comments
Open

[Feature Request] Allow resource overlap within same space #353

Agrael1 opened this issue Nov 17, 2024 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@Agrael1
Copy link

Agrael1 commented Nov 17, 2024

Is your feature request related to a problem? Please describe.
Hello, I'm trying to create a compressed universal roots for the bindless pipelines. That would be analogous to Vulkan's descriptor indexing feature.
This allows for indexing into typed array of descriptors with several binding tables. Assuming I have 5 tables with different resources.
For Vulkan the code

[[vk::binding(0, 2)]] ConstantBuffer<Camera> camera_arr[] : register(b0, space2);
[[vk::binding(0, 2)]] ConstantBuffer<PointLightCBuf> lights_arr[] : register(b0, space2);

does not cause any problem. There are 2 types of CBs in the example, and since union or reinterpret_cast can't be used in HLSL, this solution is a viable option.
Yet for DXIL this is problematic. Even if there is no register overlap I'd still have to bind those arrays to different spaces and create another binding to the root signature which points to exactly the same space, which results in fat root signatures for no reason.

Describe the solution you'd like
I'd like there to be an option to allow overlap same type of registers within the same space if they are unbounded.

Describe alternatives you've considered
I have considered using SM 6.6 yet, it is not supported on my machine, and bridging ResourceDescriptorHeap and descriptor indexing in vulkan is really hard, since it breaks the linear boundary of bindings, like differentiating buffer from texture, that may lead to errors and big buffers of indices.

Additional context
The solution may be a compiler flag. The binding model won't suffer from that feature, since bindings have the same type. That would not require any change to debugging infrastructure as well.

@Agrael1 Agrael1 added enhancement New feature or request needs-triage labels Nov 17, 2024
@Agrael1
Copy link
Author

Agrael1 commented Nov 17, 2024

#ifdef DXIL
#define DX_OFFSET(reg) (-reg)
#else // SPRIV
#define DX_OFFSET(reg) 0
#endif // DXIL


struct PSInput {
    float4 position : SV_POSITION;
};

struct PushConstants {
    uint2 buffer; // current frame, global offset
};
[[vk::push_constant]] ConstantBuffer<PushConstants> pushConstants : register(b0);

struct OffsetX {
    float offset;
};

struct OffsetY {
    float offset;
};

// binding 0, space 1 is used for samplers
// binding 0, space 2 is used for constant buffers
[[vk::binding(0, 2)]] ConstantBuffer<OffsetX> offsetsx[1] : register(b0, space2);
[[vk::binding(0, 2)]] ConstantBuffer<OffsetY> offsetsy[1] : register(b1, space2); 
// Note: different register, for DX this has to be different register
// for Vulkan it can be the same register without offset.
// That means we have to subtract the descriptor offset in the shader for DX compilation.

PSInput main(float3 position : POSITION)
{
    PSInput result = (PSInput)0;

    float offsetx = offsetsx[pushConstants.buffer.x + DX_OFFSET(0)].offset;
    float offsety = offsetsy[pushConstants.buffer.y + pushConstants.buffer.x + DX_OFFSET(1)].offset;
    result.position = float4(position.x + offsetx, position.y + offsety, position.z, 1.0f);
    return result;
}

Interestingly this stuff works, which proves, that there is no difference in type of constant buffer or the number of elements in the array. Neither is there a boundary check.

@llvm-beanz
Copy link
Collaborator

Interestingly this stuff works, which proves, that there is no difference in type of constant buffer or the number of elements in the array. Neither is there a boundary check.

You misunderstand what is happening here. Here's a compiler explorer link.

You'll note that both cbuffer loads are hitting different buffers. It just happens that your GPU driver laid out the buffers next to each other, so when you negatively index one it seems to work.

@damyanp damyanp transferred this issue from microsoft/DirectXShaderCompiler Nov 19, 2024
@damyanp damyanp moved this to Triaged in HLSL Triage Nov 19, 2024
@damyanp damyanp added this to the Dormant milestone Nov 19, 2024
@Agrael1
Copy link
Author

Agrael1 commented Nov 19, 2024

It is not just driver. If you have unbounded array of cbuffers, they are laid in an array fashion. They are always laid out next to each other, otherwise bindless would not work. I bind an array of 4 buffers, which are 2 buffers of <OffsetX> and 2 of <OffsetY>.

If I could reinterpret one array to the other type it would not be an issue. Heaps have uniform strides, and they just store indices to the resources. So if I have the same type of resource, CBV/SRV/UAV or samplers of different types they would hit the correct buffer if I index them correctly. So the question is, why is it not allowed to declare 2 or more types of resources of the same type with bindless arrays?

These codes are equivalent in case of the same binding

[[vk::binding(0, 2)]] ConstantBuffer<Camera> camera_arr[] : register(b0, space2);
[[vk::binding(0, 2)]] ConstantBuffer<PointLightCBuf> lights_arr[] : register(b0, space2);
...
Camera c = camera_arr[0];
PointLightCBuf l = lights_arr[1];

==

[[vk::binding(0, 2)]] ConstantBuffer<Camera> camera_arr[1] : register(b0, space2);
[[vk::binding(0, 2)]] ConstantBuffer<PointLightCBuf> lights_arr[1] : register(b1, space2); // the same array binding as before
...
Camera c = camera_arr[0];
PointLightCBuf l = lights_arr[0];

Yet the first one is illegal, while being perfectly fine. That causes explosion in spaces, since for each type I introduce I need to do a binding range with 0 offset, which may force to create more meaningless root signatures, when there could have been just one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Triaged
Development

No branches or pull requests

3 participants