Fix varying packing of I/O block fields

src/libANGLE/VaryingPacking.cpp

@@ -24,28 +24,67 @@ namespace
 // true if varying x has a higher priority in packing than y
 bool ComparePackedVarying(const PackedVarying &x, const PackedVarying &y)
 {
-    // If the PackedVarying 'x' or 'y' to be compared is an array element, this clones an equivalent
-    // non-array shader variable 'vx' or 'vy' for actual comparison instead.
+    // If the PackedVarying 'x' or 'y' to be compared is an array element for transform feedback,
+    // this clones an equivalent non-array shader variable 'vx' or 'vy' for actual comparison
+    // instead.  For I/O block arrays, the array index is used in the comparison.
     sh::ShaderVariable vx, vy;
     const sh::ShaderVariable *px, *py;
 
     px = &x.varying();
     py = &y.varying();
 
-    if (x.isArrayElement())
+    if (x.isTransformFeedbackArrayElement())
     {
         vx = *px;
         vx.arraySizes.clear();
         px = &vx;
     }
 
-    if (y.isArrayElement())
+    if (y.isTransformFeedbackArrayElement())
     {
         vy = *py;
         vy.arraySizes.clear();
         py = &vy;
     }
 
+    // Make sure struct fields end up together.
+    if (x.isStructField() != y.isStructField())
+    {
+        return x.isStructField();
+    }
+
+    if (x.isStructField())
+    {
+        ASSERT(y.isStructField());
+
+        if (x.getParentStructName() != y.getParentStructName())
+        {
+            return x.getParentStructName() < y.getParentStructName();
+        }
+    }
+
+    // For I/O block fields, order first by array index:
+    if (!x.isTransformFeedbackArrayElement() && !y.isTransformFeedbackArrayElement())
+    {
+        if (x.arrayIndex != y.arrayIndex)
+        {
+            return x.arrayIndex < y.arrayIndex;
+        }
+    }
+
+    // Then order by field index
+    if (x.fieldIndex != y.fieldIndex)
+    {
+        return x.fieldIndex < y.fieldIndex;
+    }
+
+    // Then order by secondary field index
+    if (x.secondaryFieldIndex != y.secondaryFieldIndex)
+    {
+        return x.secondaryFieldIndex < y.secondaryFieldIndex;
+    }
+
+    // Otherwise order by variable
     return gl::CompareShaderVar(*px, *py);
 }
 
