[SPIR-V] Scalar layout does not follow C-like structure layout rules

[Dolkar]

Description
With -fvk-use-scalar-layout we can get a C-like structure layout, enabling easy code sharing between device and host. But the reality is that it only almost works, leading to hard-to-debug problems when you encounter such an edge case. Namely:

struct Foo {
    uint64_t a;
    int b;
};

struct Bar {
    Foo foo;
    int c;
};

Leads to the following layout with -fvk-use-scalar-layout:

OpMemberDecorate %Foo 0 Offset 0
OpMemberDecorate %Foo 1 Offset 8
OpMemberDecorate %Bar 0 Offset 0
OpMemberDecorate %Bar 1 Offset 12

Even though in C/C++ offsetof(Bar, c) == 16 instead of 12. This is because in C the size of types are required to be a multiple of their alignment, but no such requirement exists in the scalar alignment rules in Vulkan.

In Slang this was addressed by adding a separate layout toggled with -fvk-use-c-layout. But I would suggest going one step further and changing this behavior directly in the existing layout specifier. The VK_EXT_scalar_block_layout extension states:

This extension enables C-like structure layout for SPIR-V blocks. It modifies the alignment rules for uniform buffers, storage buffers and push constants, allowing non-scalar types to be aligned solely based on the size of their components, without additional requirements.

If the intention of scalar layout was to enable C-like structure layout, then I would consider this deviation from it as a bug. It's not a bug in the extension spec itself, it just defines a set of layout restrictions that are permissive enough to allow reproducing C-like layout. But it's a mistake, in my opinion, for a shader compiler to pack members as closely as the rules allow instead of leveraging the extension to follow a C-like structure layout like intended and expected.

I would not be opposed to adding a layout specifier separate to the scalar one, but then I would question the usefulness of the scalar layout compared to it. The difference is probably not large enough for scalar layout to still be useful in memory constrained situations. It being a breaking change could be a concern, but I would be surprised if anyone is relying on this unfortunate behavior rather than being bitten by it.

The fix itself seems to be a one-liner, should nobody object:

--- a/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp
+++ b/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp
@@ -170,6 +170,7 @@ std::pair<uint32_t, uint32_t> AlignmentSizeCalculator::getAlignmentAndSize(
   if (rule == SpirvLayoutRule::Scalar) {
     // A structure has a scalar alignment equal to the largest scalar
     // alignment of any of its members in VK_EXT_scalar_block_layout.
+    structSize = roundToPow2(structSize, maxAlignment);
     return {maxAlignment, structSize};
   }

Related: #6947

Steps to Reproduce
https://godbolt.org/z/qYzobEK3T

Environment

• DXC version: trunk
• Host Operating System: Windows 11

[s-perron]

I believe you are correct that the scalar layout was suppose to match the C/C++ layout. However it was not correctly implement. The problem with changing it now is that it could break many people's existing code.

We can look into adding an option similar to slang.

[s-perron]

@Dolkar I don't know when we will get to it. But it seems like you have an idea how to implement it. We would welcome the patch.

[s-perron]

This is a better example: https://godbolt.org/z/bEWTrW9j6.

EDITED: I keep going back and forth on this.

We'll need to check the best way to fix this example as well. It might be to modify the size of the struct as well.