-
Notifications
You must be signed in to change notification settings - Fork 713
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
WIP: [SPIR-V] Add support for vk::buffer_ref attribute #5089
Conversation
This following text describes the new Buffer Reference feature in HLSL. Motivation - GLSL Capability The following is an example of the GLSL buffer reference feature:
Here is the SPIR-V for this example:
Motivation - Previous HLSL Capability The previous attempt to support a similar capability in HLSL was the offering of the function vk::RawBufferLoad. Here is an example of its use to implement the same functionality as the GLSL shader above:
Here is the SPIR-V for this example:
There are several problems with this low-level approach. One is that tools such as spirv-reflect do not have the context to report on which members of The New Functionality A new attribute vk:BufferRef has been introduced into HLSL. Here is an example of how this new attribute can be used to implement the original GLSL shader:
The SPIR-V for this shader is virtually identical to that of the GLSL. It therefore avoids the problems of vk::RawBufferLoad. Note that the shader source is also nearly identical to the GLSL. Just as in the GLSL, the reference is created by annotating the referenced struct. The buffer alignment (here 16) is also similarly expressed and can be used by the compiler to automatically account for it in the struct's SPIR-V offsets. The typedef is required here since dxc does not currently support applying attributes to types. However it does allow attributes to be attached to declarations such as typedefs. Linked List Example The spec for the GLSL extension GL_EXT_buffer_reference gives the following example of using buffer reference to create a linked list of identical buffers:
vk::BufferRef can be used to program a similar shader in HLSL:
Notice the capability to create, set and use local variables of the reference type such as b in the GLSL example and g_t in the HLSL example. An Additional vk::BufferRef Form Currently dxc supports an additional, slightly different way to use vk::BufferRef. This is where the attribute is applied directly to struct members and local variables of struct type. Here is the first HLSL example using the alternate form:
And here is the linked list example:
The alternate form supports exactly the same functionality as that supported by the typedef form. The reason the alternate form is supported is that it is the first way that I implemented this feature. During that work I realized how to do the typedef form, which at the time seemed to me superior. The reason I haven't ripped out the alternate form is that I became unsure which form was most advantageous, so I left them both in, at least for the moment. I don't think it is necessarily bad to have two ways to express this. Some may prefer one way, some the other. Even so, the way I have it coded at the moment, I could rip out either form by just editing a few lines. |
Acknowledgement: This feature has been designed, implemented and tested under review and assistance of @danginsburg |
Hi Greg. The 1st argument I can understand but for the 2nd I'm pretty sure that RawBufferLoad can accept a struct directly. The following compiles:
IMHO RawBufferLoad (with the addition of alignment Buffer reference introduces complexity because you have to cast to and from uint64_t in order to calculate addresses of complex data. At the same time types between HLSL and C++ will not look the same. C++ types will have a uint64_t (?) and HLSL will have a buffer ref. At the same time what is the underlying type of the buffer ref? Is it uint64_t or uint2? Packing rules are different depending on the type. |
Please see #4986 and #4289 for some discussion of the issues with why that doesn't currently work. Also, in case it wasn't clear from the original description, a goal here beyond spirv-reflect was about introspection and debugability in RenderDoc. If you have a GLSL shader using buffer_reference RenderDoc is able to perform introspection on the type to provide the ability to see the data in the buffer viewer and to be able to get the data type in the shader debugger. Part of the goal of this change from a usability point-of-view was to enable that same ability from shaders generated with HLSL via DXC. |
One other possible advantage of vk::buffer_ref is its syntactic similarity to the GLSL functionality, which aids in understanding and porting to the HLSL. |
@godlikepanos The SPIR-V for both approaches is above. To my eyes, it is the RawBufferLoad approach that is doing the BitCasting with uint64_t or whatever. You can see in the SPIR-V that the vk::buffer_ref approach is able to keep to a strictly logical addressing scheme, using pointers to types. The underlying type is whatever is specified by the API. Another apparent advantage to vk::buffer_ref is that "." sequences with multiple/linked/nested references are able to stick with the original syntax, aiding in compactness and readability. |
Thanks for the explanation to both. It sounds good. I guess you will also allow casting from uint64_t to a buffer ref and the opposite. Is that correct? |
vk::buffer_ref(X) when prepended to a struct typedef indicates the typedef is actually a reference to the struct, similar to C++ references, that is, syntactically treated like a struct, but implemented with a pointer with byte alignment X. vk::buffer_ref can alternately be prepended to member and local variable declarations of struct type creating similar semantics.
I don't think there is a way to do the casting within a shader. Certainly with Vulkan a uint64_t address in a buffer can be used as a buffer reference by a shader which declares the buffer with a buffer reference aliasing the uint64_t. |
Hi Greg! I can see the benefit of having this kind of feature vs the [[vk::buffer_ref(16)]] typedef struct Globals_s
{
float4 g_vSomeConstantA;
float4 g_vTestFloat4;
float4 g_vSomeConstantB;
} Globals_t; This looks weird to me, as the attribute seems to apply to the whole struct, a bit like I would be in favor of your second variant, when the field/variable gets this reference attribute. struct MyStruct
{
[[vk::buffer_ref(16)]] MyStruct next;
}; Now this means declaring a variable means typing typedef MyStruct MyStructValue, [[vk::buffer_ref(16)]] MyStructRef; Or the attribute after the type, like clang attributes typedef unsigned *aligned_1k __attribute__((align_value(1024))), *aligned_16 __attribute__((align_value(16))); (Would be cool to have the |
Hey Nathan! Thanks for looking at this. Yes, the syntax for the typedef form can be visually a little awkward. A couple things about that:
For example, from the first example:
The attribute only modifies the typedef and Globals_t; Globals_s is not affected by the attribute and can continue to be used in the remainder of the program without any buffer_ref semantics affecting it. Visually this might not be intuitive, but I think logically this is intuitive as well as desirable. If the user is concerned about the appearance that Globals_s is affected by the attribute, they can use this syntax instead:
Visually, it seems fairly intuitive here that Globals_s is not affected by the attribute. |
I'm not sure, as I haven't tried, but is it that disruptive to the codebase to add a compiler attribute?
Sure, but if the language can enforce something readable, that's better 😊 Given: struct A;
struct A
{
int int_value;
[[vk::buffer_ref(16)]] struct A ptr_to_a;
[[vk::buffer_ref(16)]] int ptr_to_int;
};
struct TestPushConstant_t
{
[[vk::buffer_ref(16)]] struct A root;
};
[[vk::push_constant]] TestPushConstant_t push_constants; Played around with the current implementation (I know it's a "draft" and not robust yet), but what about those case: [numthreads(1,1,1)]
void main() {
// ptr_to_a is a ref to a struct A.
[[vk::buffer_ref(16)]] struct A ptr_to_a = push_constants.root.ptr_to_a;
// ptr_to_int is a ref to an int? And like in C++, the pointer would be deref of moved depending on the result type?
[[vk::buffer_ref(16)]] int ptr_to_int = push_constants.root.ptr_to_int;
int int_value = push_constants.root.ptr_to_int;
} void foo([[vk::buffer_ref(16)]] struct A ref) { }
[numthreads(1,1,1)]
void main() {
[[vk::buffer_ref(16)]] struct A ptr_to_a = push_constants.root.ptr_to_a;
foo(ptr_to_a);
} Shall we handle this exactly as a C++ ref? Btw, this compiles (just fails validation because vk::buffer_ref doesn't like ints:
Generated SPIR-V here don't seem correct: Same for this (non-aligned store) [[vk::buffer_ref(16)]] struct A ptr_to_a = push_constants.root.ptr_to_a;
ptr_to_a.int_value = ptr_to_a.int_value + 1; And this blows up my stack (cycle in IsHLSLCopyableAnnotatableRecord/IsHLSLNumericOrAggregateOfNumericType) [[vk::buffer_ref(16)]] struct A ptr_to_a = push_constants.root.ptr_to_int;
ptr_to_a.int_value = ptr_to_a.int_value + 1; Same for this (clang::spirv::SpirvEmitter::reconstructValue/ [[vk::buffer_ref(16)]] struct A get_at_depth(int depth) {
return push_constants.root.ptr_to_a;
}
[numthreads(1,1,1)]
void main() {
get_at_depth(10);
} (Overall, a lot of cases causes infinite loops around the codebase, as we don't expect those) |
I think this isn't the right venue for this discussion. This change has significant language implications, and we should probably discuss this as a more core language change rather than just an attribute. We have a process for designing and developing language changes here. One big thought in my mind is should this be an attribute, or should this be an actual reference with appropriate address space annotations. |
This probably also fixes #4983, #4924 #4620 and maybe even #5095. By the way, current P.S. my two cents, I'd probably want to use this as using ptr_to_A = [[vk::buffer_ref(16)]] A; or something similar.
As @ahmadi-erfan might say "We need this today, not in a few months/years". If keeping this around is an issue I suggest making a forward-compatibility macro // will change to something like `using REFERENCE_ALIAS = TYPE& global` when HLSL introduces actual pointers and pointer scopes
#define HLSL_DECLARE_REF_TYPE(REFERENCE_ALIAS ,TYPE) [[vk::buffer_ref(alignof(TYPE))]] typedef TYPE REFERENCE_ALIAS P.S. Made an edit after realizing GLSL BDA is 99% a C++ reference and not a pointer despite the casting with |
@greg-lunarg a serious question, given this #version 450
#extension GL_EXT_buffer_reference : require
layout(buffer_reference, std430, buffer_reference_align = 16) readonly buffer Globals
{
vec4 g_vSomeConstantA;
vec4 g_vTestFloat4;
vec4 g_vSomeConstantB;
};
layout( set=0, binding=0) buffer
{
Globals g_globals;
} SSBO; How does one resolve the ambiguity between SSBO.g_globals = 0xdeadbeefBADC0FFEull; and SSBO.g_globals = {vec4(1.f),vec4(2.f),vec4(3.f)}; i.e. changing the pointer vs changing the pointee? Is it transparent like a reference, and I need another "view" where the buffer reference variable is a EDIT: Answered my own question, its a reference not a pointer, being passed around by value, so it can't be changed without a second view. |
Ok reading https://github.com/KhronosGroup/GLSL/blob/master/extensions/ext/GLSL_EXT_buffer_reference.txt it would seem that this is more of a Its super weird that GLSL does syntax abuse that lets you do I guess I really do need a "uint64_t" view onto the place where the hmm, in C++ I wouldn't be able to change the reference either, because |
This change would be great. Currently RWStructuredBuffer arrays are not supported in hlsl. Thismakes bindless buffers in hlsl with vulkan very painfull as the only good option is to make all buffers ByteAddressBuffers. |
We are not rushing fundamental language changes. Putting in changes that have bypassed language design review has caused an enormous amount of technical debt in HLSL and DXC. Nobody is going to die if we put this through a proper process so that this is designed, reviewed and implemented to our quality standards. |
I implemented a |
This. We want Vulkan features added to HLSL to be HLSL features, not just ports of GLSL. This is a factor in why we added the requirment that all language changes go through our design and review process. Language features are also incredibly difficult to change once introduced, and they cause significant disruption when we do change them. As a result, we need to be extra careful about what we introduce. We very much understand the utility and desire for this feature. There is no question that we want to add something to the language that addresses this use case. Whatever we add needs to be consistent with the design philosophy of HLSL. |
I had suggested the exact same thing in private conversations with LunarG. |
We are working on a proposal that will be brought to the hlsl specs. |
Sorry everyone for going dark. As @s-perron said, we have been working on a doc to submit to https://github.com/microsoft/hlsl-specs/. We will cross reference on this PR. We will answer all of your questions in that space. We never meant to go around a language review. We thought it would happen here. We were not aware of the process. Talk to you all soon on the other side :) |
I have submitted a PR proposing an HLSL language change at microsoft/hlsl-specs#37. At the insistence of MS and Google, the feature has changed from an attribute to a first class pointer type. We have also added the capability for function args and returns to take this type. We have also added the ability to convert a buffer pointer to a uint64_t to compare with zero for linked list traversal. |
Vk::RawBufferLoad/RawBufferStore cannot generate compatible code MultiIndirectDraw , vulkan & DX12 There will be a common implementation method
|
One thing I don’t understand with this proposed syntax is how pointers to pointers would work… like, if I have a buffer of addresses (maybe a list of emissive triangle meshes I want to sample in a path tracer), how would that work here? I can do buffer2 = vk::RawBufferLoad<uint64_t>(buffer1, idx1); But with attributes I have no idea how I’d accomplish that. Also, I agree I tend to dislike pulling GLSL syntax into HLSL. The proposed attribute API seems very un-CUDA-like, which I think is the opposite direction we should be going. However, the proposed API in microsoft/hlsl-specs#37 is much nicer IMO, and I’m much more on board with a first-class pointer type. |
Since we're iterating on the design for this change over here, and this change as designed and implemented is not going to be accepted I'm going to close this PR. Please direct further feedback to hlsl-specs on the current review or by filing an issue. |
Hey have you considered using just the pointer syntax? That's what those are, right? Pointers? Just use the uint64_t, but type them instead of a raw uint64_t value (which btw is akin to using a void* and then casting it to whatever is the real type?) |
@danielmaciel, the proposal for this is evolving over on the specification. Introducing pointer syntax is a significantly more complicated and invasive change to the language, and requires a new language version. The proposal as we're evolving it doesn't require changing the language, instead it is a backwards-compatible feature that can be exposed in older language modes since it has no new grammatical elements. This has the benefit of being able to allow users to adopt this feature without forcing them to update their code to a new language version (or making them wait until the new language version is ready). We are also working to design it in a way that will not introduce complicated syntax that would be removed when HLSL does gain pointers in the future, so that we have a good transition path from this interface to pointers when they become available. |
Is there a plan for when exactly the feature should be implemented? It's quite frustrating to still not be able to use the buffer pointers decently with HLSL, and GLSL is no real alternative. |
It's been mostly implemented in slang (shader-slang/slang#3814). I'm not sure what the plan is for implementing it in DXC, but Valve/LunarG are not planning to implement it. I've moved our engine over to using slang. |
Good point, I also move my stuff over to GLSL or slang if this is the case, HLSL just feels slow in adopting spirv support in my opinion. |
Hi! Owners on the SPIR-V side are either on leave, retired, or not working on compilers anymore. The proposal has been accepted, so seems like we should be able to add this to HLSL/DXC. (#6489) |
The engineering time is the main bottleneck IIUC yep. Slang has a decent number of engineers working hard on the SPIR-V backend. For anyone looking to use slang and SPIR-V, I recommend using the —emit-spirv-directly flag, since that enables more “hot off the press” features like buffer pointers. For more info on buffer pointers themselves, here's the current docs: https://shader-slang.com/slang/user-guide/convenience-features.html#pointers As well as the current test we're using, which has some more examples: https://github.com/shader-slang/slang/blob/master/tests/spirv/pointer.slang |
vk::buffer_ref(X) when prepended to a struct typedef indicates the typedef is actually a reference to the struct, similar to C++ references, that is, syntactically treated like a struct, but implemented with a pointer with byte alignment X. vk::buffer_ref can alternately be prepended to member and local variable declarations of struct type with similar semantics.
@Keenuts @cassiebeckley This is a WIP as I still need to add tests, but the implementation is otherwise complete, so if you could start reviewing the code, that would be great.
I will presently be adding additional high-level description of the feature to this PR.