Improve memory layout handling in SPIRV generator

Added asserts that verify we don't try to emit the same struct or array
with two different memory layout rules. Some code paths were failing to
inspect the associated variable, leading to incorrect errors about the
attached offsets of members.

Added a test case that triggered that error, and also triggers the new
asserts.

Then, fixed the underlying cause: writing out the struct definition as a
side effect of accessing a member in getLValue().

Bug: skia:11205
Change-Id: I6e5fb76ea918ec9ff10425f2d519ddbc54404b27
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/357436
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>

gn/sksl_tests.gni

@@ -195,6 +195,7 @@ sksl_metal_tests = [
 
 sksl_spirv_tests = [
   "/sksl/spirv/ConstantVectorFromVector.sksl",
+  "/sksl/spirv/InterfaceBlockPushConstant.sksl",
   "/sksl/spirv/LayoutMultipleOf4.sksl",
   "/sksl/spirv/LayoutOutOfOrder.sksl",
   "/sksl/spirv/OpaqueTypeInArray.sksl",

resources/sksl/spirv/InterfaceBlockPushConstant.sksl

@@ -0,0 +1,8 @@
+layout(push_constant, binding=456) uniform testBlock {
+    layout(offset=16) half2x2 m1;
+    layout(offset=32) half2x2 m2;
+};
+
+void main() {
+    sk_FragColor = half4(m1[0][0], m1[1][1], m2[0][0], m2[1][1]);
+}

src/sksl/SkSLSPIRVCodeGenerator.cpp

@@ -496,6 +496,15 @@ SpvId SPIRVCodeGenerator::getType(const Type& rawType, const MemoryLayout& layou
     String key = type.name();
     if (type.isStruct() || type.isArray()) {
         key += to_string((int)layout.fStd);
+#ifdef SK_DEBUG
+        SkASSERT(layout.fStd == MemoryLayout::Standard::k140_Standard ||
+                 layout.fStd == MemoryLayout::Standard::k430_Standard);
+        MemoryLayout::Standard otherStd = layout.fStd == MemoryLayout::Standard::k140_Standard
+                                                  ? MemoryLayout::Standard::k430_Standard
+                                                  : MemoryLayout::Standard::k140_Standard;
+        String otherKey = type.name() + to_string((int)otherStd);
+        SkASSERT(fTypeMap.find(otherKey) == fTypeMap.end());
+#endif
     }
     auto entry = fTypeMap.find(key);
     if (entry == fTypeMap.end()) {
@@ -1865,7 +1874,7 @@ std::unique_ptr<SPIRVCodeGenerator::LValue> SPIRVCodeGenerator::getLValue(const
                 typeId = this->getType(*Type::MakeArrayType("sk_in", var.type().componentType(),
                                                             fSkInCount));
             } else {
-                typeId = this->getType(type);
+                typeId = this->getType(type, this->memoryLayoutForVariable(var));
             }
             auto entry = fVariableMap.find(&var);
             SkASSERT(entry != fVariableMap.end());
@@ -2756,6 +2765,12 @@ void SPIRVCodeGenerator::writeLayout(const Layout& layout, SpvId target, int mem
     }
 }
 
+MemoryLayout SPIRVCodeGenerator::memoryLayoutForVariable(const Variable& v) const {
+    bool isBuffer     = ((v.modifiers().fFlags & Modifiers::kBuffer_Flag) != 0);
+    bool pushConstant = ((v.modifiers().fLayout.fFlags & Layout::kPushConstant_Flag) != 0);
+    return (pushConstant || isBuffer) ? MemoryLayout(MemoryLayout::k430_Standard) : fDefaultLayout;
+}
+
 static void update_sk_in_count(const Modifiers& m, int* outSkInCount) {
     switch (m.fLayout.fPrimitive) {
         case Layout::kPoints_Primitive:
@@ -2779,12 +2794,7 @@ static void update_sk_in_count(const Modifiers& m, int* outSkInCount) {
 }
 
 SpvId SPIRVCodeGenerator::writeInterfaceBlock(const InterfaceBlock& intf, bool appendRTHeight) {
-    bool isBuffer = ((intf.variable().modifiers().fFlags & Modifiers::kBuffer_Flag) != 0);
-    bool pushConstant = ((intf.variable().modifiers().fLayout.fFlags &
-                          Layout::kPushConstant_Flag) != 0);
-    MemoryLayout memoryLayout = (pushConstant || isBuffer) ?
-                                MemoryLayout(MemoryLayout::k430_Standard) :
-                                fDefaultLayout;
+    MemoryLayout memoryLayout = this->memoryLayoutForVariable(intf.variable());
     SpvId result = this->nextId();
     std::unique_ptr<Type> rtHeightStructType;
     const Type* type = &intf.variable().type();

src/sksl/SkSLSPIRVCodeGenerator.h

@@ -382,6 +382,8 @@ private:
 
     void writeGeometryShaderExecutionMode(SpvId entryPoint, OutputStream& out);
 
+    MemoryLayout memoryLayoutForVariable(const Variable&) const;
+
     const Context& fContext;
     const MemoryLayout fDefaultLayout;
 

tests/sksl/spirv/InterfaceBlockPushConstant.asm.frag

@@ -0,0 +1,67 @@
+OpCapability Shader
+%1 = OpExtInstImport "GLSL.std.450"
+OpMemoryModel Logical GLSL450
+OpEntryPoint Fragment %main "main" %sk_FragColor %sk_Clockwise
+OpExecutionMode %main OriginUpperLeft
+OpName %testBlock "testBlock"
+OpMemberName %testBlock 0 "m1"
+OpMemberName %testBlock 1 "m2"
+OpName %sk_FragColor "sk_FragColor"
+OpName %sk_Clockwise "sk_Clockwise"
+OpName %main "main"
+OpMemberDecorate %testBlock 0 Offset 16
+OpMemberDecorate %testBlock 0 ColMajor
+OpMemberDecorate %testBlock 0 MatrixStride 8
+OpMemberDecorate %testBlock 0 RelaxedPrecision
+OpMemberDecorate %testBlock 1 Offset 32
+OpMemberDecorate %testBlock 1 ColMajor
+OpMemberDecorate %testBlock 1 MatrixStride 8
+OpMemberDecorate %testBlock 1 RelaxedPrecision
+OpDecorate %testBlock Block
+OpDecorate %3 Binding 456
+OpDecorate %3 DescriptorSet 0
+OpDecorate %sk_FragColor RelaxedPrecision
+OpDecorate %sk_FragColor Location 0
+OpDecorate %sk_FragColor Index 0
+OpDecorate %sk_Clockwise RelaxedPrecision
+OpDecorate %sk_Clockwise BuiltIn FrontFacing
+OpDecorate %22 RelaxedPrecision
+OpDecorate %26 RelaxedPrecision
+OpDecorate %29 RelaxedPrecision
+OpDecorate %32 RelaxedPrecision
+%float = OpTypeFloat 32
+%v2float = OpTypeVector %float 2
+%mat2v2float = OpTypeMatrix %v2float 2
+%testBlock = OpTypeStruct %mat2v2float %mat2v2float
+%_ptr_PushConstant_testBlock = OpTypePointer PushConstant %testBlock
+%3 = OpVariable %_ptr_PushConstant_testBlock PushConstant
+%v4float = OpTypeVector %float 4
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+%sk_FragColor = OpVariable %_ptr_Output_v4float Output
+%bool = OpTypeBool
+%_ptr_Input_bool = OpTypePointer Input %bool
+%sk_Clockwise = OpVariable %_ptr_Input_bool Input
+%void = OpTypeVoid
+%16 = OpTypeFunction %void
+%int = OpTypeInt 32 1
+%int_0 = OpConstant %int 0
+%_ptr_PushConstant_v2float = OpTypePointer PushConstant %v2float
+%int_1 = OpConstant %int 1
+%main = OpFunction %void None %16
+%17 = OpLabel
+%20 = OpAccessChain %_ptr_PushConstant_v2float %3 %int_0 %int_0
+%22 = OpLoad %v2float %20
+%23 = OpCompositeExtract %float %22 0
+%25 = OpAccessChain %_ptr_PushConstant_v2float %3 %int_0 %int_1
+%26 = OpLoad %v2float %25
+%27 = OpCompositeExtract %float %26 1
+%28 = OpAccessChain %_ptr_PushConstant_v2float %3 %int_1 %int_0
+%29 = OpLoad %v2float %28
+%30 = OpCompositeExtract %float %29 0
+%31 = OpAccessChain %_ptr_PushConstant_v2float %3 %int_1 %int_1
+%32 = OpLoad %v2float %31
+%33 = OpCompositeExtract %float %32 1
+%34 = OpCompositeConstruct %v4float %23 %27 %30 %33
+OpStore %sk_FragColor %34
+OpReturn
+OpFunctionEnd