-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add a proposal for how to explicitly specify struct layouts #171
Open
bogner
wants to merge
5
commits into
llvm:main
Choose a base branch
from
bogner:2025-02-11-explicit-layout-structs
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
e785c3c
Add a proposal for how to explicitly specify struct layouts
bogner 29fd8d3
Update proposals/NNNN-explicit-layout-struct.md
bogner f442a3c
Elaborate on open questions and remove mixed packoffset examples
bogner ddfd3d6
Add struct layout in clang question
bogner 53c33d4
alternative solutions
bogner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,203 @@ | ||
<!-- {% raw %} --> | ||
|
||
# Types with explicit layouts in DXIL and SPIR-V | ||
|
||
* Proposal: [NNNN](NNNN-explicit-layout-struct.md) | ||
* Author(s): [bogner](https://github.com/bogner) | ||
* Status: **Design In Progress** | ||
|
||
## Introduction | ||
|
||
This introduces the `dx.Layout` and `spirv.Layout` target extension types, | ||
which can be used to represent HLSL structures that need explicit layout | ||
information in LLVM IR. | ||
|
||
## Motivation | ||
|
||
Some HLSL types have layout that isn't practical to derive from the module's | ||
DataLayout. This includes all kinds of `cbuffer`, but especially those that use | ||
`packoffset`, and also applies to structs that use the vulkan `[[vk::offset()]] | ||
extension and possibly objects with specific alignment specified on subobjects. | ||
|
||
We need to be able to represent these types in IR so that we can generate | ||
correct code in the backends. | ||
|
||
## Proposed solution | ||
|
||
We should implement a target type that includes a struct type, total size, and | ||
offsets for each member of the struct. This type can then be used in other | ||
target types, such as CBuffer or StructuredBuffer definitions, or even in other | ||
layout types. We need target types for DirectX and for SPIR-V, but the types | ||
can and should mirror each other. | ||
|
||
``` | ||
target("[dx|spirv].Layout", %struct_type, <size>, [offset...]) | ||
``` | ||
|
||
### Examples | ||
|
||
In the examples below we generally use "dx.Layout", since the "spirv.Layout" | ||
variants would be identical. | ||
|
||
While these aren't necessarily needed for types that don't have explicit layout | ||
rules, some examples of "standard" layout objects represented this way are | ||
helpful: | ||
|
||
```llvm | ||
; struct simple1 { | ||
; float x; | ||
; float y; | ||
; }; | ||
%__hlsl_simple1 = { i32, i32 } | ||
target("dx.Layout", %__hlsl_simple1, 8, 0, 4) | ||
|
||
; struct simple2 { | ||
; float3 x; | ||
; float y; | ||
; }; | ||
%__hlsl_simple2 = { <3 x float>, float } | ||
target("dx.Layout", %__hlsl_simple2, 16, 0, 12) | ||
|
||
; struct nested { | ||
; simple2 s2; | ||
; simple1 s1; | ||
; }; | ||
%__hlsl_nested = type { target("dx.Layout", %__hlsl_simple2, 16, 0, 12), | ||
target("dx.Layout", %__hlsl_simple1, 8, 0, 4) } | ||
target("dx.Layout", %__layout_nested, 24, 0, 16) | ||
``` | ||
|
||
Objects whose layout differs in cbuffers than in structs: | ||
|
||
```llvm | ||
; struct array_struct { | ||
; float x[4]; | ||
; float y; | ||
; }; | ||
%__hlsl_array_struct = type { [4 x float], float } | ||
target("dx.Layout", %__hlsl_array_struct, 20, 0, 16) | ||
|
||
; cbuffer array_cbuf1 { | ||
; float x[4]; | ||
; float y; | ||
; }; | ||
target("dx.Layout", %__hlsl_array_struct, 56, 0, 52) | ||
|
||
; cbuffer array_cbuf2 { | ||
; array_struct s; | ||
; }; | ||
target("dx.Layout", %__hlsl_array_struct, 56, 0, 52) | ||
|
||
; struct nested2 { | ||
; simple1 s1; | ||
; simple2 s2; | ||
; }; | ||
%__hlsl_nested2 = type { target("dx.Layout", %__hlsl_simple1, 8, 0, 4), | ||
target("dx.Layout", %__hlsl_simple2, 16, 0, 12) } | ||
target("dx.Layout", %__hlsl_nested2, 24, 0, 8) | ||
|
||
; cbuffer nested_cbuf { | ||
; simple1 s1; | ||
; simple2 s2; | ||
; }; | ||
target("dx.Layout", %__hlsl_nested2, 32, 0, 16) | ||
``` | ||
|
||
Simple usage of packoffset: | ||
|
||
```llvm | ||
; cbuffer packoffset1 { | ||
; float x : packoffset(c1.x); | ||
; float y : packoffset(c2.y); | ||
; }; | ||
target("dx.Layout", { i32, i32 }, 40, 16, 36) | ||
``` | ||
|
||
packoffset that reorders fields: | ||
> note: This does not currently work in DXC targeting SPIR-V. | ||
|
||
```llvm | ||
; cbuffer packoffset1 { | ||
; float x : packoffset(c2.y); | ||
; float y : packoffset(c1.x); | ||
; }; | ||
target("dx.Layout", { i32, i32 }, 40, 36, 16) | ||
``` | ||
|
||
Use of `[[vk::offset()]]`: | ||
|
||
```llvm | ||
; struct vkoffset1 { | ||
; float2 a; | ||
; [[vk::offset(8)]] float2 b; | ||
; } | ||
%__hlsl_vkoffset1 = { <2 x float>, <2 x float> } | ||
target("spirv.Layout", %__hlsl_vkoffset1, 12, 0, 8) | ||
|
||
; struct complex { | ||
; float r; | ||
; float i; | ||
; }; | ||
; struct vkoffset2 { | ||
; float2 a; | ||
; [[vk::offset(8) complex b; | ||
; } | ||
%__hlsl_vkoffset2 = { <2 x float>, { float, float } } | ||
target("spirv.Layout", %__hlsl_vkoffset1, 16, 0, 8) | ||
``` | ||
|
||
## Open questions | ||
Keenuts marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### Arrays in CBuffers | ||
|
||
Should we also add a `target("dx.CBufArray", <type>, <size>)` type, rather than | ||
having the CBuffer logic need to be aware of special array element spacing | ||
rules? | ||
|
||
#### Decaying to non-target types | ||
|
||
Operations like `resource.getpointer` can expose us to the contained type of a | ||
resource, but should that give us an object of the underlying struct or the | ||
target type? For the former, we would lose the layout, which is problematic. | ||
For the latter, we need to talk about GEPs. | ||
|
||
If we can have pointers of the target type, we'd either need to teach GEP to | ||
handle these, which is a fairly wide-reaching change, or we would need a set of | ||
GEP-like intrinsics. | ||
|
||
This will need to be resolved in order to use these in StructuredBuffer and for | ||
most real use-cases of `vk::offset`. | ||
|
||
#### Copying in and out of the layout | ||
|
||
How do we convert from the Layout types to types without the offsets? We'll | ||
probably need an intrinsic that does a logical copy from the target type to a | ||
compatible structure. See https://godbolt.org/z/rh5dvd3E7 for an example. | ||
|
||
#### Struct layouts in Clang | ||
|
||
Clang uses [ASTContext::getRecordLayout] to get the offset of a member when | ||
needed. This proposal means that we will not update this class to be aware of | ||
`vk::offset` and `packedoffset`. The goal of this proposal is to avoid adding | ||
explicit padding members in the llvm:StructType as is done when handling | ||
`alignof` in C++. | ||
|
||
[ASTContext::getRecordLayout]: https://github.com/llvm/llvm-project/blob/aa9e519b2423/clang/lib/AST/RecordLayoutBuilder.cpp#L3321 | ||
|
||
If we update `ASTContext::getRecordLayout`, then the explicit padding will be | ||
added to the llvm::StructType. If we do not update it, then we will have to | ||
consider every spot in Clang that calls it to see if we need to special case | ||
the HLSL layout. | ||
|
||
Currently known issue are: `offsetof` and `sizeof`. There is a lot more that | ||
needs to be looked into in Clang. `ASTRecordLayout` is passed around to a lot | ||
of functions, without the original type. | ||
|
||
## Acknowledgments | ||
|
||
s-perron marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another open issue. How will member functions work? This is related to how the |
||
This proposal is expanded from comments in [llvm/wg-hlsl#94] and follow up | ||
conversations. | ||
|
||
[llvm/wg-hlsl#94]: https://github.com/llvm/wg-hlsl/pull/94 | ||
|
||
<!-- {% endraw %} --> |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 there are other open question. How and when do you convert from the
spirv.Layout
ordx.Layout
types to the type without the offset, and how could it interact with optimizations. Consider a structured buffer access like https://godbolt.org/z/z49rEWe58. I'm guessing this would change by replacing%struct.T
withtarget("dx.Layout", 16, 0, 8)
in thedx.RawBuffer
type.But we still have the question of what should the type of the store. How should the GEP look?
At first the store becomes:
Note that the GEPs currently act on the struct type. We cannot change the GEPs to use the target extension type, because they are not allowed in GEPs. We could leave it implicit in some ways, but then the optimizer will assume it knows the layout and optimize accordingly:
Note that the opimizer modified the GEPs assuming it knows the offsets, even though it does not. How will you stop the optimizer from making assumptions about the layout of the struct, since we are representing it differently than llvm-ir usually expects?
This is less of a problem for cbuffers because we expect all access to the cbuffer to be through an intrinsic. There are a few options for structrued buffers:
I have a couple ideas on how to fix this, but only one that seems reasonable. Add an intrinsic that does a GEP of the
dx.Layout
type. This should hide everything from the optimizer.We might also need an intrinsic that will do a
memcpy
between a type with a layout and a type that does not. Consider this example: https://godbolt.org/z/rh5dvd3E7. Thememcpy
copies the buffer contents to a variable forFoo
which expects the struct without the layout information.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
memcpy
type intrinsic could correspond to OpCopyLogical in SPIR-V.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 could also expand the
memcpy
to copy the individual member one at a time, if we want to expose more to the optimizer. If we use an intrinsic, copy propagation will not work well.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.
Would it be hard to allow target types into a GEP instruction? Because I feel this would be the best option vs adding another intrinsic:
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.
Adding target type handling to the GEP instruction sounds nicer than needing a parallel set of operations, but that's definitely a larger change to LLVM. I'll add some notes to the open questions to capture these ideas - we'll need to answer this in some satisfying way in order to handle vk::offset properly.
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.
just to be pedantic,
memcpy
is astd::bit_cast
/OpBitCast
equivalent, theOpCopyLogical
is more of astd::copy
- a magical member by member logical copyY'all already know all that, its just that if the meaning
memcpy
starts getting overloaded like that it will lead to horrible head-scratching for outside/new contributors.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 we try to use a target type in the existing GEP instruction, I believe we will have many places we will have to add special cases. Any code that tries to optimize a GEP will have have a special case for the target extension type, even if it does nothing with it. That defeats the purpose of using an opaque type.
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 added some text to try to capture the questions here.
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.
It looks like I indeed missed an important aspect (Thanks DevSH)
If we use the target type in the GEP, then do a
load
/store
, we are saying we do amemcpy
. And if the layout is different, that would be wrong.Maybe in such case we should emit an intrinsic to have carry the semantic of the
OpCopyLogical
, but not amemcpy
?(This doesn't solve the issue of GEP being allowed to use target types still)