Explicit Padding in CBuffers Proposal

[bogner]

Closes #308

[bogner]

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.

[s-perron]

1. 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.
2. 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.
3. 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.

[s-perron]

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.

[s-perron]

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.

[llvm-beanz]

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.

[s-perron]

I'm trying to think about what could happen with SPIR-V if we do not identify the padding and remove it.

1. If we keep the i8 just for padding, we will needlessly require the Int8 capability. Support for that capability was not required until Vulkan 1.4.
2. It would be nice to be able to convert from the cbuffer layout to the structuredbuffer layout, or the type without a layout using OpCopyLogical. If we could not remove the padding, then OpCopyLogical would not work.

I think that 1 is essential, and 2 is a very nice to have.

[llvm-beanz]

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.

[llvm-beanz]

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.

[s-perron]

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.