Fix varying linking by location

src/libANGLE/Program.cpp

@@ -405,13 +405,19 @@ void InitShaderStorageBlockLinker(const ProgramState &state, ShaderStorageBlockL
 }
 
 // Find the matching varying or field by name.
-const sh::ShaderVariable *FindVaryingOrField(const ProgramMergedVaryings &varyings,
-                                             const std::string &name)
+const sh::ShaderVariable *FindOutputVaryingOrField(const ProgramMergedVaryings &varyings,
+                                                   ShaderType stage,
+                                                   const std::string &name)
 {
     const sh::ShaderVariable *var = nullptr;
-    for (const auto &ref : varyings)
+    for (const ProgramVaryingRef &ref : varyings)
     {
-        const sh::ShaderVariable *varying = ref.second.get();
+        if (ref.frontShaderStage != stage)
+        {
+            continue;
+        }
+
+        const sh::ShaderVariable *varying = ref.get(stage);
         if (varying->name == name)
         {
             var = varying;
@@ -1583,7 +1589,7 @@ angle::Result Program::link(const Context *context)
             return angle::Result::Continue;
         }
 
-        const auto &mergedVaryings = getMergedVaryings();
+        const ProgramMergedVaryings &mergedVaryings = getMergedVaryings();
 
         gl::Shader *vertexShader = mState.mAttachedShaders[ShaderType::Vertex];
         if (vertexShader)
@@ -1594,8 +1600,11 @@ angle::Result Program::link(const Context *context)
         InitUniformBlockLinker(mState, &resources->uniformBlockLinker);
         InitShaderStorageBlockLinker(mState, &resources->shaderStorageBlockLinker);
 
+        ShaderType tfStage = mState.mAttachedShaders[ShaderType::Geometry] ? ShaderType::Geometry
+                                                                           : ShaderType::Vertex;
+
         if (!linkValidateTransformFeedback(context->getClientVersion(), mInfoLog, mergedVaryings,
-                                           context->getCaps()))
+                                           tfStage, context->getCaps()))
         {
             return angle::Result::Continue;
         }
@@ -1606,7 +1615,7 @@ angle::Result Program::link(const Context *context)
             return angle::Result::Continue;
         }
 
-        gatherTransformFeedbackVaryings(mergedVaryings);
+        gatherTransformFeedbackVaryings(mergedVaryings, tfStage);
         mState.updateTransformFeedbackStrides();
     }
 
