Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
Push constant nested struct alignment/padding issue
We have the following set of structures that we are using for push constants:

struct StructA
{
    uint64_t   valA;  // this is technically a reference to a buffer in our shader but I don't think that matters for this discussion
    uint          valB;
    uint          valC;
    uint          valD;
};

struct StructB
{
    StructA     aValues;
    uint           valE;
    uint           valF;
};

I think that struct A will have an alignment of 8 bytes and size of 24.  The Vulkan spec points to section 7.6.22 "Standard Uniform Block Layout" of the OpenGL specification which says:

"If the member is a structure, the base alignment of the structure is N, where N is the largest base alignment value of any of its members, and rounded up to the base alignment of a vec4. The individual members of this substructure are then assigned offsets by 
 applying this set of rules recursively, where the base offset of the first member of the sub-structure is equal to the aligned offset of the structure. The structure may have padding at the end; the base offset of the member following the sub-structure is rounded up to
the next multiple of the base alignment of the structure."

By those rules the base offset of the member following the sub structure (valE) is rounded up to the next multiple of the base alignment of the structure (8).  However, spirv-dis shows that the offset of valE in this scenario is 20 when I believe it should be 24.

This is easy to work-around by inserting explicit padding or making valD a uint64_t so not a show-stopper but it did take some time to diagnose and seems like an error WRT the spec.



Discussion / hints:
Sorry, I should add that this is with Vulkan SDK 1.3.224.0.  
It appears the validator is only validating this for uniform blocks. That does appear to be incorrect according to the Vulkan specification. So that's one thing to fix. This requirement only applies if scalar layouts are not enabled. Which generator are you using? Some (e.g. DXC) use assume scalar block layout.

Fix patch:
diff --git a/source/val/validate_decorations.cpp b/source/val/validate_decorations.cpp
index ef9753da45..f9c843521f 100644
--- a/source/val/validate_decorations.cpp
+++ b/source/val/validate_decorations.cpp
@@ -633,10 +633,10 @@ spv_result_t checkLayout(uint32_t struct_id, const char* storage_class_str,
       }
     }
     nextValidOffset = offset + size;
-    if (!scalar_block_layout && blockRules &&
+    if (!scalar_block_layout &&
         (spv::Op::OpTypeArray == opcode || spv::Op::OpTypeStruct == opcode)) {
-      // Uniform block rules don't permit anything in the padding of a struct
-      // or array.
+      // Non-scalar block layout rules don't permit anything in the padding of
+      // a struct or array.
       nextValidOffset = align(nextValidOffset, alignment);
     }
   }


Test patch:
diff --git a/test/val/val_decoration_test.cpp b/test/val/val_decoration_test.cpp
index ff62f4b0cd..04d373a75f 100644
--- a/test/val/val_decoration_test.cpp
+++ b/test/val/val_decoration_test.cpp
@@ -4520,6 +4520,48 @@ TEST_F(ValidateDecorations, ArrayArrayRowMajorMatrixNextMemberOverlapsBad) {
 TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackGood) {
   // Original GLSL
 
+  // #version 450
+  // layout (set=0,binding=0) buffer S {
+  //   uvec3 arr[2][2]; // first 3 elements are 16 bytes, last is 12
+  //   uint i;  // Can't have offset 60 = 3x16 + 12
+  // } B;
+  // void main() {}
+
+  std::string spirv = R"(
+               OpCapability Shader
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Vertex %1 "main"
+               OpDecorate %_arr_v3uint_uint_2 ArrayStride 16
+               OpDecorate %_arr__arr_v3uint_uint_2_uint_2 ArrayStride 32
+               OpMemberDecorate %_struct_4 0 Offset 0
+               OpMemberDecorate %_struct_4 1 Offset 64
+               OpDecorate %_struct_4 BufferBlock
+               OpDecorate %5 DescriptorSet 0
+               OpDecorate %5 Binding 0
+       %void = OpTypeVoid
+          %7 = OpTypeFunction %void
+       %uint = OpTypeInt 32 0
+     %v3uint = OpTypeVector %uint 3
+     %uint_2 = OpConstant %uint 2
+%_arr_v3uint_uint_2 = OpTypeArray %v3uint %uint_2
+%_arr__arr_v3uint_uint_2_uint_2 = OpTypeArray %_arr_v3uint_uint_2 %uint_2
+  %_struct_4 = OpTypeStruct %_arr__arr_v3uint_uint_2_uint_2 %uint
+%_ptr_Uniform__struct_4 = OpTypePointer Uniform %_struct_4
+          %5 = OpVariable %_ptr_Uniform__struct_4 Uniform
+          %1 = OpFunction %void None %7
+         %12 = OpLabel
+               OpReturn
+               OpFunctionEnd
+  )";
+
+  CompileSuccessfully(spirv);
+  EXPECT_EQ(SPV_SUCCESS,
+            ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
+}
+
+TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackGoodScalar) {
+  // Original GLSL
+
   // #version 450
   // layout (set=0,binding=0) buffer S {
   //   uvec3 arr[2][2]; // first 3 elements are 16 bytes, last is 12
@@ -4554,6 +4596,7 @@ TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackGood) {
                OpFunctionEnd
   )";
 
+  options_->scalar_block_layout = true;
   CompileSuccessfully(spirv);
   EXPECT_EQ(SPV_SUCCESS,
             ValidateAndRetrieveValidationState(SPV_ENV_VULKAN_1_0));
@@ -4569,7 +4612,7 @@ TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackBad) {
                OpDecorate %_arr_v3uint_uint_2 ArrayStride 16
                OpDecorate %_arr__arr_v3uint_uint_2_uint_2 ArrayStride 32
                OpMemberDecorate %_struct_4 0 Offset 0
-               OpMemberDecorate %_struct_4 1 Offset 56
+               OpMemberDecorate %_struct_4 1 Offset 60
                OpDecorate %_struct_4 BufferBlock
                OpDecorate %5 DescriptorSet 0
                OpDecorate %5 Binding 0
@@ -4595,8 +4638,8 @@ TEST_F(ValidateDecorations, StorageBufferArraySizeCalculationPackBad) {
   EXPECT_THAT(getDiagnosticString(),
               HasSubstr("Structure id 4 decorated as BufferBlock for variable "
                         "in Uniform storage class must follow standard storage "
-                        "buffer layout rules: member 1 at offset 56 overlaps "
-                        "previous member ending at offset 59"));
+                        "buffer layout rules: member 1 at offset 60 overlaps "
+                        "previous member ending at offset 63"));
 }
 
 TEST_F(ValidateDecorations, UniformBufferArraySizeCalculationPackGood) {