[spirv] Fix Vulkan layout struct member alignment error (#1418)

For VK_HKR_relaxed_block_layout, a vector inside structs can have
its offset as a multiple of its base element type's alignment.
But the vector's alignment is unaffected, esp. when used for
calculating the alignment of the whole enclosing struct.

Also enables SPIRV-Tools validation on block layouts.

external/SPIRV-Tools

@@ -1 +1 @@
-Subproject commit 9ecbcf5fc87db00d3d6275522c735b5667007647
+Subproject commit 4db9c789ffb69193ccd5d2dfad1d7bcab5e00f23

tools/clang/lib/SPIRV/SPIRVEmitter.cpp

@@ -230,7 +230,8 @@ bool spirvToolsOptimize(spv_target_env env, std::vector<uint32_t> *module,
 }
 
 bool spirvToolsValidate(spv_target_env env, std::vector<uint32_t> *module,
-                        std::string *messages, bool relaxLogicalPointer) {
+                        std::string *messages, bool relaxLogicalPointer,
+                        bool glLayout, bool dxLayout) {
   spvtools::SpirvTools tools(env);
 
   tools.SetMessageConsumer(
@@ -240,8 +241,11 @@ bool spirvToolsValidate(spv_target_env env, std::vector<uint32_t> *module,
 
   spvtools::ValidatorOptions options;
   options.SetRelaxLogicalPointer(relaxLogicalPointer);
-  // TODO: Fix the following to enable validating layout.
-  options.SetRelaxBlockLayout(true);
+  // GL: strict block layout rules
+  // VK: relaxed block layout rules
+  // DX: Skip block layout rules
+  options.SetRelaxBlockLayout(!glLayout && !dxLayout);
+  options.SetSkipBlockLayout(dxLayout);
 
   return tools.Validate(module->data(), module->size(), options);
 }
@@ -694,8 +698,9 @@ void SPIRVEmitter::HandleTranslationUnit(ASTContext &context) {
   // Validate the generated SPIR-V code
   if (!spirvOptions.disableValidation) {
     std::string messages;
-    if (!spirvToolsValidate(targetEnv, &m, &messages,
-                            declIdMapper.requiresLegalization())) {
+    if (!spirvToolsValidate(
+            targetEnv, &m, &messages, declIdMapper.requiresLegalization(),
+            spirvOptions.useGlLayout, spirvOptions.useDxLayout)) {
       emitFatalError("generated SPIR-V is invalid: %0", {}) << messages;
       emitNote("please file a bug report on "
                "https://github.com/Microsoft/DirectXShaderCompiler/issues "

tools/clang/lib/SPIRV/TypeTranslator.cpp

@@ -1306,7 +1306,7 @@ llvm::SmallVector<const Decoration *, 4> TypeTranslator::getLayoutDecorations(
     if (rule == LayoutRule::RelaxedGLSLStd140 ||
         rule == LayoutRule::RelaxedGLSLStd430 ||
         rule == LayoutRule::FxcCTBuffer) {
-      alignUsingHLSLRelaxedLayout(fieldType, memberSize, &memberAlignment,
+      alignUsingHLSLRelaxedLayout(fieldType, memberSize, memberAlignment,
                                   &offset);
     } else {
       offset = roundToPow2(offset, memberAlignment);
@@ -1678,7 +1678,7 @@ bool TypeTranslator::canFitIntoOneRegister(QualType structType,
 
 void TypeTranslator::alignUsingHLSLRelaxedLayout(QualType fieldType,
                                                  uint32_t fieldSize,
-                                                 uint32_t *fieldAlignment,
+                                                 uint32_t fieldAlignment,
                                                  uint32_t *currentOffset) {
   QualType vecElemType = {};
   const bool fieldIsVecType = isVectorType(fieldType, &vecElemType);
@@ -1691,17 +1691,17 @@ void TypeTranslator::alignUsingHLSLRelaxedLayout(QualType fieldType,
     std::tie(scalarAlignment, std::ignore) =
         getAlignmentAndSize(vecElemType, LayoutRule::Void, nullptr);
     if (scalarAlignment <= 4)
-      *fieldAlignment = scalarAlignment;
+      fieldAlignment = scalarAlignment;
   }
 
-  *currentOffset = roundToPow2(*currentOffset, *fieldAlignment);
+  *currentOffset = roundToPow2(*currentOffset, fieldAlignment);
 
   // Adjust according to HLSL relaxed layout rules.
   // Bump to 4-component vector alignment if there is a bad straddle
   if (fieldIsVecType &&
       improperStraddle(fieldType, fieldSize, *currentOffset)) {
-    *fieldAlignment = kStd140Vec4Alignment;
-    *currentOffset = roundToPow2(*currentOffset, *fieldAlignment);
+    fieldAlignment = kStd140Vec4Alignment;
+    *currentOffset = roundToPow2(*currentOffset, fieldAlignment);
   }
 }
 
@@ -1886,7 +1886,7 @@ TypeTranslator::getAlignmentAndSize(QualType type, LayoutRule rule,
           rule == LayoutRule::RelaxedGLSLStd430 ||
           rule == LayoutRule::FxcCTBuffer) {
         alignUsingHLSLRelaxedLayout(field->getType(), memberSize,
-                                    &memberAlignment, &structSize);
+                                    memberAlignment, &structSize);
       } else {
         structSize = roundToPow2(structSize, memberAlignment);
       }

tools/clang/lib/SPIRV/TypeTranslator.h

@@ -309,7 +309,7 @@ private:
   /// fieldSize and fieldAlignment are the original size and alignment
   /// calculated without considering the HLSL vector relaxed rule.
   void alignUsingHLSLRelaxedLayout(QualType fieldType, uint32_t fieldSize,
-                                   uint32_t *fieldAlignment,
+                                   uint32_t fieldAlignment,
                                    uint32_t *currentOffset);
 
 public:

tools/clang/test/CodeGenSPIRV/type.cbuffer.hlsl

@@ -42,5 +42,6 @@ cbuffer AnotherCBuffer : register(b2) {
 // CHECK: %MyCbuffer = OpVariable %_ptr_Uniform_type_MyCbuffer Uniform
 // CHECK: %AnotherCBuffer = OpVariable %_ptr_Uniform_type_AnotherCBuffer Uniform
 
-void main() {
-}
+float  main() : A {
+  return t[0] + m[0];
+}

tools/clang/test/CodeGenSPIRV/type.struct.hlsl

@@ -52,7 +52,7 @@ struct T {
   S z;
 };
 
-void main() {
+float4 main() : A {
   N n;
   S s;
   T t;
@@ -66,4 +66,6 @@ void main() {
 
 // CHECK: %r1 = OpVariable %_ptr_Function_R Function
   R r1;
+
+  return x1.a + x2.a;
 }

tools/clang/test/CodeGenSPIRV/vk.layout.struct.relaxed.hlsl

@@ -0,0 +1,48 @@
+// Run: %dxc -T vs_6_0 -E main
+
+struct S {
+    float4 f1;
+    float3 f2;
+};
+
+
+
+
+// CHECK: OpMemberDecorate %type_MyCbuffer 0 Offset 0
+// CHECK: OpMemberDecorate %type_MyCbuffer 1 Offset 16
+// CHECK: OpMemberDecorate %type_MyCbuffer 2 Offset 48
+// CHECK: OpMemberDecorate %type_MyCbuffer 3 Offset 52
+// CHECK: OpMemberDecorate %type_MyCbuffer 4 Offset 64
+// CHECK: OpMemberDecorate %type_MyCbuffer 5 Offset 128
+
+cbuffer MyCbuffer : register(b1) {
+    bool     CB_a;
+    S        CB_s;
+    int      CB_b;
+    uint2    CB_c;
+    float3x4 CB_d;
+    float    CB_t[4];
+};
+
+
+
+// CHECK: OpMemberDecorate %type_MyTbuffer 0 Offset 0
+// CHECK: OpMemberDecorate %type_MyTbuffer 1 Offset 16
+// CHECK: OpMemberDecorate %type_MyTbuffer 2 Offset 48
+// CHECK: OpMemberDecorate %type_MyTbuffer 3 Offset 52
+// CHECK: OpMemberDecorate %type_MyTbuffer 4 Offset 64
+// CHECK: OpMemberDecorate %type_MyTbuffer 5 Offset 128
+
+tbuffer MyTbuffer : register(t1) {
+    bool     TB_a;
+    S        TB_s;
+    int      TB_b;
+    uint2    TB_c;
+    float3x4 TB_d;
+    float    TB_t[4];
+};
+
+
+float4 main() : A {
+  return CB_t[0] + TB_t[0];
+}

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -1273,34 +1273,34 @@ TEST_F(FileTest, SpirvInterpolationError) {
 }
 
 TEST_F(FileTest, SpirvLegalizationOpaqueStruct) {
-  runFileTest("spirv.legal.opaque-struct.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.opaque-struct.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferUsage) {
-  runFileTest("spirv.legal.sbuffer.usage.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.sbuffer.usage.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferMethods) {
-  runFileTest("spirv.legal.sbuffer.methods.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.sbuffer.methods.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferCounter) {
-  runFileTest("spirv.legal.sbuffer.counter.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.sbuffer.counter.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferCounterInStruct) {
   // Tests using struct/class having associated counters
-  runFileTest("spirv.legal.sbuffer.counter.struct.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.sbuffer.counter.struct.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferCounterInMethod) {
   // Tests using methods whose enclosing struct/class having associated counters
-  runFileTest("spirv.legal.sbuffer.counter.method.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.sbuffer.counter.method.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferInStruct) {
-  runFileTest("spirv.legal.sbuffer.struct.hlsl", Expect::Success,
-              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
+  setRelaxLogicalPointer();
+  runFileTest("spirv.legal.sbuffer.struct.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationConstantBuffer) {
   runFileTest("spirv.legal.cbuffer.hlsl");
@@ -1414,12 +1414,15 @@ TEST_F(FileTest, VulkanLayoutCBufferMatrixZpc) {
   runFileTest("vk.layout.cbuffer.zpc.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutCBufferStd140) {
+  setGlLayout();
   runFileTest("vk.layout.cbuffer.std140.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutCBufferNestedStd140) {
+  setGlLayout();
   runFileTest("vk.layout.cbuffer.nested.std140.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutCBufferNestedEmptyStd140) {
+  setGlLayout();
   runFileTest("vk.layout.cbuffer.nested.empty.std140.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutCBufferBoolean) {
@@ -1429,27 +1432,35 @@ TEST_F(FileTest, VulkanLayoutRWStructuredBufferBoolean) {
   runFileTest("vk.layout.rwstructuredbuffer.boolean.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutSBufferStd430) {
+  setGlLayout();
   runFileTest("vk.layout.sbuffer.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutSBufferNestedStd430) {
+  setGlLayout();
   runFileTest("vk.layout.sbuffer.nested.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutAppendSBufferStd430) {
+  setGlLayout();
   runFileTest("vk.layout.asbuffer.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutConsumeSBufferStd430) {
+  setGlLayout();
   runFileTest("vk.layout.csbuffer.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutTBufferStd430) {
+  setGlLayout();
   runFileTest("vk.layout.tbuffer.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutTextureBufferStd430) {
+  setGlLayout();
   runFileTest("vk.layout.texture-buffer.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayout64BitTypesStd430) {
+  setGlLayout();
   runFileTest("vk.layout.64bit-types.std430.hlsl");
 }
 TEST_F(FileTest, VulkanLayout64BitTypesStd140) {
+  setGlLayout();
   runFileTest("vk.layout.64bit-types.std140.hlsl");
 }
 TEST_F(FileTest, VulkanLayout16BitTypesPushConstant) {
@@ -1469,13 +1480,19 @@ TEST_F(FileTest, VulkanLayoutVectorRelaxedLayout) {
   // causing improper straddle
   runFileTest("vk.layout.vector.relaxed.hlsl");
 }
+TEST_F(FileTest, VulkanLayoutStructRelaxedLayout) {
+  // Checks VK_KHR_relaxed_block_layout on struct types
+  runFileTest("vk.layout.struct.relaxed.hlsl");
+}
 
 TEST_F(FileTest, VulkanLayoutVkOffsetAttr) {
   // Checks the behavior of [[vk::offset]]
+  setDxLayout();
   runFileTest("vk.layout.attr.offset.hlsl");
 }
 
 TEST_F(FileTest, VulkanLayoutPushConstantStd430) {
+  setGlLayout();
   runFileTest("vk.layout.push-constant.std430.hlsl");
 }
 
@@ -1488,10 +1505,12 @@ TEST_F(FileTest, VulkanLayoutCBufferPackOffsetError) {
 
 TEST_F(FileTest, VulkanLayoutFxcRulesSBuffer) {
   // structured buffers with fxc layout rules
+  setDxLayout();
   runFileTest("vk.layout.sbuffer.fxc.hlsl");
 }
 TEST_F(FileTest, VulkanLayoutFxcRulesCBuffer) {
   // cbuffer/tbuffer/ConstantBuffer/TextureBuffer with fxc layout rules
+  setDxLayout();
   runFileTest("vk.layout.cbuffer.fxc.hlsl");
 }
 

tools/clang/unittests/SPIRV/FileTestFixture.cpp

@@ -61,7 +61,7 @@ bool FileTest::parseInputFile() {
 }
 
 void FileTest::runFileTest(llvm::StringRef filename, Expect expect,
-                           bool runValidation, bool relaxLogicalPointer) {
+                           bool runValidation) {
   if (relaxLogicalPointer)
     assert(runValidation);
 
@@ -128,8 +128,8 @@ void FileTest::runFileTest(llvm::StringRef filename, Expect expect,
 
   // Run SPIR-V validation for successful compilations
   if (runValidation && expect != Expect::Failure) {
-    EXPECT_TRUE(utils::validateSpirvBinary(targetEnv, generatedBinary,
-                                           relaxLogicalPointer));
+    EXPECT_TRUE(utils::validateSpirvBinary(
+        targetEnv, generatedBinary, relaxLogicalPointer, glLayout, dxLayout));
   }
 }
 

tools/clang/unittests/SPIRV/FileTestFixture.h

@@ -26,13 +26,18 @@ public:
     Failure, // Failure (with errors) - check error message
   };
 
-  FileTest() : targetEnv(SPV_ENV_VULKAN_1_0) {}
+  FileTest()
+      : targetEnv(SPV_ENV_VULKAN_1_0), relaxLogicalPointer(false),
+        glLayout(false), dxLayout(false) {}
 
   void useVulkan1p1() { targetEnv = SPV_ENV_VULKAN_1_1; }
+  void setRelaxLogicalPointer() { relaxLogicalPointer = true; }
+  void setGlLayout() { glLayout = true; }
+  void setDxLayout() { dxLayout = true; }
 
   /// \brief Runs a File Test! (See class description for more info)
   void runFileTest(llvm::StringRef path, Expect expect = Expect::Success,
-                   bool runValidation = true, bool relaxLogicalPointer = false);
+                   bool runValidation = true);
 
 private:
   /// \brief Reads in the given input file.
@@ -46,6 +51,9 @@ private:
   std::string checkCommands;             ///< CHECK commands that verify output
   std::string generatedSpirvAsm;         ///< Disassembled binary (SPIR-V code)
   spv_target_env targetEnv;              ///< Environment to validate against
+  bool relaxLogicalPointer;
+  bool glLayout;
+  bool dxLayout;
 };
 
 } // end namespace spirv

tools/clang/unittests/SPIRV/FileTestUtils.cpp

@@ -35,11 +35,12 @@ bool disassembleSpirvBinary(std::vector<uint32_t> &binary,
 }
 
 bool validateSpirvBinary(spv_target_env env, std::vector<uint32_t> &binary,
-                         bool relaxLogicalPointer) {
+                         bool relaxLogicalPointer, bool glLayout,
+                         bool dxLayout) {
   spvtools::ValidatorOptions options;
   options.SetRelaxLogicalPointer(relaxLogicalPointer);
-  // TODO: Fix the following to enable validating layout.
-  options.SetRelaxBlockLayout(true);
+  options.SetRelaxBlockLayout(!glLayout && !dxLayout);
+  options.SetSkipBlockLayout(dxLayout);
   spvtools::SpirvTools spirvTools(env);
   spirvTools.SetMessageConsumer(
       [](spv_message_level_t, const char *, const spv_position_t &,

tools/clang/unittests/SPIRV/FileTestUtils.h

@@ -33,7 +33,8 @@ bool disassembleSpirvBinary(std::vector<uint32_t> &binary,
 /// \brief Runs the SPIR-V Tools validation on the given SPIR-V binary.
 /// Returns true if validation is successful; false otherwise.
 bool validateSpirvBinary(spv_target_env, std::vector<uint32_t> &binary,
-                         bool relaxLogicalPointer);
+                         bool relaxLogicalPointer, bool glLayout,
+                         bool dxLayout);
 
 /// \brief Parses the Target Profile and Entry Point from the Run command
 /// Returns the target profile, entry point, and the rest via arguments.

tools/clang/unittests/SPIRV/WholeFileTestFixture.cpp

@@ -107,8 +107,9 @@ void WholeFileTest::runWholeFileTest(llvm::StringRef filename,
 
   // Run SPIR-V validation if requested.
   if (runSpirvValidation) {
-    EXPECT_TRUE(utils::validateSpirvBinary(SPV_ENV_VULKAN_1_0, generatedBinary,
-                                           /*relaxLogicalPointer=*/false));
+    EXPECT_TRUE(utils::validateSpirvBinary(
+        SPV_ENV_VULKAN_1_0, generatedBinary,
+        /*relaxLogicalPointer=*/false, /*glLayout=*/false, /*dxLayout=*/false));
   }
 }