@@ -95,6 +134,7 @@ PackedVarying::PackedVarying(VaryingInShaderRef &&frontVaryingIn,
       backVarying(std::move(backVaryingIn)),
       interpolation(interpolationIn),
       arrayIndex(arrayIndexIn),
+      isTransformFeedback(false),
       fieldIndex(fieldIndexIn),
       secondaryFieldIndex(secondaryFieldIndexIn)
 {}
@@ -114,6 +154,7 @@ PackedVarying &PackedVarying::operator=(PackedVarying &&other)
     std::swap(backVarying, other.backVarying);
     std::swap(interpolation, other.interpolation);
     std::swap(arrayIndex, other.arrayIndex);
+    std::swap(isTransformFeedback, other.isTransformFeedback);
     std::swap(fieldIndex, other.fieldIndex);
     std::swap(secondaryFieldIndex, other.secondaryFieldIndex);
 
@@ -184,7 +225,7 @@ bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
     // GLSL ES 3.10 section 4.3.6: Output variables cannot be arrays of arrays or arrays of
     // structures, so we may use getBasicTypeElementCount().
     const unsigned int elementCount = varying.getBasicTypeElementCount();
-    varyingRows *= (packedVarying.isArrayElement() ? 1 : elementCount);
+    varyingRows *= (packedVarying.isTransformFeedbackArrayElement() ? 1 : elementCount);
 
     unsigned int maxVaryingVectors = static_cast<unsigned int>(mRegisterMap.size());
 
@@ -279,7 +320,8 @@ bool VaryingPacking::packVarying(const PackedVarying &packedVarying)
                     registerInfo.registerRow    = row + arrayIndex;
                     registerInfo.registerColumn = bestColumn;
                     registerInfo.varyingArrayIndex =
-                        (packedVarying.isArrayElement() ? packedVarying.arrayIndex : arrayIndex);
+                        (packedVarying.isTransformFeedbackArrayElement() ? packedVarying.arrayIndex
+                                                                         : arrayIndex);
                     registerInfo.varyingRowIndex = 0;
                     // Do not record register info for builtins.
                     // TODO(jmadill): Clean this up.
@@ -341,7 +383,8 @@ void VaryingPacking::insert(unsigned int registerRow,
     const unsigned int arrayElementCount = varying.getBasicTypeElementCount();
     for (unsigned int arrayElement = 0; arrayElement < arrayElementCount; ++arrayElement)
     {
-        if (packedVarying.isArrayElement() && arrayElement != packedVarying.arrayIndex)
+        if (packedVarying.isTransformFeedbackArrayElement() &&
+            arrayElement != packedVarying.arrayIndex)
         {
             continue;
         }
@@ -468,7 +511,8 @@ void VaryingPacking::packUserVaryingTF(const ProgramVaryingRef &ref, size_t subs
 
     mPackedVaryings.emplace_back(std::move(frontVarying), std::move(backVarying),
                                  input->interpolation);
-    mPackedVaryings.back().arrayIndex = static_cast<GLuint>(subscript);
+    mPackedVaryings.back().arrayIndex          = static_cast<GLuint>(subscript);
+    mPackedVaryings.back().isTransformFeedback = true;
 }
 
 void VaryingPacking::packUserVaryingFieldTF(const ProgramVaryingRef &ref,

src/libANGLE/VaryingPacking.h

@@ -80,7 +80,10 @@ struct PackedVarying : angle::NonCopyable
                                     : !backVarying.parentStructName.empty();
     }
 
-    bool isArrayElement() const { return arrayIndex != GL_INVALID_INDEX; }
+    bool isTransformFeedbackArrayElement() const
+    {
+        return isTransformFeedback && arrayIndex != GL_INVALID_INDEX;
+    }
 
     // Return either front or back varying, whichever is available.  Only used when the name of the
     // varying is not important, but only the type is interesting.
@@ -89,6 +92,12 @@ struct PackedVarying : angle::NonCopyable
         return frontVarying.varying ? *frontVarying.varying : *backVarying.varying;
     }
 
+    const std::string &getParentStructName() const
+    {
+        ASSERT(isStructField());
+        return frontVarying.varying ? frontVarying.parentStructName : backVarying.parentStructName;
+    }
+
     std::string fullName(ShaderType stage) const
     {
         ASSERT(stage == frontVarying.stage || stage == backVarying.stage);
@@ -121,8 +130,10 @@ struct PackedVarying : angle::NonCopyable
     // Cached so we can store sh::ShaderVariable to point to varying fields.
     sh::InterpolationType interpolation;
 
-    // Used by varyings that are captured with transform feedback, xor arrays of shader I/O blocks.
+    // Used by varyings that are captured with transform feedback, xor arrays of shader I/O blocks,
+    // distinguished by isTransformFeedback;
     GLuint arrayIndex;
+    bool isTransformFeedback;
 
     // Field index in the struct.  In Vulkan, this is used to assign a
     // struct-typed varying location to the location of its first field.

src/libANGLE/renderer/glslang_wrapper_utils.cpp

@@ -447,7 +447,8 @@ bool IsFirstRegisterOfVarying(const gl::PackedVaryingRegister &varyingReg)
     }
 
     // Similarly, assign array varying locations to the assigned location of the first element.
-    if (varyingReg.varyingArrayIndex != 0 || (varying.isArrayElement() && varying.arrayIndex != 0))
+    if (varyingReg.varyingArrayIndex != 0 ||
+        (varying.arrayIndex != GL_INVALID_INDEX && varying.arrayIndex != 0))
     {
         return false;
     }

src/tests/gl_tests/GLSLTest.cpp

@@ -8779,7 +8779,7 @@ void main()
     EXPECT_EQ(0u, program);
 }
 
-// Verify I/O block array locations:
+// Verify I/O block array locations
 TEST_P(GLSLTest_ES31, IOBlockLocations)
 {
     ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_shader_io_blocks"));
@@ -8788,10 +8788,6 @@ TEST_P(GLSLTest_ES31, IOBlockLocations)
     // http://anglebug.com/5444
     ANGLE_SKIP_TEST_IF(IsIntel() && IsOpenGL() && IsWindows());
 
-    // Incorrect SPIR-V transformation and possibly varying packing of I/O blocks with location
-    // qualifier on fields.
-    ANGLE_SKIP_TEST_IF(IsVulkan());
-
     constexpr char kVS[] = R"(#version 310 es
 #extension GL_EXT_shader_io_blocks : require
 
@@ -8910,12 +8906,81 @@ void main()
     bool passB = isEq(gIn, vec4(0.84, 0.87, 0.9, 0.93));
 
     color = vec4(passR, passG, passB, 1.0);
-
 })";
 
     ANGLE_GL_PROGRAM_WITH_GS(program, kVS, kGS, kFS);
     EXPECT_GL_NO_ERROR();
 }
