Fix location validation for I/O blocks

src/compiler/translator/ValidateVaryingLocations.cpp

@@ -64,9 +64,26 @@ int GetStructLocationCount(const TStructure *structure)
     return totalLocation;
 }
 
+int GetInterfaceBlockLocationCount(const TType &varyingType, bool ignoreVaryingArraySize)
+{
+    int totalLocation = 0;
+    for (const TField *field : varyingType.getInterfaceBlock()->fields())
+    {
+        totalLocation += GetFieldLocationCount(field);
+    }
+
+    if (!ignoreVaryingArraySize && varyingType.isArray())
+    {
+        totalLocation *= varyingType.getArraySizeProduct();
+    }
+    return totalLocation;
+}
+
 int GetLocationCount(const TIntermSymbol *varying, bool ignoreVaryingArraySize)
 {
     const TType &varyingType = varying->getType();
+    ASSERT(!varyingType.isInterfaceBlock());
+
     if (varyingType.getStruct() != nullptr)
     {
         int totalLocation = 0;
@@ -80,22 +97,6 @@ int GetLocationCount(const TIntermSymbol *varying, bool ignoreVaryingArraySize)
         return totalLocation;
     }
 
-    if (varyingType.isInterfaceBlock())
-    {
-        unsigned int totalLocation = 0;
-        for (const TField *field : varyingType.getInterfaceBlock()->fields())
-        {
-            totalLocation += GetFieldLocationCount(field);
-        }
-
-        ASSERT(!varyingType.isArrayOfArrays() || ignoreVaryingArraySize);
-        if (!ignoreVaryingArraySize && varyingType.isArray())
-        {
-            totalLocation *= varyingType.getArraySizeProduct();
-        }
-        return totalLocation;
-    }
-
     ASSERT(varyingType.isMatrix() || varyingType.getSecondarySize() == 1);
     int elementLocationCount = varyingType.isMatrix() ? varyingType.getNominalSize() : 1;
 
@@ -115,6 +116,47 @@ int GetLocationCount(const TIntermSymbol *varying, bool ignoreVaryingArraySize)
     return elementLocationCount * varyingType.getArraySizeProduct();
 }
 