@@ -4107,6 +4116,7 @@ bool Program::linkValidateBuiltInVaryings(InfoLog &infoLog) const
 bool Program::linkValidateTransformFeedback(const Version &version,
                                             InfoLog &infoLog,
                                             const ProgramMergedVaryings &varyings,
+                                            ShaderType stage,
                                             const Caps &caps) const
 {
 
@@ -4147,7 +4157,7 @@ bool Program::linkValidateTransformFeedback(const Version &version,
         std::vector<unsigned int> subscripts;
         std::string baseName = ParseResourceName(tfVaryingName, &subscripts);
 
-        const sh::ShaderVariable *var = FindVaryingOrField(varyings, baseName);
+        const sh::ShaderVariable *var = FindOutputVaryingOrField(varyings, stage, baseName);
         if (var == nullptr)
         {
             infoLog << "Transform feedback varying " << tfVaryingName
@@ -4328,7 +4338,8 @@ bool Program::linkValidateGlobalNames(InfoLog &infoLog) const
     return true;
 }
 
-void Program::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings)
+void Program::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings,
+                                              ShaderType stage)
 {
     // Gather the linked varyings that are used for transform feedback, they should all exist.
     mState.mLinkedTransformFeedbackVaryings.clear();
@@ -4341,9 +4352,14 @@ void Program::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyi
         {
             subscript = subscripts.back();
         }
-        for (const auto &ref : varyings)
+        for (const ProgramVaryingRef &ref : varyings)
         {
-            const sh::ShaderVariable *varying = ref.second.get();
+            if (ref.frontShaderStage != stage)
+            {
+                continue;
+            }
+
+            const sh::ShaderVariable *varying = ref.get(stage);
             if (baseName == varying->name)
             {
                 mState.mLinkedTransformFeedbackVaryings.emplace_back(
@@ -4366,25 +4382,113 @@ void Program::gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyi
 
 ProgramMergedVaryings Program::getMergedVaryings() const
 {
+    ASSERT(mState.mAttachedShaders[ShaderType::Compute] == nullptr);
+
+    // Varyings are matched between pairs of consecutive stages, by location if assigned or
+    // by name otherwise.  Note that it's possible for one stage to specify location and the other
+    // not: https://cvs.khronos.org/bugzilla/show_bug.cgi?id=16261
+
+    // Map stages to the previous active stage in the rendering pipeline.  When looking at input
+    // varyings of a stage, this is used to find the stage whose output varyings are being linked
+    // with them.
+    ShaderMap<ShaderType> previousActiveStage;
+
+    // Note that kAllGraphicsShaderTypes is sorted according to the rendering pipeline.
+    ShaderType lastActiveStage = ShaderType::InvalidEnum;
+    for (ShaderType stage : kAllGraphicsShaderTypes)
+    {
+        previousActiveStage[stage] = lastActiveStage;
+        if (mState.mAttachedShaders[stage])
+        {
+            lastActiveStage = stage;
+        }
+    }
+
+    // First, go through output varyings and create two maps (one by name, one by location) for
+    // faster lookup when matching input varyings.
+
+    ShaderMap<std::map<std::string, size_t>> outputVaryingNameToIndex;
+    ShaderMap<std::map<int, size_t>> outputVaryingLocationToIndex;
+
     ProgramMergedVaryings merged;
 
+    // Gather output varyings.
     for (Shader *shader : mState.mAttachedShaders)
     {
-        if (shader)
+        if (!shader)
+        {
+            continue;
+        }
+        ShaderType stage = shader->getType();
+
+        for (const sh::ShaderVariable &varying : shader->getOutputVaryings())
+        {
+            merged.push_back({});
+            ProgramVaryingRef *ref = &merged.back();
+
+            ref->frontShader      = &varying;
+            ref->frontShaderStage = stage;
+
+            // Always map by name.  Even if location is provided in this stage, it may not be in the
+            // paired stage.
+            outputVaryingNameToIndex[stage][varying.name] = merged.size() - 1;
+
+            // If location is provided, also keep it in a map by location.
+            if (varying.location != -1)
+            {
+                outputVaryingLocationToIndex[stage][varying.location] = merged.size() - 1;
+            }
+        }
+    }
+
+    // Gather input varyings, and match them with output varyings of the previous stage.
+    for (Shader *shader : mState.mAttachedShaders)
+    {
+        if (!shader)
         {
-            ShaderType shaderType = shader->getType();
-            for (const sh::ShaderVariable &varying : shader->getOutputVaryings())
+            continue;
+        }
+        ShaderType stage         = shader->getType();
+        ShaderType previousStage = previousActiveStage[stage];
+
+        for (const sh::ShaderVariable &varying : shader->getInputVaryings())
+        {
+            size_t mergedIndex = merged.size();
+            if (previousStage != ShaderType::InvalidEnum)
             {
-                ProgramVaryingRef *ref = &merged[varying.name];
-                ref->frontShader       = &varying;
-                ref->frontShaderStage  = shaderType;
+                // If location is provided, see if we can match by location.
+                if (varying.location != -1)
+                {
+                    auto byLocationIter =
+                        outputVaryingLocationToIndex[previousStage].find(varying.location);
+                    if (byLocationIter != outputVaryingLocationToIndex[previousStage].end())
+                    {
+                        mergedIndex = byLocationIter->second;
+                    }
+                }
+
+                // If not found, try to match by name.
+                if (mergedIndex == merged.size())
+                {
+                    auto byNameIter = outputVaryingNameToIndex[previousStage].find(varying.name);
+                    if (byNameIter != outputVaryingNameToIndex[previousStage].end())
+                    {
+                        mergedIndex = byNameIter->second;
+                    }
+                }
             }
-            for (const sh::ShaderVariable &varying : shader->getInputVaryings())
+
+            // If no previous stage, or not matched by location or name, create a new entry for it.
+            if (mergedIndex == merged.size())
             {
-                ProgramVaryingRef *ref = &merged[varying.name];
-                ref->backShader        = &varying;
-                ref->backShaderStage   = shaderType;
+                merged.push_back({});
+                mergedIndex = merged.size() - 1;
             }
+
+            ProgramVaryingRef *ref = &merged[mergedIndex];
+
+            ref->backShader      = &varying;
+            ref->backShaderStage = stage;
         }
     }
 

src/libANGLE/Program.h

@@ -568,7 +568,13 @@ class ProgramAliasedBindings final : angle::NonCopyable
 
 struct ProgramVaryingRef
 {
-    const sh::ShaderVariable *get() const { return frontShader ? frontShader : backShader; }
+    const sh::ShaderVariable *get(ShaderType stage) const
+    {
+        ASSERT(stage == frontShaderStage || stage == backShaderStage);
+        const sh::ShaderVariable *ref = stage == frontShaderStage ? frontShader : backShader;
+        ASSERT(ref);
+        return ref;
+    }
 
     const sh::ShaderVariable *frontShader = nullptr;
     const sh::ShaderVariable *backShader  = nullptr;
@@ -576,7 +582,7 @@ struct ProgramVaryingRef
     ShaderType backShaderStage            = ShaderType::InvalidEnum;
 };
 
-using ProgramMergedVaryings = std::map<std::string, ProgramVaryingRef>;
+using ProgramMergedVaryings = std::vector<ProgramVaryingRef>;
 
 class Program final : angle::NonCopyable, public LabeledObject
 {
@@ -1025,10 +1031,11 @@ class Program final : angle::NonCopyable, public LabeledObject
     bool linkValidateTransformFeedback(const Version &version,
                                        InfoLog &infoLog,
                                        const ProgramMergedVaryings &linkedVaryings,
+                                       ShaderType stage,
                                        const Caps &caps) const;
     bool linkValidateGlobalNames(InfoLog &infoLog) const;
 
-    void gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings);
+    void gatherTransformFeedbackVaryings(const ProgramMergedVaryings &varyings, ShaderType stage);
 
     ProgramMergedVaryings getMergedVaryings() const;
     int getOutputLocationForLink(const sh::ShaderVariable &outputVariable) const;

src/libANGLE/VaryingPacking.h

@@ -25,34 +25,77 @@ namespace gl
 class InfoLog;
 struct ProgramVaryingRef;
 
-using ProgramMergedVaryings = std::map<std::string, ProgramVaryingRef>;
+using ProgramMergedVaryings = std::vector<ProgramVaryingRef>;
+
+// A varying can have different names between stages if matched by the location layout qualifier.
+// Additionally, same name varyings could still be of two identical struct types with different
+// names.  This struct contains information on the varying in one of the two stages.  PackedVarying
+// will thus contain two copies of this along with common information, such as interpolation or
+// field index.
+struct VaryingInShaderRef : angle::NonCopyable
+{
+    VaryingInShaderRef(ShaderType stageIn, const sh::ShaderVariable *varyingIn);
+    VaryingInShaderRef(VaryingInShaderRef &&other);
+    ~VaryingInShaderRef();
+
+    VaryingInShaderRef &operator=(VaryingInShaderRef &&other);
+
+    const sh::ShaderVariable *varying;
+
+    ShaderType stage;
+
+    // Struct name
+    std::string parentStructName;
+    std::string parentStructMappedName;
+};
 
 struct PackedVarying : angle::NonCopyable
 {
-    PackedVarying(const sh::ShaderVariable &varyingIn, sh::InterpolationType interpolationIn);
-    PackedVarying(const sh::ShaderVariable &varyingIn,
+    // Throughout this file, the "front" stage refers to the stage that outputs the varying, and the
+    // "back" stage refers to the stage that takes the varying as input.  Note that this struct
+    // contains linked varyings, which means both front and back stage varyings are valid, except
+    // for the following which may have only one valid stage.
+    //
+    //  - transform-feedback-captured varyings
+    //  - builtins
+    //  - separable program stages,
+    //
+    PackedVarying(VaryingInShaderRef &&frontVaryingIn,
+                  VaryingInShaderRef &&backVaryingIn,
+                  sh::InterpolationType interpolationIn);
+    PackedVarying(VaryingInShaderRef &&frontVaryingIn,
+                  VaryingInShaderRef &&backVaryingIn,
                   sh::InterpolationType interpolationIn,
-                  const std::string &parentStructNameIn,
-                  const std::string &parentStructMappedNameIn,
                   GLuint fieldIndexIn);
     PackedVarying(PackedVarying &&other);
     ~PackedVarying();
 
     PackedVarying &operator=(PackedVarying &&other);
 
-    bool isStructField() const { return !parentStructName.empty(); }
+    bool isStructField() const { return !frontVarying.parentStructName.empty(); }
 
     bool isArrayElement() const { return arrayIndex != GL_INVALID_INDEX; }
 
-    std::string fullName() const
+    // 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.
+    const sh::ShaderVariable &varying() const
+    {
+        return frontVarying.varying ? *frontVarying.varying : *backVarying.varying;
+    }
+
+    std::string fullName(ShaderType stage) const
     {
+        ASSERT(stage == frontVarying.stage || stage == backVarying.stage);
+        const VaryingInShaderRef &varying =
+            stage == frontVarying.stage ? frontVarying : backVarying;
+
         std::stringstream fullNameStr;
         if (isStructField())
         {
-            fullNameStr << parentStructName << ".";
+            fullNameStr << varying.parentStructName << ".";
         }
 
-        fullNameStr << varying->name;
+        fullNameStr << varying.varying->name;
         if (arrayIndex != GL_INVALID_INDEX)
         {
             fullNameStr << "[" << arrayIndex << "]";
@@ -63,22 +106,15 @@ struct PackedVarying : angle::NonCopyable
     // Transform feedback varyings can be only referenced in the VS.
     bool vertexOnly() const
     {
-        ShaderBitSet vertex;
-        vertex.set(ShaderType::Vertex);
-        return shaderStages == vertex;
+        return frontVarying.stage == ShaderType::Vertex && backVarying.varying == nullptr;
     }
 
-    const sh::ShaderVariable *varying;
-
-    ShaderBitSet shaderStages;
+    VaryingInShaderRef frontVarying;
+    VaryingInShaderRef backVarying;
 
     // Cached so we can store sh::ShaderVariable to point to varying fields.
     sh::InterpolationType interpolation;
 
-    // Struct name
-    std::string parentStructName;
-    std::string parentStructMappedName;
-
     GLuint arrayIndex;
 
     // Field index in the struct.  In Vulkan, this is used to assign a
@@ -112,14 +148,7 @@ struct PackedVaryingRegister final
 
     std::string tfVaryingName() const
     {
-        if (packedVarying->isArrayElement() || packedVarying->isStructField())
-        {
-            return packedVarying->fullName();
-        }
-        else
-        {
-            return packedVarying->varying->name;
-        }
+        return packedVarying->fullName(packedVarying->frontVarying.stage);
     }
 
     // Index to the array of varyings.
@@ -182,13 +211,11 @@ class VaryingPacking final : angle::NonCopyable
         return static_cast<unsigned int>(mRegisterList.size());
     }
 
-    const std::vector<std::string> &getInactiveVaryingMappedNames() const
+    const ShaderMap<std::vector<std::string>> &getInactiveVaryingMappedNames() const
     {
         return mInactiveVaryingMappedNames;
     }
 
-    const std::vector<sh::ShaderVariable> &getInputVaryings() const { return mInputVaryings; }
-
   private:
     bool packVarying(const PackedVarying &packedVarying);
     bool isFree(unsigned int registerRow,
@@ -199,11 +226,20 @@ class VaryingPacking final : angle::NonCopyable
                 unsigned int registerColumn,
                 const PackedVarying &packedVarying);
 
+    using VaryingUniqueFullNames = ShaderMap<std::set<std::string>>;
+    void packUserVarying(const ProgramVaryingRef &ref, VaryingUniqueFullNames *uniqueFullNames);
+    void packUserVaryingField(const ProgramVaryingRef &ref,
+                              GLuint fieldIndex,
+                              VaryingUniqueFullNames *uniqueFullNames);
+    void packUserVaryingTF(const ProgramVaryingRef &ref, size_t subscript);
+    void packUserVaryingFieldTF(const ProgramVaryingRef &ref,
+                                const sh::ShaderVariable &field,
+                                GLuint fieldIndex);
+
     std::vector<Register> mRegisterMap;
     std::vector<PackedVaryingRegister> mRegisterList;
-    std::vector<sh::ShaderVariable> mInputVaryings;
     std::vector<PackedVarying> mPackedVaryings;
-    std::vector<std::string> mInactiveVaryingMappedNames;
+    ShaderMap<std::vector<std::string>> mInactiveVaryingMappedNames;
 
     PackMode mPackMode;
 };

src/libANGLE/VaryingPacking_unittest.cpp

@@ -30,9 +30,11 @@ class VaryingPackingTest : public ::testing::TestWithParam<GLuint>
                             VaryingPacking *varyingPacking)
     {
         std::vector<PackedVarying> packedVaryings;
-        for (const auto &shVarying : shVaryings)
+        for (const sh::ShaderVariable &shVarying : shVaryings)
         {
-            packedVaryings.push_back(PackedVarying(shVarying, shVarying.interpolation));
+            packedVaryings.push_back(PackedVarying(
+                VaryingInShaderRef(ShaderType::Vertex, &shVarying),
+                VaryingInShaderRef(ShaderType::Fragment, &shVarying), shVarying.interpolation));
         }
 
         InfoLog infoLog;

src/libANGLE/renderer/d3d/DynamicHLSL.cpp

@@ -414,7 +414,7 @@ void DynamicHLSL::generateVaryingLinkHLSL(const VaryingPacking &varyingPacking,
     for (GLuint registerIndex = 0u; registerIndex < registerInfos.size(); ++registerIndex)
     {
         const PackedVaryingRegister &registerInfo = registerInfos[registerIndex];
-        const auto &varying                       = *registerInfo.packedVarying->varying;
+        const auto &varying                       = registerInfo.packedVarying->varying();
         ASSERT(!varying.isStruct());
 
         // TODO: Add checks to ensure D3D interpolation modifiers don't result in too many
@@ -599,14 +599,15 @@ void DynamicHLSL::generateShaderLinkHLSL(const gl::Caps &caps,
     {
         const PackedVaryingRegister &registerInfo = registerInfos[registerIndex];
         const auto &packedVarying                 = *registerInfo.packedVarying;
-        const auto &varying                       = *packedVarying.varying;
+        const auto &varying                       = *packedVarying.frontVarying.varying;
         ASSERT(!varying.isStruct());
 
         vertexGenerateOutput << "    output.v" << registerIndex << " = ";
 
         if (packedVarying.isStructField())
         {
-            vertexGenerateOutput << DecorateVariable(packedVarying.parentStructName) << ".";
+            vertexGenerateOutput << DecorateVariable(packedVarying.frontVarying.parentStructName)
+                                 << ".";
         }
 
         vertexGenerateOutput << DecorateVariable(varying.name);
@@ -798,19 +799,28 @@ void DynamicHLSL::generateShaderLinkHLSL(const gl::Caps &caps,
     {
         const PackedVaryingRegister &registerInfo = registerInfos[registerIndex];
         const auto &packedVarying                 = *registerInfo.packedVarying;
-        const auto &varying                       = *packedVarying.varying;
+
+        // Don't reference VS-only transform feedback varyings in the PS.
+        if (packedVarying.vertexOnly())
+        {
+            continue;
+        }
+
+        const auto &varying = *packedVarying.backVarying.varying;
         ASSERT(!varying.isBuiltIn() && !varying.isStruct());
 
-        // Don't reference VS-only transform feedback varyings in the PS. Note that we're relying on
-        // that the active flag is set according to usage in the fragment shader.
-        if (packedVarying.vertexOnly() || !varying.active)
+        // Note that we're relying on that the active flag is set according to usage in the fragment
+        // shader.
+        if (!varying.active)
+        {
             continue;
+        }
 
         pixelPrologue << "    ";
 
         if (packedVarying.isStructField())
         {
-            pixelPrologue << DecorateVariable(packedVarying.parentStructName) << ".";
+            pixelPrologue << DecorateVariable(packedVarying.backVarying.parentStructName) << ".";
         }
 
         pixelPrologue << DecorateVariable(varying.name);

src/libANGLE/renderer/d3d/ProgramD3D.cpp

@@ -3141,7 +3141,7 @@ void ProgramD3D::gatherTransformFeedbackVaryings(const gl::VaryingPacking &varyi
             for (GLuint registerIndex = 0u; registerIndex < registerInfos.size(); ++registerIndex)
             {
                 const auto &registerInfo = registerInfos[registerIndex];
-                const auto &varying      = *registerInfo.packedVarying->varying;
+                const auto &varying      = registerInfo.packedVarying->varying();
                 GLenum transposedType    = gl::TransposeMatrixType(varying.type);
                 int componentCount       = gl::VariableColumnCount(transposedType);
                 ASSERT(!varying.isBuiltIn() && !varying.isStruct());

src/tests/gl_tests/GLSLTest.cpp

@@ -6166,9 +6166,6 @@ void main()
 // Test that linking varyings by location works.
 TEST_P(GLSLTest_ES31, LinkVaryingsByLocation)
 {
-    // http://anglebug.com/4355
-    ANGLE_SKIP_TEST_IF(IsVulkan() || IsMetal() || IsD3D11());
-
     constexpr char kVS[] = R"(#version 310 es
 precision highp float;
 in vec4 position;