From e785c3ce8c2494ec200d5ae8a649b4579d4e7a72 Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Tue, 11 Feb 2025 01:33:48 -0800 Subject: [PATCH 1/5] Add a proposal for how to explicitly specify struct layouts --- proposals/NNNN-explicit-layout-struct.md | 187 +++++++++++++++++++++++ 1 file changed, 187 insertions(+) create mode 100644 proposals/NNNN-explicit-layout-struct.md diff --git a/proposals/NNNN-explicit-layout-struct.md b/proposals/NNNN-explicit-layout-struct.md new file mode 100644 index 0000000..34d9706 --- /dev/null +++ b/proposals/NNNN-explicit-layout-struct.md @@ -0,0 +1,187 @@ + + +# 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, , [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 only specifies the first field: +> note: This emits a warning in DXC. Do we really want to support it? + +```llvm +; cbuffer packoffset1 { +; float x : packoffset(c1.x); +; float y; +; }; +target("dx.Layout", { i32, i32 }, 24, 16, 20) +``` + +packoffset that only specifies a later field: +> note: This behaves differently between DXIL and SPIR-V in DXC, and the DXIL +> behaviour is very surprising. Do we want to allow this? + +```llvm +; cbuffer packoffset1 { +; float x; +; float y : packoffset(c1.x); +; }; +target("dx.Layout", { i32, i32 }, 24, 20, 16) +target("spirv.Layout", { i32, i32 }, 20, 0, 16) +``` + +packoffset that reorders fields: +> note: This fails to compile for SPIR-V in DXC. Is this worth handling? + +```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 + +- Should we also add a `target("dx.CBufArray", , )` type, rather + than having the CBuffer logic need to be aware of special array element + spacing rules? +- Should reordering fields actually be allowed here? + +## Acknowledgments + +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 + + From 29fd8d37c469d000e43312a214045d44b4804d48 Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Tue, 18 Feb 2025 08:59:10 -0800 Subject: [PATCH 2/5] Update proposals/NNNN-explicit-layout-struct.md Co-authored-by: Steven Perron --- proposals/NNNN-explicit-layout-struct.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proposals/NNNN-explicit-layout-struct.md b/proposals/NNNN-explicit-layout-struct.md index 34d9706..6d605a1 100644 --- a/proposals/NNNN-explicit-layout-struct.md +++ b/proposals/NNNN-explicit-layout-struct.md @@ -153,7 +153,7 @@ Use of `[[vk::offset()]]`: ```llvm ; struct vkoffset1 { ; float2 a; -; [[vk::offset(8) float2 b; +; [[vk::offset(8)]] float2 b; ; } %__hlsl_vkoffset1 = { <2 x float>, <2 x float> } target("spirv.Layout", %__hlsl_vkoffset1, 12, 0, 8) From f442a3c7a27207a4dbfc76b4b2f87063df3f3d82 Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Tue, 18 Feb 2025 16:10:23 -0800 Subject: [PATCH 3/5] Elaborate on open questions and remove mixed packoffset examples --- proposals/NNNN-explicit-layout-struct.md | 55 +++++++++++------------- 1 file changed, 26 insertions(+), 29 deletions(-) diff --git a/proposals/NNNN-explicit-layout-struct.md b/proposals/NNNN-explicit-layout-struct.md index 6d605a1..d9d9fff 100644 --- a/proposals/NNNN-explicit-layout-struct.md +++ b/proposals/NNNN-explicit-layout-struct.md @@ -113,32 +113,8 @@ Simple usage of packoffset: target("dx.Layout", { i32, i32 }, 40, 16, 36) ``` -packoffset that only specifies the first field: -> note: This emits a warning in DXC. Do we really want to support it? - -```llvm -; cbuffer packoffset1 { -; float x : packoffset(c1.x); -; float y; -; }; -target("dx.Layout", { i32, i32 }, 24, 16, 20) -``` - -packoffset that only specifies a later field: -> note: This behaves differently between DXIL and SPIR-V in DXC, and the DXIL -> behaviour is very surprising. Do we want to allow this? - -```llvm -; cbuffer packoffset1 { -; float x; -; float y : packoffset(c1.x); -; }; -target("dx.Layout", { i32, i32 }, 24, 20, 16) -target("spirv.Layout", { i32, i32 }, 20, 0, 16) -``` - packoffset that reorders fields: -> note: This fails to compile for SPIR-V in DXC. Is this worth handling? +> note: This does not currently work in DXC targeting SPIR-V. ```llvm ; cbuffer packoffset1 { @@ -172,10 +148,31 @@ target("spirv.Layout", %__hlsl_vkoffset1, 16, 0, 8) ## Open questions -- Should we also add a `target("dx.CBufArray", , )` type, rather - than having the CBuffer logic need to be aware of special array element - spacing rules? -- Should reordering fields actually be allowed here? +#### Arrays in CBuffers + +Should we also add a `target("dx.CBufArray", , )` 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. ## Acknowledgments From ddfd3d6645c3242e14b1a2b29d6f3b139897c358 Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Wed, 19 Feb 2025 10:16:04 -0800 Subject: [PATCH 4/5] Add struct layout in clang question --- proposals/NNNN-explicit-layout-struct.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/proposals/NNNN-explicit-layout-struct.md b/proposals/NNNN-explicit-layout-struct.md index d9d9fff..e7e7e8d 100644 --- a/proposals/NNNN-explicit-layout-struct.md +++ b/proposals/NNNN-explicit-layout-struct.md @@ -174,6 +174,25 @@ 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 This proposal is expanded from comments in [llvm/wg-hlsl#94] and follow up From 53c33d412e5cae8d7e687e6355550d02d51367ce Mon Sep 17 00:00:00 2001 From: Justin Bogner Date: Mon, 24 Feb 2025 11:33:23 -0800 Subject: [PATCH 5/5] alternative solutions --- proposals/NNNN-explicit-layout-struct.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/proposals/NNNN-explicit-layout-struct.md b/proposals/NNNN-explicit-layout-struct.md index e7e7e8d..2f31343 100644 --- a/proposals/NNNN-explicit-layout-struct.md +++ b/proposals/NNNN-explicit-layout-struct.md @@ -193,6 +193,23 @@ 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. +## Alternative Solutions + +There are a few other approaches we might take here in order to get a handle on +the many open questions: + +- Target types that describe the bit layout (padding included), with + integer fields to describe the logical layout +- Target types that describe the bit layout with fields to describe padding +- Adding or modifying a first class structure type with layout information +- Adding explicit padding types to LLVM's type system +- Accessors to the layout types that give us a "physical layout" struct of the + object. + +It's clear that the layout type is sufficient for cbuffers, but it has some +problems when trying to use it more generically. We need to investigate further +if a combined solution for cbuffers, vk::offset, and alignas is feasible. + ## Acknowledgments This proposal is expanded from comments in [llvm/wg-hlsl#94] and follow up