+struct SymbolAndField
+{
+    const TIntermSymbol *symbol;
+    const TField *field;
+};
+using LocationMap = std::map<int, SymbolAndField>;
+
+void MarkVaryingLocations(TDiagnostics *diagnostics,
+                          const TIntermSymbol *varying,
+                          const TField *field,
+                          int location,
+                          int elementCount,
+                          LocationMap *locationMap)
+{
+    for (int elementIndex = 0; elementIndex < elementCount; ++elementIndex)
+    {
+        const int offsetLocation = location + elementIndex;
+        auto conflict            = locationMap->find(offsetLocation);
+        if (conflict != locationMap->end())
+        {
+            std::stringstream strstr = sh::InitializeStream<std::stringstream>();
+            strstr << "'" << varying->getName();
+            if (field)
+            {
+                strstr << "." << field->name();
+            }
+            strstr << "' conflicting location with '" << conflict->second.symbol->getName();
+            if (conflict->second.field)
+            {
+                strstr << "." << conflict->second.field->name();
+            }
+            strstr << "'";
+            error(*varying, strstr.str().c_str(), diagnostics);
+        }
+        else
+        {
+            (*locationMap)[offsetLocation] = {varying, field};
+        }
+    }
+}
+
 using VaryingVector = std::vector<const TIntermSymbol *>;
 
 void ValidateShaderInterface(TDiagnostics *diagnostics,
@@ -127,29 +169,89 @@ void ValidateShaderInterface(TDiagnostics *diagnostics,
         return;
     }
 
-    std::map<int, const TIntermSymbol *> locationMap;
+    LocationMap locationMap;
     for (const TIntermSymbol *varying : varyingVector)
     {
-        const int location = varying->getType().getLayoutQualifier().location;
+        const TType &varyingType = varying->getType();
+        const int location       = varyingType.getLayoutQualifier().location;
         ASSERT(location >= 0);
 
-        const int elementCount = GetLocationCount(varying, ignoreVaryingArraySize);
-        for (int elementIndex = 0; elementIndex < elementCount; ++elementIndex)
+        // A varying is either:
+        //
+        // - A vector or matrix, which can take a number of contiguous locations
+        // - A struct, which also takes a number of contiquous locations
+        // - An interface block.
+        //
+        // Interface blocks can assign arbitrary locations to their fields, for example:
+        //
+        //     layout(location = 4) in block {
+        //         vec4 a;                         // gets location 4
+        //         vec4 b;                         // gets location 5
+        //         layout(location = 7) vec4 c;    // gets location 7
+        //         vec4 d;                         // gets location 8
+        //         layout (location = 1) vec4 e;   // gets location 1
+        //         vec4 f;                         // gets location 2
+        //     };
+        //
+        // The following code therefore takes two paths.  For non-interface-block types, the number
+        // of locations for the varying is calculated (elementCount), and all locations in
+        // [location, location + elementCount) are marked as occupied.
+        //
+        // For interface blocks, a similar algorithm is implemented except each field is
+        // individually marked with the location either advancing automatically or taking its value
+        // from the field's layout qualifier.
+
+        if (varyingType.isInterfaceBlock())
         {
-            const int offsetLocation = location + elementIndex;
-            if (locationMap.find(offsetLocation) != locationMap.end())
+            int currentLocation       = location;
+            bool anyFieldWithLocation = false;
+
+            for (const TField *field : varyingType.getInterfaceBlock()->fields())
             {
-                std::stringstream strstr = sh::InitializeStream<std::stringstream>();
-                strstr << "'" << varying->getName()
-                       << "' conflicting location with previously defined '"
-                       << locationMap[offsetLocation]->getName() << "'";
-                error(*varying, strstr.str().c_str(), diagnostics);
+                const int fieldLocation = field->type()->getLayoutQualifier().location;
+                if (fieldLocation >= 0)
+                {
+                    currentLocation      = fieldLocation;
+                    anyFieldWithLocation = true;
+                }
+
+                const int fieldLocationCount = GetFieldLocationCount(field);
+                MarkVaryingLocations(diagnostics, varying, field, currentLocation,
+                                     fieldLocationCount, &locationMap);
+
+                currentLocation += fieldLocationCount;
             }
-            else
+
+            // Array interface blocks can't have location qualifiers on fields.
+            ASSERT(ignoreVaryingArraySize || !anyFieldWithLocation || !varyingType.isArray());
+
+            if (!ignoreVaryingArraySize && varyingType.isArray())
             {
-                locationMap[offsetLocation] = varying;
+                // This is only reached if the varying is an array of interface blocks, with only a
+                // layout qualifier on the block itself, for example:
+                //
+                //     layout(location = 4) in block {
+                //         vec4 a;
+                //         vec4 b;
+                //         vec4 c;
+                //         vec4 d;
+                //     } instance[N];
+                //
+                // The locations for instance[0] are already marked by the above code, so we need to
+                // further mark locations occupied by instances [1, N).  |currentLocation| is
+                // already just past the end of instance[0], which is the beginning of instance[1].
+                //
+                int remainingLocations = currentLocation * (varyingType.getArraySizeProduct() - 1);
+                MarkVaryingLocations(diagnostics, varying, nullptr, currentLocation,
+                                     remainingLocations, &locationMap);
             }
         }
+        else
+        {
+            const int elementCount = GetLocationCount(varying, ignoreVaryingArraySize);
+            MarkVaryingLocations(diagnostics, varying, nullptr, location, elementCount,
+                                 &locationMap);
+        }
     }
 }
 
@@ -225,9 +327,16 @@ void ValidateVaryingLocationsTraverser::validate(TDiagnostics *diagnostics)
 
 unsigned int CalculateVaryingLocationCount(TIntermSymbol *varying, GLenum shaderType)
 {
-    const TQualifier qualifier        = varying->getType().getQualifier();
+    const TType &varyingType          = varying->getType();
+    const TQualifier qualifier        = varyingType.getQualifier();
     const bool isShaderIn             = IsShaderIn(qualifier);
     const bool ignoreVaryingArraySize = isShaderIn && shaderType == GL_GEOMETRY_SHADER_EXT;
+
+    if (varyingType.isInterfaceBlock())
+    {
+        return GetInterfaceBlockLocationCount(varyingType, ignoreVaryingArraySize);
+    }
+
     return GetLocationCount(varying, ignoreVaryingArraySize);
 }
 

src/tests/gl_tests/GLSLTest.cpp

@@ -8778,6 +8778,144 @@ void main()
     program = CompileProgram(kVSStaticUse, kFSStaticUse);
     EXPECT_EQ(0u, program);
 }
