[SPIR-V] Feature request: use "uniform" syntax to access SBT record data in ray tracing workflows

[natevm]

Hello,
I'm working on a framework leveraging Vulkan Ray Tracing + HLSL for my shader language. In using HLSL because it supports multiple simultaneous entry points while GLSL does not.

In Vulkan, I can pass buffer addresses through the SBT using vkGetBufferDeviceAddress and the GL_EXT_buffer_reference2 extension, which looks something like this:

layout(buffer_reference, buffer_reference_align=8, scalar) buffer VertexBuffer {
    vec3 v[];
};

layout(buffer_reference, buffer_reference_align=8, scalar) buffer IndexBuffer {
    uvec3 i[];
};

layout(shaderRecordEXT, std430) buffer SBT {
    VertexBuffer verts;
    IndexBuffer indices;
};

and in OptiX, I simply pass a pointer into the SBT record's data section. However, in DXR I run into issues...

The following produces invalid SPIR-V with the DXC compiler:

[shader("raygeneration")]
void simpleRayGen(
  uniform StructuredBuffer<float> test
)
{
  printf("Hello from the raygen program! %f\n", test);
}

fatal error : generated SPIR-V is invalid: In Logical addressing, variables may not allocate a pointer type
%param_var_test = OpVariable %_ptr_Function__ptr_StorageBuffer_type_StructuredBuffer_float Function
note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible

Interestingly, the following code compiles correctly:

[shader("raygeneration")]
void simpleRayGen(
  uniform Buffer<float> test
)
{
  printf("Hello from the raygen program! %f\n", test);
}

But then this causes an access violation in the compiler without any useful error messages

struct Test{
  float test;
};
[shader("raygeneration")]
void simpleRayGen(
  uniform Buffer<Test> testing
)
{
  printf("Hello from the raygen program! %f\n", testing.Load(0).test);
}

So, I'm confused about how to achieve the same layout(buffer_reference, ...) thing I would do in GLSL but with HLSL... Any clarifications here would be really useful

Update: the above issue is actually sorta mixing two different issues together. The primary issue is that the "uniform" syntax I'm using above doesn't appear to work in the HLSL -> Vulkan path. The second issue is the strange and undefined behavior of putting StructuredBuffers into the SBT record data section.

[natevm]

I wonder if the RawBufferLoad functionality described here could work as a workaround?

https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/SPIR-V.rst#rawbufferload

Update, yes, this seems doable using vk::RayBufferLoad.

[natevm]

Update, seems that the "uniform" syntax I'm used to with slang doesn't work with HLSL RT -> Vulkan. Is this a bug?

The [[vk::shader_record_ext]] with a ConstantBuffer appears to work.

[tex3d]

Generally, in DXC, uniform parameters to entry points are not supported this way. This was a change from FXC. However, DXC doesn't provide good error reporting for this case in the front end.

See #3377 for issue related to error reporting for uniform resource arguments on entry points.

[pow2clk]

Hi @natevm!

I'm afraid that SPIRV issues are handled by our friends at Google. Tagging @vettoreldaniele for visibility.

Meanwhile, I'm afraid there are a number of problems with the code you've provided. Some of them might be revealed if you try compiling these with a DXIL target just to get the errors. These resources can't be parameters and you'll need an index to access a Structured buffer. Unfortunately, you will have to leave out the printfs for a DXIL target, but it can vet the rest of your code. Of course no source should crash the compiler and we should have useful error messages, but this might help you find your way to a working shader in the meantime.

[vettoreldaniele]

Hey @natevm, As suggested by @pow2clk, it would be useful to start with a shader that works with DXIL to exclude the case where the shader is ill-formed (in that case the SPIR-V backend cannot guarantee to produce correct code).

[natevm]

I was largely confused between the differences with slang-shader's flavor of HLSL and DXC.

I thought in the case of multiple entry points that only one ConstantBuffer with [[vk::shader_record_ext]] could be used. But it appears that I can actually use multiple such buffers with different template types, all in the same program:

[[vk::shader_record_ext]]
ConstantBuffer<RayGenData> raygenSBTData;
[shader("raygeneration")]
void simpleRayGen() {
  printf("Hello from the raygen program! %d \n", raygenSBTData.abc); // here
}

[[vk::shader_record_ext]]
ConstantBuffer<MissProgData> missSBTData;
[shader("miss")]
void simpleMissProg(inout Payload tmp) {
    printf("Hello from the raygen program! %d \n", missSBTData.xyz); // and then here
}

So, just to clarify, this is more a syntax sugar proposal. The above is a little messy syntax wise, and seeing as the "uniform" keyword isn't used for RT entry points, I think this would look cleaner:

[shader("raygeneration")]
void simpleRayGen(uniform RayGenData raygenSBTData) {
  printf("Hello from the raygen program! %d \n", raygenSBTData.abc); // here
}

[shader("miss")]
void simpleMissProg(inout Payload tmp, uniform MissProgData missSBTData) {
    printf("Hello from the raygen program! %d \n", missSBTData.xyz); // and then here
}

I'm not sure what that would look like for DXIL though, since [[vk::shader_record_ext]] is vulkan specific.

[sudonatalie]

Although the Vulkan attributes can be a bit cumbersome, supporting a syntax change like this is outside the scope of feature work we're currently considering for targeting the SPIR-V backend of DXC, so I'm going to close this issue.

[natevm]

I’m not sure you (or many DXR devs) really understand the nuances of shader binding table records.

There are significant issues I’m running up against with the current HLSL syntax, and I’d argue my syntax above resolves a substantial issue.

The current HLSL syntax is unsafe, as the underlying pointer to the shader binding table record is illegally byte cast by HLSL if multiple global entries of [[vk::shader_record_ext]] exist in the same code. (I can access two globally defined records pointing to the same memory in the same underlying kernel) This is almost always the case when multiple ray tracing programs exist in the same .HLSL file.

[natevm]

I suppose I’m just confused by HLSL standards. In Vulkan Buffer Pointer discussions, you argue that byte casting is not legal in HLSL. But then the same isn’t true for shader binding table records? Perhaps the solution here would just be to confirm that only one [[vk::shader_record_ext]] is actually accessed by any HLSL program?

I suppose this is also an issue with GLSL…

really, the issue is that shader binding table records are an OptiX 7 construct, which you access through a CUDA pointer. Their implementation in GLSL and HLSL were hacked in without consideration for type safety standards of either of these languages. I have grievances with that.

[s-perron]

So, just to clarify, this is more a syntax sugar proposal.

This was a change that happened in Oct of last year, but if you want to make changes to HLSL you need to open an issue in HLSL specs. In the DXC repository, we are concerned with translating HLSL into SPIR-V or DXIL using the existing specification of HLSL. If you want that change, bring it up in https://github.com/microsoft/hlsl-specs.

My main concern would be that the syntax is not obviously Vulkan specific. You would have to define what happens in DXIL if you want that syntax. It seems like the direction DXC is going is to not use that type of syntax. See #4566 (comment).

Of course no source should crash the compiler and we should have useful error messages,

It is bad that we have incorrect syntax, but are not giving a good error message. There is another issue open for that. See #4566 (comment) again.

But it appears that I can actually use multiple such buffers with different template types, all in the same program

That is correct, you can have multiple buffers marked with the vk::shader_record_ext attribute, but only one can be used per entry point. It is not a great design because it makes it too easy to get things wrong, but it is what we have.

Correct me if I am wrong, but there is nothing that needs to be addressed in the DXC repo that is not covered by another issue, so this issue should remain closed.