+
+// Test varying packing in presence of multiple I/O blocks
+TEST_P(GLSLTest_ES31, MultipleIOBlocks)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_shader_io_blocks"));
+
+    constexpr char kVS[] = R"(#version 310 es
+#extension GL_EXT_shader_io_blocks : require
+
+in highp vec4 position;
+
+out VSBlock1
+{
+    vec4 a;
+    vec4 b[2];
+} blockOut1;
+
+out VSBlock2
+{
+    vec4 c[2];
+    vec4 d;
+} blockOut2;
+
+void main()
+{
+    blockOut1.a = vec4(0.15, 0.18, 0.21, 0.24);
+    blockOut1.b[0] = vec4(0.27, 0.30, 0.33, 0.36);
+    blockOut1.b[1] = vec4(0.39, 0.42, 0.45, 0.48);
+    blockOut2.c[0] = vec4(0.51, 0.54, 0.57, 0.6);
+    blockOut2.c[1] = vec4(0.63, 0.66, 0.66, 0.69);
+    blockOut2.d = vec4(0.72, 0.75, 0.78, 0.81);
+    gl_Position = position;
+})";
+
+    constexpr char kFS[] = R"(#version 310 es
+#extension GL_EXT_shader_io_blocks : require
+precision mediump float;
+
+layout(location = 0) out mediump vec4 color;
+
+in VSBlock1
+{
+    vec4 a;
+    vec4 b[2];
+} blockIn1;
+
+in VSBlock2
+{
+    vec4 c[2];
+    vec4 d;
+} blockIn2;
+
+bool isEq(vec4 a, vec4 b) { return all(lessThan(abs(a-b), vec4(0.001))); }
+
+void main()
+{
+    bool passR = isEq(blockIn1.a, vec4(0.15, 0.18, 0.21, 0.24));
+    bool passG = isEq(blockIn1.b[0], vec4(0.27, 0.30, 0.33, 0.36)) &&
+                 isEq(blockIn1.b[1], vec4(0.39, 0.42, 0.45, 0.48));
+    bool passB = isEq(blockIn2.c[0], vec4(0.51, 0.54, 0.57, 0.6)) &&
+                 isEq(blockIn2.c[1], vec4(0.63, 0.66, 0.66, 0.69));
+    bool passA = isEq(blockIn2.d, vec4(0.72, 0.75, 0.78, 0.81));
+
+    color = vec4(passR, passG, passB, passA);
+})";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    EXPECT_GL_NO_ERROR();
+}
+
 }  // anonymous namespace
 
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these