+
+// Verify I/O block array locations:
+TEST_P(GLSLTest_ES31, IOBlockLocations)
+{
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_shader_io_blocks"));
+    ANGLE_SKIP_TEST_IF(!IsGLExtensionEnabled("GL_EXT_geometry_shader"));
+
+    // 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
+
+in highp vec4 position;
+
+layout(location = 0) out vec4 aOut;
+
+layout(location = 6) out VSBlock
+{
+    vec4 b;     // location 6
+    vec4 c;     // location 7
+    layout(location = 1) vec4 d;
+    vec4 e;     // location 2
+    vec4 f[2];  // locations 3 and 4
+} blockOut;
+
+layout(location = 5) out vec4 gOut;
+
+void main()
+{
+    aOut = vec4(0.03, 0.06, 0.09, 0.12);
+    blockOut.b = vec4(0.15, 0.18, 0.21, 0.24);
+    blockOut.c = vec4(0.27, 0.30, 0.33, 0.36);
+    blockOut.d = vec4(0.39, 0.42, 0.45, 0.48);
+    blockOut.e = vec4(0.51, 0.54, 0.57, 0.6);
+    blockOut.f[0] = vec4(0.63, 0.66, 0.66, 0.69);
+    blockOut.f[1] = vec4(0.72, 0.75, 0.78, 0.81);
+    gOut = vec4(0.84, 0.87, 0.9, 0.93);
+    gl_Position = position;
+})";
+
+    constexpr char kGS[] = R"(#version 310 es
+#extension GL_EXT_geometry_shader : require
+layout (invocations = 3, triangles) in;
+layout (triangle_strip, max_vertices = 3) out;
+
+// Input varyings
+layout(location = 0) in vec4 aIn[];
+
+layout(location = 6) in VSBlock
+{
+    vec4 b;
+    vec4 c;
+    layout(location = 1) vec4 d;
+    vec4 e;
+    vec4 f[2];
+} blockIn[];
+
+layout(location = 5) in vec4 gIn[];
+
+// Output varyings
+layout(location = 1) out vec4 aOut;
+
+layout(location = 0) out GSBlock
+{
+    vec4 b;     // location 0
+    layout(location = 3) vec4 c;
+    layout(location = 7) vec4 d;
+    layout(location = 5) vec4 e[2];
+    layout(location = 4) vec4 f;
+} blockOut;
+
+layout(location = 2) out vec4 gOut;
+
+void main()
+{
+    int n;
+    for (n = 0; n < gl_in.length(); n++)
+    {
+        gl_Position = gl_in[n].gl_Position;
+
+        aOut = aIn[n];
+        blockOut.b = blockIn[n].b;
+        blockOut.c = blockIn[n].c;
+        blockOut.d = blockIn[n].d;
+        blockOut.e[0] = blockIn[n].e;
+        blockOut.e[1] = blockIn[n].f[0];
+        blockOut.f = blockIn[n].f[1];
+        gOut = gIn[n];
+
+        EmitVertex();
+    }
+    EndPrimitive();
+})";
+
+    constexpr char kFS[] = R"(#version 310 es
+#extension GL_EXT_shader_io_blocks : require
+precision mediump float;
+
+layout(location = 0) out mediump vec4 color;
+
+layout(location = 1) in vec4 aIn;
+
+layout(location = 0) in GSBlock
+{
+    vec4 b;
+    layout(location = 3) vec4 c;
+    layout(location = 7) vec4 d;
+    layout(location = 5) vec4 e[2];
+    layout(location = 4) vec4 f;
+} blockIn;
+
+layout(location = 2) in vec4 gIn;
+
+bool isEq(vec4 a, vec4 b) { return all(lessThan(abs(a-b), vec4(0.001))); }
+
+void main()
+{
+    bool passR = isEq(aIn, vec4(0.03, 0.06, 0.09, 0.12));
+    bool passG = isEq(blockIn.b, vec4(0.15, 0.18, 0.21, 0.24)) &&
+                 isEq(blockIn.c, vec4(0.27, 0.30, 0.33, 0.36)) &&
+                 isEq(blockIn.d, vec4(0.39, 0.42, 0.45, 0.48)) &&
+                 isEq(blockIn.e[0], vec4(0.51, 0.54, 0.57, 0.6)) &&
+                 isEq(blockIn.e[1], vec4(0.63, 0.66, 0.66, 0.69)) &&
+                 isEq(blockIn.f, vec4(0.72, 0.75, 0.78, 0.81));
+    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();
+}
 }  // anonymous namespace
 
 // Use this to select which configurations (e.g. which renderer, which GLES major version) these