-
Notifications
You must be signed in to change notification settings - Fork 17
[SPIR-V] Add proposal for vk::push_constant #360
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
base: main
Are you sure you want to change the base?
Conversation
Adds proposal to implement VK push constant support in Clang.
| - There can only be one ``[[vk::push_constant]]`` attribute in the shader. | ||
| - There can be no VLA in the struct type (or a nested struct type). | ||
|
|
||
| DXC allows any storage class to be attached to a push constant. I believe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Please list which storage classes are currently accepted by DXC, that will not be supported in Clang.
We should update DXC to match 202X?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keywords DXC accepts are:
- extern
- static
- groupshared
- shared
- uniform
Seems like [const] [extern] uniform is the good class, but [const] auto/none should be allowed as the attributes implies const extern uniform already.
| This being a Vulkan specific feature, the attribute is ignored when targeting | ||
| DXIL. This means the push constant global becomes part of the cbuffer when | ||
| building for DXIL. | ||
|
|
||
| The attribute being ignored, the variable belong to the cbuffer, meaning no | ||
| additional handling is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't fully understand all of the requirements for HLSL attributes in SEMA. For the resource work @hekota mentioned that for the clang based tools like the rewriter, the attributes will have to be in the AST.
Can you be more explicit about where and how it will be ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AST will have the HLSLVkPushConstant attribute since parsing will see it. But in Sema, this attribute will be ignored, meaning we won't change the current behavior which is to put a global in the global cbuffer.
| Codegen will be left almost as-is as we simply load a variable in a different | ||
| address space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more explicit about what the layout of the struct should be? I believe it is the same as a RWStructuredBuffer.
The DXC SPIR-V documentation does not mention it.
The rules in the Vulkan documentation give the same layout rules for push constant as storage buffers.
Adds proposal to implement VK push constant support in Clang.
E2E implementation draft: llvm/llvm-project#166793