-
Notifications
You must be signed in to change notification settings - Fork 17
Explicit Padding in CBuffers Proposal #311
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
Conversation
proposals/NNNN-explicit-padding.md
Outdated
The padding type will be defined as one of the following: | ||
|
||
- A first class LLVM type called `pad8`, which is equivalent but distinct from | ||
`i8`. This would need an RFC to the wider LLVM community and would need to be | ||
useful in other contexts (such as ABI-mandated padding). | ||
- A well-known named type `%pad8`, defined as a named struct containing a | ||
single `i8`. This is the simplest option but requires backends that are | ||
interested in this type to participate in a secret handshake. | ||
- Target types such as `target("dx.pad8")` and `target("spirv.pad8")`. This is | ||
somewhat awkward because the type isn't really tied to a target, but target | ||
types need to be. Targets that don't need to differentiate between padding | ||
and actual members could simply use `i8`. | ||
|
||
> TODO: Choose one of these three options and move the others to the | ||
> "alternatives" section. |
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.
This is the main unanswered question that I want feedback on here. I'm leaning towards the simple well-known name approach for its simplicity, with the option of pushing for a first class type in the future if this proves useful otherwise. The downside, of course, is that if there were a name collision with some other type very bad things would happen.
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.
- If you use the target type, you can do something like we do for the
vk::SpirvType
where the size is a parameter. So the padding is always just one instance of the type, not an array. However, not a significant advantage. - The "well known name" solution has the problem that theoretically, the optimizer does not know the name is special, and might change it in some way. Let me know if there is something in the llvm-ir spec that would guarantee that it will not be changed.
- The
pad8
is a great idea if it is accepted by the llvm community. But it could take a while.
My thoughts are to do the target type for now. See if the LLVM community is interested in pad8
. If so, we can switch to it when it is added.
I do not have strong opinions on this, and I will not hold up this proposal if you do something different.
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've updated this to use a target type with a size parameter.
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.
There is one issue that I don't think has been addressed. That is type conversions. Consider this example: https://godbolt.org/z/xY4MshTr1.
struct S {
float f[4];
};
RWStructuredBuffer<S> sb;
S s;
[numthreads(1,1,1)]
void main() {
sb[0] = s;
}
From an HLSL perspective, the type stored in the structured buffer and the type stored in the cbuffer are the same type S
, so you can assign one to the other. With this proposal, the type in the cbuffer will now be different than the type in the structured buffer in the AST. We will have to have some type of conversion from one to the other.
proposals/NNNN-explicit-padding.md
Outdated
template <typename T, std::size_t N> struct CBufArray<T, N, false> { | ||
T Elems[N]; | ||
|
||
const T &operator[](std::size_t I) const { return Elems[I]; } |
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.
Base on the way this is written, if T
is a struct it will have the cbuffer layout, but the code that uses it might expect it to have the standard layout. I think you might get a type mismatch. You will have to have some way of doing a transition. Note that if you do the transition in this function, then you cannot return a reference.
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 think one of the complications you end up with if we try to represent this in the AST is that we need a type trait that produces the cbuffer layout types effectively recursively. Because you may have something like:
struct MyStruct {
float2 F;
int Arr[4];
int2 I;
};
cbuffer example {
MyStruct S;
float2 F;
MyStruct Arr[2];
int I;
};
The cbuffer layout struct is effectively:
struct __cbuffer_layout_example {
__cbuffer_layout_MyStruct;
float2;
CBufArray<__cbuffer_layout_MyStruct, 2>;
int;
}
I don't think this is impossible to deal with, but if we do represent this in the AST we'll also need to think about how we handle conversions. __cbuffer_layout
types will need to implicitly convert to their non-cbuffer types during any lvalue->rvalue conversion.
We may also need to massage the diagnostics for the inverse case because while we won't need to support converting a value of a non-cbuffer layout type to the cbuffer type since cbuffers are read-only, we really won't want the diagnostics to refer to the cbuffer types directly.
proposals/NNNN-explicit-padding.md
Outdated
template <typename T, std::size_t N> struct CBufArray<T, N, false> { | ||
T Elems[N]; | ||
|
||
const T &operator[](std::size_t I) const { return Elems[I]; } |
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 think one of the complications you end up with if we try to represent this in the AST is that we need a type trait that produces the cbuffer layout types effectively recursively. Because you may have something like:
struct MyStruct {
float2 F;
int Arr[4];
int2 I;
};
cbuffer example {
MyStruct S;
float2 F;
MyStruct Arr[2];
int I;
};
The cbuffer layout struct is effectively:
struct __cbuffer_layout_example {
__cbuffer_layout_MyStruct;
float2;
CBufArray<__cbuffer_layout_MyStruct, 2>;
int;
}
I don't think this is impossible to deal with, but if we do represent this in the AST we'll also need to think about how we handle conversions. __cbuffer_layout
types will need to implicitly convert to their non-cbuffer types during any lvalue->rvalue conversion.
We may also need to massage the diagnostics for the inverse case because while we won't need to support converting a value of a non-cbuffer layout type to the cbuffer type since cbuffers are read-only, we really won't want the diagnostics to refer to the cbuffer types directly.
types. | ||
|
||
[llvm-project/wg-hlsl#171]: https://github.com/llvm/wg-hlsl/pull/171 | ||
|
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 think the one other alternative to consider is a hybrid, where we create the layout types in the AST, but don't actually have the cbuffer members be of the layout types. That would avoid needing to have special casting behavior for cbuffer types. We could insert the "conversion" code late in CodeGen based of the address space of the pointer being loaded.
I'm not sure if this actually simplifies things or not.
DXC does a bunch of things in CodeGen that shouldn't be done there because it adds data type conversions that actually change values, but in this case these conversions aren't really "type" conversions as much as layout conversions, so I feel less icky about doing them in CodeGen and not fully representing them in the AST.
Curious for thoughts.
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.
My uninform thoughts are that it could work. It is worth checking out. Somewhere in clang, we have to handle conversions. I just don't know the best place.
Also note that conversion will have to be done in such a way that they do not cause too much code, and they can be optimized aways. See a recent issue we fixed for SPIR-V: microsoft/DirectXShaderCompiler#7493. Their code copies the entirety of a large cbuffer to return it by value. The expectation is that the optimizer is able copy propagate everything and only load the values that are actually used.
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.
We can handle the copies directly in clang codegen in EmitAggregateCopy - this already has some special handling for things like ObjC types, so it doesn't seem wrong to do. This is how things are currently working in llvm/llvm-project#156919 and I've updated the proposal.
16dcb22
to
7d64060
Compare
proposals/NNNN-explicit-padding.md
Outdated
We introduce an explicit padding type for HLSL, and construct cbuffer arrays | ||
and structs that are annotated with `packoffset` or `vk::offset` using this | ||
type to unambiguously lay out these objects. |
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.
We introduce an explicit padding type for HLSL, and construct cbuffer arrays | |
and structs that are annotated with `packoffset` or `vk::offset` using this | |
type to unambiguously lay out these objects. | |
We introduce an explicit padding type for HLSL, and construct cbuffer arrays, | |
structs, and cbuffer elements that are annotated with `packoffset` or `vk::offset` using this | |
type to unambiguously lay out these objects. |
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.
Structs in cbuffer must also start on a 16-byte boundary and we need to insert padding before if needed: https://godbolt.org/z/YEjc7TEY9
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.
Reworded this to make it clearer.
may attempt to come up with a first-class type in LLVM for these purposes in | ||
the future. | ||
|
||
### Structs with annotations |
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 would probably move this section after "### Arrays in a cbuffer" because this is way less common than ararys in cbuffer.
proposals/NNNN-explicit-padding.md
Outdated
[CBuffer Padded arrays at the HLSL-level] for details. | ||
|
||
[CBuffer Padded arrays at the HLSL-level]: #cbuffer-padded-arrays-at-the-hlsl-level | ||
|
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.
Add section "### Struct in a cbuffer" (here or before the array section) describing that structs in cbuffer must also start on a 16-byte boundary and we need to insert padding before if as needed.
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 found a separate section for this more confusing so I instead reworked this a bit to talk about scalars, vectors, structs, and arrays explicitly.
``` | ||
|
||
[__cblayout]: https://github.com/llvm/wg-hlsl/blob/4570a9cfc5c4b1e5bc0b773a6fb7b22014ac6d3b/proposals/0016-constant-buffers.md#lowering-constant-buffer-resources-to-llvm-ir "Lowering Constant Buffer Resources to LLVM IR" | ||
|
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.
Add example of cbuffer with struct with padding before it.
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.
Thanks. This LGTM. I'll approve once I get a chance to try out the prototype. Hopefully this week.
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.
LGTM!
2faae94
to
2e5c481
Compare
Closes #308