Skip to content
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
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bogner
Copy link
Collaborator

@bogner bogner commented Feb 11, 2025

No description provided.

Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a problem with the general direction. However, I think we need to expand on how the type will affect the codegen and optimizations.

```

packoffset that reorders fields:
> note: This fails to compile for SPIR-V in DXC. Is this worth handling?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, I think it is intuitive for users to have the order of the members in the struct match their order in memory. We'd have to see what kind of usage there is.

On the other hand, this is not a fundamental problem for SPIR-V. In https://docs.vulkan.org/spec/latest/chapters/interfaces.html#interfaces-resources-layout, there is an explicit note:

The numeric order of Offset decorations does not need to follow member declaration order.

We would not have to jump through too many hoops to implement this. The deciding factor should be what is best for users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both DXC and FXC support this and it seems to be fairly well defined in the language. I think we'll want to treat the current situation as a bug in DXC's SPIR-V backend and support it in clang.

target("spirv.Layout", %__hlsl_vkoffset1, 16, 0, 8)
```

## Open questions
Copy link
Collaborator

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 or dx.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 with target("dx.Layout", 16, 0, 8) in the dx.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:

  %2 = getelementptr inbounds nuw %struct.T, ptr %1, i32 0, i32 0
  store <2 x float> zeroinitializer, ptr %2, align 8
  %3 = getelementptr inbounds nuw %struct.T, ptr %1, i32 0, i32 1
  store <2 x float> splat (float 1.000000e+00), ptr %3, align 8

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:

  store <2 x float> zeroinitializer, ptr %2, align 8
  %3 = getelementptr inbounds nuw i8, ptr %2, i32 8
  store <2 x float> splat (float 1.000000e+00), ptr %3, align 8

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. The memcpy copies the buffer contents to a variable for Foo which expects the struct without the layout information.

Copy link
Collaborator

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.

Copy link
Collaborator

@s-perron s-perron Feb 11, 2025

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.

Copy link
Collaborator

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:

  • we keep the GEP semantic, we are just saying "don't assume anything about the offset computation we do, let the backend handle it"

Copy link
Collaborator Author

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.

Copy link

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 a std::bit_cast/OpBitCast equivalent, the OpCopyLogical is more of a std::copy - a magical member by member logical copy

Y'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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

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 a memcpy. 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 a memcpy ?

(This doesn't solve the issue of GEP being allowed to use target types still)

@bogner bogner self-assigned this Feb 11, 2025
@devshgraphicsprogramming
Copy link

devshgraphicsprogramming commented Feb 18, 2025

My most important question is, for the [[vk::offset]] and packoffset will I be able to see those reflected and be constexpr in

struct vkoffset1
{
  float2 a;
  [[vk::offset(8)]] float2 b;
};
vkoffset1 var;

sizeof(decltype(var))
offsetof(decltype(var),b)

Finally, whats your plan for handling alignments/alignof ?

@bogner
Copy link
Collaborator Author

bogner commented Feb 18, 2025

My most important question is, for the [[vk::offset]] and packoffset will I be able to see those reflected and be constexpr in

struct vkoffset1
{
  float2 a;
  [[vk::offset(8)]] float2 b;
};
vkoffset1 var;

sizeof(decltype(var))
offsetof(decltype(var),b)

I believe sizeof and offsetof work off of the AST, so the target types shouldn't be relevant for them. I would expect both of these to Do The Right Thing(TM) when we're done though.

Finally, whats your plan for handling alignments/alignof ?

These constructs could obviously also be used there if desired, at the risk of diverging or needing to modify how we handle alignment in non-resource contexts.

@s-perron
Copy link
Collaborator

I believe sizeof and offsetof work off of the AST, so the target types shouldn't be relevant for them. I would expect both of these to Do The Right Thing(TM) when we're done though.

I don't think this is so simple. Many things are integrated. The code that evalutates offsetof uses ASTContext::getASTRecordLayout to get the offset for each member. If we update that function to know about vk::offset and packoffset, then codegen will use that when creating the llvm::StructType. This will add the explicit padding to the structs that we wanted to avoid. In general, we will have to examine all uses of ASTContext::getASTRecordLayout, and see if we need a special case for the our attributes. Is that something that will be acceptable to the clang community?

of functions, without the original type.

## Acknowledgments

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 pointer will be handled in member functions.

Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a section for alternative solutions. The more I see, the more I wonder if it will be easier to add explicit padding to the struct, and then find a way to filter out the padding.

@bogner
Copy link
Collaborator Author

bogner commented Feb 20, 2025

We should also add a section for alternative solutions. The more I see, the more I wonder if it will be easier to add explicit padding to the struct, and then find a way to filter out the padding.

I think we're coming to the conclusion here that this solution isn't quite viable for the general problem of vk::offset and alignas, and that we're going to need to do more work to generalize this. Short term, I think we should probably go this route to get constant buffers into a working state, but we need to spend more time on what to do for the non cbuffer cases as they add enough complexity that this and other solutions aren't quite sufficient.

I'll add a few things to an alternative solutions part of this document tomorrow. I have a few ideas here, but none of them are slam dunks:

  • 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
  • more or less the solution described here, but with intrinsics to get a "physical layout" struct out of the object

bogner added a commit to bogner/llvm-project that referenced this pull request Feb 25, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
bogner added a commit to llvm/llvm-project that referenced this pull request Feb 25, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
bogner added a commit to llvm/llvm-project that referenced this pull request Feb 25, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Feb 25, 2025
…128697)

This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 27, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
jph-13 pushed a commit to jph-13/llvm-project that referenced this pull request Feb 28, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
YutongZhuu pushed a commit to YutongZhuu/llvm-project that referenced this pull request Mar 8, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
YutongZhuu pushed a commit to YutongZhuu/llvm-project that referenced this pull request Mar 8, 2025
This adds support cbuffers based on llvm/wg-hlsl#171 - the type argument
of the CBuffer TargetExtType is either a `dx.Layout` type which reports
its own size, or it's a normal type and we can simply refer to
DataLayout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

4 participants