Vulkan: Fix layout substitution for struct varyings

If the shader contains code such as the following:

    struct S {
        vec4 field;
    };
    out S varStruct;

The layout qualifier macro is defined as @@ LAYOUT-varStruct @@.
However, the Vulkan backend was replacing @@ LAYOUT-field @@.

Bug: angleproject:3220
Change-Id: Iae15003867e0bed2cc939159a6653429c7a431e0
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/1571389
Reviewed-by: Geoff Lang <geofflang@chromium.org>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>

src/common/utilities.cpp

@@ -771,7 +771,8 @@ std::string ParseResourceName(const std::string &name, std::vector<unsigned int>
 }
 
 const sh::ShaderVariable *FindShaderVarField(const sh::ShaderVariable &var,
-                                             const std::string &fullName)
+                                             const std::string &fullName,
+                                             GLuint *fieldIndexOut)
 {
     if (var.fields.empty())
     {
@@ -792,11 +793,12 @@ const sh::ShaderVariable *FindShaderVarField(const sh::ShaderVariable &var,
     {
         return nullptr;
     }
-    for (const auto &field : var.fields)
+    for (size_t field = 0; field < var.fields.size(); ++field)
     {
-        if (field.name == fieldName)
+        if (var.fields[field].name == fieldName)
         {
-            return &field;
+            *fieldIndexOut = static_cast<GLuint>(field);
+            return &var.fields[field];
         }
     }
     return nullptr;

src/common/utilities.h

@@ -62,7 +62,8 @@ std::string ParseResourceName(const std::string &name, std::vector<unsigned int>
 // Find the child field which matches 'fullName' == var.name + "." + field.name.
 // Return nullptr if not found.
 const sh::ShaderVariable *FindShaderVarField(const sh::ShaderVariable &var,
-                                             const std::string &fullName);
+                                             const std::string &fullName,
+                                             GLuint *fieldIndexOut);
 
 // Find the range of index values in the provided indices pointer.  Primitive restart indices are
 // only counted in the range if primitive restart is disabled.

src/libANGLE/Program.cpp

@@ -376,7 +376,8 @@ const sh::ShaderVariable *FindVaryingOrField(const ProgramMergedVaryings &varyin
             var = varying;
             break;
         }
-        var = FindShaderVarField(*varying, name);
+        GLuint fieldIndex = 0;
+        var               = FindShaderVarField(*varying, name, &fieldIndex);
         if (var != nullptr)
         {
             break;
@@ -3667,7 +3668,8 @@ void Program::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyi
             }
             else if (varying->isStruct())
             {
-                const auto *field = FindShaderVarField(*varying, tfVaryingName);
+                GLuint fieldIndex = 0;
+                const auto *field = FindShaderVarField(*varying, tfVaryingName, &fieldIndex);
                 if (field != nullptr)
                 {
                     mState.mLinkedTransformFeedbackVaryings.emplace_back(*field, *varying);

src/libANGLE/Shader.cpp

@@ -621,7 +621,8 @@ std::string Shader::getTransformFeedbackVaryingMappedName(const std::string &tfV
             }
             else if (varying.isStruct())
             {
-                const auto *field = FindShaderVarField(varying, tfVaryingName);
+                GLuint fieldIndex = 0;
+                const auto *field = FindShaderVarField(varying, tfVaryingName, &fieldIndex);
                 ASSERT(field != nullptr && !field->isStruct() && !field->isArray());
                 return varying.mappedName + "." + field->mappedName;
             }

src/libANGLE/VaryingPacking.cpp

@@ -314,10 +314,13 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
                 if (varying->isStruct())
                 {
                     ASSERT(!varying->isArray());
-                    for (const auto &field : varying->fields)
+                    for (GLuint fieldIndex = 0; fieldIndex < varying->fields.size(); ++fieldIndex)
                     {
+                        const sh::ShaderVariable &field = varying->fields[fieldIndex];
+
                         ASSERT(!field.isStruct() && !field.isArray());
-                        mPackedVaryings.emplace_back(field, interpolation, varying->name);
+                        mPackedVaryings.emplace_back(field, interpolation, varying->name,
+                                                     fieldIndex);
                         uniqueFullNames.insert(mPackedVaryings.back().fullName());
                     }
                 }
@@ -354,11 +357,14 @@ bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
             }
             if (input->isStruct())
             {
-                const sh::ShaderVariable *field = FindShaderVarField(*input, tfVarying);
+                GLuint fieldIndex = 0;
+                const sh::ShaderVariable *field =
+                    FindShaderVarField(*input, tfVarying, &fieldIndex);
                 if (field != nullptr)
                 {
                     ASSERT(!field->isStruct() && !field->isArray());
-                    mPackedVaryings.emplace_back(*field, input->interpolation, input->name);
+                    mPackedVaryings.emplace_back(*field, input->interpolation, input->name,
+                                                 fieldIndex);
                     mPackedVaryings.back().vertexOnly = true;
                     mPackedVaryings.back().arrayIndex = GL_INVALID_INDEX;
                     uniqueFullNames.insert(tfVarying);
@@ -425,9 +431,4 @@ bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
 
     return true;
 }
-
-const std::vector<std::string> &VaryingPacking::getInactiveVaryingNames() const
-{
-    return mInactiveVaryingNames;
-}
 }  // namespace gl

src/libANGLE/VaryingPacking.h

@@ -29,16 +29,18 @@ using ProgramMergedVaryings = std::map<std::string, ProgramVaryingRef>;
 struct PackedVarying
 {
     PackedVarying(const sh::ShaderVariable &varyingIn, sh::InterpolationType interpolationIn)
-        : PackedVarying(varyingIn, interpolationIn, "")
+        : PackedVarying(varyingIn, interpolationIn, "", false)
     {}
     PackedVarying(const sh::ShaderVariable &varyingIn,
                   sh::InterpolationType interpolationIn,
-                  const std::string &parentStructNameIn)
+                  const std::string &parentStructNameIn,
+                  GLuint fieldIndexIn)
         : varying(&varyingIn),
           vertexOnly(false),
           interpolation(interpolationIn),
           parentStructName(parentStructNameIn),
-          arrayIndex(GL_INVALID_INDEX)
+          arrayIndex(GL_INVALID_INDEX),
+          fieldIndex(fieldIndexIn)
     {}
 
     bool isStructField() const { return !parentStructName.empty(); }
@@ -73,6 +75,10 @@ struct PackedVarying
     std::string parentStructName;
 
     GLuint arrayIndex;
+
+    // Field index in the struct.  In Vulkan, this is used to assign a
+    // struct-typed varying location to the location of its first field.
+    GLuint fieldIndex;
 };
 
 struct PackedVaryingRegister final
@@ -171,7 +177,10 @@ class VaryingPacking final : angle::NonCopyable
         return static_cast<unsigned int>(mRegisterList.size());
     }
 
-    const std::vector<std::string> &getInactiveVaryingNames() const;
+    const std::vector<std::string> &getInactiveVaryingNames() const
+    {
+        return mInactiveVaryingNames;
+    }
 
   private:
     bool packVarying(const PackedVarying &packedVarying);

src/libANGLE/renderer/vulkan/GlslangWrapper.cpp

@@ -214,6 +214,19 @@ void GlslangWrapper::GetShaderSource(const gl::ProgramState &programState,
     {
         const auto &varying = *varyingReg.packedVarying;
 
+        // In Vulkan GLSL, struct fields are not allowed to have location assignments.  The varying
+        // of a struct type is thus given a location equal to the one assigned to its first field.
+        if (varying.isStructField() && varying.fieldIndex > 0)
+        {
+            continue;
+        }
+
+        // Similarly, assign array varying locations to the assigned location of the first element.
+        if (varying.isArrayElement() && varying.arrayIndex != 0)
+        {
+            continue;
+        }
+
         std::string locationString = "location = " + Str(varyingReg.registerRow);
         if (varyingReg.registerColumn > 0)
         {
@@ -222,12 +235,22 @@ void GlslangWrapper::GetShaderSource(const gl::ProgramState &programState,
             locationString += ", component = " + Str(varyingReg.registerColumn);
         }
 
-        InsertLayoutSpecifierString(&vertexSource, varying.varying->name, locationString);
+        // In the following:
-        InsertLayoutSpecifierString(&fragmentSource, varying.varying->name, locationString);
+        //
+        //     struct S { vec4 field; };
+        //     out S varStruct;
+        //
+        // "varStruct" is found through `parentStructName`, with `varying->name` being "field".  In
+        // such a case, use `parentStructName`.
+        const std::string &name =
+            varying.isStructField() ? varying.parentStructName : varying.varying->name;
+
+        InsertLayoutSpecifierString(&vertexSource, name, locationString);
+        InsertLayoutSpecifierString(&fragmentSource, name, locationString);
 
         ASSERT(varying.interpolation == sh::INTERPOLATION_SMOOTH);
-        InsertQualifierSpecifierString(&vertexSource, varying.varying->name, "out");
+        InsertQualifierSpecifierString(&vertexSource, name, "out");
-        InsertQualifierSpecifierString(&fragmentSource, varying.varying->name, "in");
+        InsertQualifierSpecifierString(&fragmentSource, name, "in");
     }
 
     // Remove all the markers for unused varyings.

src/tests/gl_tests/GLSLTest.cpp

@@ -3339,24 +3339,67 @@ TEST_P(GLSLTest_ES3, VaryingStructNotDeclaredInFragmentShader)
     ANGLE_GL_PROGRAM(program, kVS, kFS);
 }
 
-// Test that a varying struct that gets used in the fragment shader works.
+// Test that a varying struct that's not declared in the vertex shader, and is unused in the
-TEST_P(GLSLTest_ES3, VaryingStructUsedInFragmentShader)
+// fragment shader links successfully.
+TEST_P(GLSLTest_ES3, VaryingStructNotDeclaredInVertexShader)
 {
-    // TODO(syoussefi): missing ES3 shader feature support.
+    // GLSL ES allows the vertex shader to not declare a varying if the fragment shader is not
-    // http://anglebug.com/3199
+    // going to use it.  See section 9.1 in
-    ANGLE_SKIP_TEST_IF(IsVulkan());
+    // https://www.khronos.org/registry/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.pdf or
+    // section 4.3.5 in https://www.khronos.org/files/opengles_shading_language.pdf
+    //
+    // However, windows nvidia OpenGL ES driver fails to link this program.
+    //
+    // http://anglebug.com/3413
+    ANGLE_SKIP_TEST_IF(IsOpenGLES() && IsWindows() && IsNVIDIA());
+
+    constexpr char kVS[] =
+        "#version 300 es\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = vec4(1.0);\n"
+        "}\n";
+
+    constexpr char kFS[] =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 col;\n"
+        "struct S {\n"
+        "    vec4 field;\n"
+        "};\n"
+        "in S varStruct;\n"
+        "void main()\n"
+        "{\n"
+        "    col = vec4(1.0);\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+}
+
+// Test that a varying struct that's not initialized in the vertex shader links successfully.
+TEST_P(GLSLTest_ES3, VaryingStructNotInitializedInVertexShader)
+{
+    // GLSL ES allows the vertex shader to declare but not initialize a varying (with a
+    // specification that the varying values are undefined in the fragment stage).  See section 9.1
+    // in https://www.khronos.org/registry/OpenGL/specs/es/3.2/GLSL_ES_Specification_3.20.pdf
+    // or section 4.3.5 in https://www.khronos.org/files/opengles_shading_language.pdf
+    //
+    // However, windows and mac OpenGL drivers fail to link this program.  With a message like:
+    //
+    // > Input of fragment shader 'varStruct' not written by vertex shader
+    //
+    // http://anglebug.com/3413
+    ANGLE_SKIP_TEST_IF(IsDesktopOpenGL() && (IsOSX() || IsWindows()));
 
     constexpr char kVS[] =
         "#version 300 es\n"
-        "in vec4 inputAttribute;\n"
         "struct S {\n"
         "    vec4 field;\n"
         "};\n"
         "out S varStruct;\n"
         "void main()\n"
         "{\n"
-        "    gl_Position = inputAttribute;\n"
+        "    gl_Position = vec4(1.0);\n"
-        "    varStruct.field = vec4(0.0, 1.0, 0.0, 1.0);\n"
         "}\n";
 
     constexpr char kFS[] =
@@ -3373,10 +3416,172 @@ TEST_P(GLSLTest_ES3, VaryingStructUsedInFragmentShader)
         "}\n";
 
     ANGLE_GL_PROGRAM(program, kVS, kFS);
+}
+
+// Test that a varying struct that gets used in the fragment shader works.
+TEST_P(GLSLTest_ES3, VaryingStructUsedInFragmentShader)
+{
+    constexpr char kVS[] =
+        "#version 300 es\n"
+        "in vec4 inputAttribute;\n"
+        "struct S {\n"
+        "    vec4 field;\n"
+        "};\n"
+        "out S varStruct;\n"
+        "out S varStruct2;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = inputAttribute;\n"
+        "    varStruct.field = vec4(0.0, 0.5, 0.0, 1.0);\n"
+        "    varStruct2.field = vec4(0.0, 0.5, 0.0, 1.0);\n"
+        "}\n";
+
+    constexpr char kFS[] =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 col;\n"
+        "struct S {\n"
+        "    vec4 field;\n"
+        "};\n"
+        "in S varStruct;\n"
+        "in S varStruct2;\n"
+        "void main()\n"
+        "{\n"
+        "    col = varStruct.field + varStruct2.field;\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
     drawQuad(program.get(), "inputAttribute", 0.5f);
     EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
 }
 
+// Test that multiple multi-field varying structs that get used in the fragment shader work.
+TEST_P(GLSLTest_ES3, ComplexVaryingStructsUsedInFragmentShader)
+{
+    // TODO(syoussefi): fails on android with:
+    //
+    // > Internal Vulkan error: A return array was too small for the result
+    //
+    // http://anglebug.com/3220
+    ANGLE_SKIP_TEST_IF(IsVulkan() && IsAndroid());
+
+    constexpr char kVS[] =
+        "#version 300 es\n"
+        "in vec4 inputAttribute;\n"
+        "struct S {\n"
+        "    vec4 field1;\n"
+        "    vec4 field2;\n"
+        "};\n"
+        "out S varStruct;\n"
+        "out S varStruct2;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = inputAttribute;\n"
+        "    varStruct.field1 = vec4(0.0, 0.5, 0.0, 1.0);\n"
+        "    varStruct.field2 = vec4(0.0, 0.5, 0.0, 1.0);\n"
+        "    varStruct2.field1 = vec4(0.0, 0.5, 0.0, 1.0);\n"
+        "    varStruct2.field2 = vec4(0.0, 0.5, 0.0, 1.0);\n"
+        "}\n";
+
+    constexpr char kFS[] =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 col;\n"
+        "struct S {\n"
+        "    vec4 field1;\n"
+        "    vec4 field2;\n"
+        "};\n"
+        "in S varStruct;\n"
+        "in S varStruct2;\n"
+        "void main()\n"
+        "{\n"
+        "    col = varStruct.field1 + varStruct2.field2;\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    drawQuad(program.get(), "inputAttribute", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Test that an inactive varying struct that doesn't get used in the fragment shader works.
+TEST_P(GLSLTest_ES3, InactiveVaryingStructUnusedInFragmentShader)
+{
+    // TODO(syoussefi): http://anglebug.com/3412
+    ANGLE_SKIP_TEST_IF(IsVulkan());
+
+    constexpr char kVS[] =
+        "#version 300 es\n"
+        "in vec4 inputAttribute;\n"
+        "struct S {\n"
+        "    vec4 field;\n"
+        "};\n"
+        "out S varStruct;\n"
+        "out S varStruct2;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = inputAttribute;\n"
+        "    varStruct.field = vec4(0.0, 1.0, 0.0, 1.0);\n"
+        "    varStruct2.field = vec4(0.0, 1.0, 0.0, 1.0);\n"
+        "}\n";
+
+    constexpr char kFS[] =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 col;\n"
+        "struct S {\n"
+        "    vec4 field;\n"
+        "};\n"
+        "in S varStruct;\n"
+        "in S varStruct2;\n"
+        "void main()\n"
+        "{\n"
+        "    col = varStruct.field;\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    drawQuad(program.get(), "inputAttribute", 0.5f);
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
+// Test that multiple varying matrices that get used in the fragment shader work.
+TEST_P(GLSLTest_ES3, VaryingMatrices)
+{
+    constexpr char kVS[] =
+        "#version 300 es\n"
+        "in vec4 inputAttribute;\n"
+        "out mat2x2 varMat;\n"
+        "out mat2x2 varMat2;\n"
+        "out mat4x3 varMat3;\n"
+        "void main()\n"
+        "{\n"
+        "    gl_Position = inputAttribute;\n"
+        "    varMat[0] = vec2(1, 1);\n"
+        "    varMat[1] = vec2(1, 1);\n"
+        "    varMat2[0] = vec2(0.5, 0.5);\n"
+        "    varMat2[1] = vec2(0.5, 0.5);\n"
+        "    varMat3[0] = vec3(0.75, 0.75, 0.75);\n"
+        "    varMat3[1] = vec3(0.75, 0.75, 0.75);\n"
+        "    varMat3[2] = vec3(0.75, 0.75, 0.75);\n"
+        "    varMat3[3] = vec3(0.75, 0.75, 0.75);\n"
+        "}\n";
+
+    constexpr char kFS[] =
+        "#version 300 es\n"
+        "precision mediump float;\n"
+        "out vec4 col;\n"
+        "in mat2x2 varMat;\n"
+        "in mat2x2 varMat2;\n"
+        "in mat4x3 varMat3;\n"
+        "void main()\n"
+        "{\n"
+        "    col = vec4(varMat[0].x, varMat2[1].y, varMat3[2].z, 1);\n"
+        "}\n";
+
+    ANGLE_GL_PROGRAM(program, kVS, kFS);
+    drawQuad(program.get(), "inputAttribute", 0.5f);
+    EXPECT_PIXEL_COLOR_NEAR(0, 0, GLColor(255, 127, 191, 255), 1);
+}
+
 // This test covers passing a struct containing a sampler as a function argument.
 TEST_P(GLSLTest, StructsWithSamplersAsFunctionArg)
 {