Refactor VaryingPacking

src/libANGLE/Program.cpp

@@ -106,37 +106,6 @@ void UniformStateQueryCastLoop(DestT *dataOut, const uint8_t *srcPointer, int co
     }
 }
 
-// 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.
-    sh::ShaderVariable vx, vy;
-    const sh::ShaderVariable *px, *py;
-    if (x.isArrayElement())
-    {
-        vx           = *x.varying;
-        vx.arraySize = 0;
-        px           = &vx;
-    }
-    else
-    {
-        px = x.varying;
-    }
-
-    if (y.isArrayElement())
-    {
-        vy           = *y.varying;
-        vy.arraySize = 0;
-        py           = &vy;
-    }
-    else
-    {
-        py = y.varying;
-    }
-
-    return gl::CompareShaderVar(*px, *py);
-}
 
 template <typename VarT>
 GLuint GetResourceIndexFromName(const std::vector<VarT> &list, const std::string &name)
@@ -888,9 +857,6 @@ Error Program::link(const gl::Context *context)
 
         linkOutputVariables(context);
 
-        // Validate we can pack the varyings.
-        std::vector<PackedVarying> packedVaryings = getPackedVaryings(mergedVaryings);
-
         // Map the varyings to the register file
         // In WebGL, we use a slightly different handling for packing variables.
         auto packMode = data.getExtensions().webglCompatibility ? PackMode::WEBGL_STRICT
@@ -904,13 +870,13 @@ Error Program::link(const gl::Context *context)
         InitUniformBlockLinker(context, mState, &resources.uniformBlockLinker);
         InitShaderStorageBlockLinker(context, mState, &resources.shaderStorageBlockLinker);
 
-        if (!resources.varyingPacking.packUserVaryings(mInfoLog, packedVaryings,
-                                                       mState.getTransformFeedbackVaryingNames()))
+        if (!linkValidateTransformFeedback(context, mInfoLog, mergedVaryings, caps))
         {
             return NoError();
         }
 
-        if (!linkValidateTransformFeedback(context, mInfoLog, mergedVaryings, caps))
+        if (!resources.varyingPacking.collectAndPackUserVaryings(
+                mInfoLog, mergedVaryings, mState.getTransformFeedbackVaryingNames()))
         {
             return NoError();
         }
@@ -2643,9 +2609,12 @@ bool Program::linkValidateTransformFeedback(const gl::Context *context,
             infoLog << "Capture of array elements is undefined and not supported.";
             return false;
         }
-        // All transform feedback varyings are expected to exist since packUserVaryings checks for
-        // them.
-        ASSERT(found);
+        if (!found)
+        {
+            infoLog << "Transform feedback varying " << tfVaryingName
+                    << " does not exist in the vertex shader.";
+            return false;
+        }
     }
 
     if (mState.mTransformFeedbackBufferMode == GL_INTERLEAVED_ATTRIBS &&
@@ -2733,85 +2702,6 @@ Program::MergedVaryings Program::getMergedVaryings(const Context *context) const
     return merged;
 }
 
-std::vector<PackedVarying> Program::getPackedVaryings(
-    const Program::MergedVaryings &mergedVaryings) const
-{
-    const std::vector<std::string> &tfVaryings = mState.getTransformFeedbackVaryingNames();
-    std::vector<PackedVarying> packedVaryings;
-    std::set<std::string> uniqueFullNames;
-
-    for (const auto &ref : mergedVaryings)
-    {
-        const sh::Varying *input  = ref.second.vertex;
-        const sh::Varying *output = ref.second.fragment;
-
-        // Only pack varyings that have a matched input or output, plus special builtins.
-        if ((input && output) || (output && output->isBuiltIn()))
-        {
-            // Will get the vertex shader interpolation by default.
-            auto interpolation = ref.second.get()->interpolation;
-
-            // Note that we lose the vertex shader static use information here. The data for the
-            // variable is taken from the fragment shader.
-            if (output->isStruct())
-            {
-                ASSERT(!output->isArray());
-                for (const auto &field : output->fields)
-                {
-                    ASSERT(!field.isStruct() && !field.isArray());
-                    packedVaryings.push_back(PackedVarying(field, interpolation, output->name));
-                }
-            }
-            else
-            {
-                packedVaryings.push_back(PackedVarying(*output, interpolation));
-            }
-            continue;
-        }
-
-        // Keep Transform FB varyings in the merged list always.
-        if (!input)
-        {
-            continue;
-        }
-
-        for (const std::string &tfVarying : tfVaryings)
-        {
-            std::vector<unsigned int> subscripts;
-            std::string baseName = ParseResourceName(tfVarying, &subscripts);
-            size_t subscript     = GL_INVALID_INDEX;
-            if (!subscripts.empty())
-            {
-                subscript = subscripts.back();
-            }
-            if (uniqueFullNames.count(tfVarying) > 0)
-            {
-                continue;
-            }
-            if (baseName == input->name)
-            {
-                // Transform feedback for varying structs is underspecified.
-                // See Khronos bug 9856.
-                // TODO(jmadill): Figure out how to be spec-compliant here.
-                if (!input->isStruct())
-                {
-                    packedVaryings.push_back(PackedVarying(*input, input->interpolation));
-                    packedVaryings.back().vertexOnly = true;
-                    packedVaryings.back().arrayIndex = static_cast<GLuint>(subscript);
-                    uniqueFullNames.insert(tfVarying);
-                }
-                if (subscript == GL_INVALID_INDEX)
-                {
-                    break;
-                }
-            }
-        }
-    }
-
-    std::sort(packedVaryings.begin(), packedVaryings.end(), ComparePackedVarying);
-
-    return packedVaryings;
-}
 
 void Program::linkOutputVariables(const Context *context)
 {

src/libANGLE/Program.h

@@ -49,7 +49,6 @@ class State;
 class InfoLog;
 class Buffer;
 class Framebuffer;
-struct PackedVarying;
 
 extern const char * const g_fakepath;
 
@@ -604,9 +603,6 @@ class Program final : angle::NonCopyable, public LabeledObject
     int getNumViews() const { return mState.getNumViews(); }
     bool usesMultiview() const { return mState.usesMultiview(); }
 
-  private:
-    ~Program();
-
     struct VaryingRef
     {
         const sh::Varying *get() const { return vertex ? vertex : fragment; }
@@ -614,9 +610,11 @@ class Program final : angle::NonCopyable, public LabeledObject
         const sh::Varying *vertex   = nullptr;
         const sh::Varying *fragment = nullptr;
     };
-
     using MergedVaryings = std::map<std::string, VaryingRef>;
 
+  private:
+    ~Program();
+
     void unlink();
 
     bool linkAttributes(const Context *context, InfoLog &infoLog);
@@ -654,7 +652,6 @@ class Program final : angle::NonCopyable, public LabeledObject
     void gatherTransformFeedbackVaryings(const MergedVaryings &varyings);
 
     MergedVaryings getMergedVaryings(const Context *context) const;
-    std::vector<PackedVarying> getPackedVaryings(const MergedVaryings &mergedVaryings) const;
     void linkOutputVariables(const Context *context);
 
     void setUniformValuesFromBindingQualifiers();

src/libANGLE/VaryingPacking.cpp

@@ -13,10 +13,48 @@
 
 #include "common/utilities.h"
 #include "libANGLE/Program.h"
+#include "libANGLE/Shader.h"
 
 namespace gl
 {
 
+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.
+    sh::ShaderVariable vx, vy;
+    const sh::ShaderVariable *px, *py;
+    if (x.isArrayElement())
+    {
+        vx           = *x.varying;
+        vx.arraySize = 0;
+        px           = &vx;
+    }
+    else
+    {
+        px = x.varying;
+    }
+
+    if (y.isArrayElement())
+    {
+        vy           = *y.varying;
+        vy.arraySize = 0;
+        py           = &vy;
+    }
+    else
+    {
+        py = y.varying;
+    }
+
+    return gl::CompareShaderVar(*px, *py);
+}
+
+}  // anonymous namespace
+
 // Implementation of VaryingPacking
 VaryingPacking::VaryingPacking(GLuint maxVaryingVectors, PackMode packMode)
     : mRegisterMap(maxVaryingVectors), mPackMode(packMode)
@@ -226,74 +264,107 @@ void VaryingPacking::insert(unsigned int registerRow,
     }
 }
 
-// See comment on packVarying.
-bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
-                                      const std::vector<PackedVarying> &packedVaryings,
-                                      const std::vector<std::string> &transformFeedbackVaryings)
+bool VaryingPacking::collectAndPackUserVaryings(gl::InfoLog &infoLog,
+                                                const Program::MergedVaryings &mergedVaryings,
+                                                const std::vector<std::string> &tfVaryings)
 {
-    std::set<std::string> uniqueVaryingNames;
+    std::set<std::string> uniqueFullNames;
+    mPackedVaryings.clear();
 
-    // "Variables are packed into the registers one at a time so that they each occupy a contiguous
-    // subrectangle. No splitting of variables is permitted."
-    for (const PackedVarying &packedVarying : packedVaryings)
+    for (const auto &ref : mergedVaryings)
     {
-        const auto &varying = *packedVarying.varying;
+        const sh::Varying *input  = ref.second.vertex;
+        const sh::Varying *output = ref.second.fragment;
 
-        // Do not assign registers to built-in or unreferenced varyings
-        if (!varying.staticUse && !packedVarying.isStructField())
+        // Only pack statically used varyings that have a matched input or output, plus special
+        // builtins.
+        if (((input && output) || (output && output->isBuiltIn())) && output->staticUse)
         {
+            // Will get the vertex shader interpolation by default.
+            auto interpolation = ref.second.get()->interpolation;
+
+            // Note that we lose the vertex shader static use information here. The data for the
+            // variable is taken from the fragment shader.
+            if (output->isStruct())
+            {
+                ASSERT(!output->isArray());
+                for (const auto &field : output->fields)
+                {
+                    ASSERT(!field.isStruct() && !field.isArray());
+                    mPackedVaryings.push_back(PackedVarying(field, interpolation, output->name));
+                    uniqueFullNames.insert(mPackedVaryings.back().nameWithArrayIndex());
+                }
+            }
+            else
+            {
+                mPackedVaryings.push_back(PackedVarying(*output, interpolation));
+                uniqueFullNames.insert(mPackedVaryings.back().nameWithArrayIndex());
+            }
             continue;
         }
 
-        ASSERT(!varying.isStruct());
-        ASSERT(uniqueVaryingNames.count(packedVarying.nameWithArrayIndex()) == 0);
-
-        if (packVarying(packedVarying))
+        // Keep Transform FB varyings in the merged list always.
+        if (!input)
         {
-            uniqueVaryingNames.insert(packedVarying.nameWithArrayIndex());
-        }
-        else
-        {
-            infoLog << "Could not pack varying " << packedVarying.nameWithArrayIndex();
-            return false;
+            continue;
         }
-    }
-
-    // Make sure transform feedback varyings aren't optimized out.
-    for (const std::string &transformFeedbackVaryingName : transformFeedbackVaryings)
-    {
-        std::string tfVaryingBaseName = ParseResourceName(transformFeedbackVaryingName, nullptr);
 
-        bool found = (uniqueVaryingNames.count(transformFeedbackVaryingName) > 0 ||
-                      uniqueVaryingNames.count(tfVaryingBaseName) > 0);
-
-        if (!found)
+        for (const std::string &tfVarying : tfVaryings)
         {
-            for (const PackedVarying &packedVarying : packedVaryings)
+            std::vector<unsigned int> subscripts;
+            std::string baseName = ParseResourceName(tfVarying, &subscripts);
+            size_t subscript     = GL_INVALID_INDEX;
+            if (!subscripts.empty())
+            {
+                subscript = subscripts.back();
+            }
+            // Already packed for fragment shader.
+            if (uniqueFullNames.count(tfVarying) > 0 || uniqueFullNames.count(baseName) > 0)
+            {
+                continue;
+            }
+            // Array as a whole and array element conflict has already been checked in
+            // linkValidateTransformFeedback.
+            if (baseName == input->name)
             {
-                const auto &varying = *packedVarying.varying;
-                if (tfVaryingBaseName == varying.name)
+                // Transform feedback for varying structs is underspecified.
+                // See Khronos bug 9856.
+                // TODO(jmadill): Figure out how to be spec-compliant here.
+                if (!input->isStruct() && tfVarying.compare(0, 3, "gl_") != 0)
+                {
+                    mPackedVaryings.push_back(PackedVarying(*input, input->interpolation));
+                    mPackedVaryings.back().vertexOnly = true;
+                    mPackedVaryings.back().arrayIndex = static_cast<GLuint>(subscript);
+                    uniqueFullNames.insert(tfVarying);
+                }
+                // Continue to match next array element for 'input' if the current match is array
+                // element.
+                if (subscript == GL_INVALID_INDEX)
                 {
-                    // only pack varyings that are not builtins.
-                    if (transformFeedbackVaryingName.compare(0, 3, "gl_") != 0)
-                    {
-                        if (!packVarying(packedVarying))
-                        {
-                            infoLog << "Could not pack varying " << varying.name;
-                            return false;
-                        }
-                        uniqueVaryingNames.insert(packedVarying.nameWithArrayIndex());
-                    }
-                    found = true;
                     break;
                 }
             }
         }
+    }
+
+    std::sort(mPackedVaryings.begin(), mPackedVaryings.end(), ComparePackedVarying);
+
+    return packUserVaryings(infoLog, mPackedVaryings, tfVaryings);
+}
+
+// See comment on packVarying.
+bool VaryingPacking::packUserVaryings(gl::InfoLog &infoLog,
+                                      const std::vector<PackedVarying> &packedVaryings,
+                                      const std::vector<std::string> &transformFeedbackVaryings)
+{
 
-        if (!found)
+    // "Variables are packed into the registers one at a time so that they each occupy a contiguous
+    // subrectangle. No splitting of variables is permitted."
+    for (const PackedVarying &packedVarying : packedVaryings)
+    {
+        if (!packVarying(packedVarying))
         {
-            infoLog << "Transform feedback varying " << transformFeedbackVaryingName
-                    << " does not exist in the vertex shader.";
+            infoLog << "Could not pack varying " << packedVarying.nameWithArrayIndex();
             return false;
         }
     }

src/libANGLE/VaryingPacking.h

@@ -16,6 +16,7 @@
 
 #include "angle_gl.h"
 #include "common/angleutils.h"
+#include "libANGLE/Program.h"
 
 namespace gl
 {
@@ -133,7 +134,11 @@ class VaryingPacking final : angle::NonCopyable
 
     bool packUserVaryings(gl::InfoLog &infoLog,
                           const std::vector<PackedVarying> &packedVaryings,
-                          const std::vector<std::string> &transformFeedbackVaryings);
+                          const std::vector<std::string> &tfVaryings);
+
+    bool collectAndPackUserVaryings(gl::InfoLog &infoLog,
+                                    const Program::MergedVaryings &mergedVaryings,
+                                    const std::vector<std::string> &tfVaryings);
 
     struct Register
     {
@@ -168,6 +173,7 @@ class VaryingPacking final : angle::NonCopyable
 
     std::vector<Register> mRegisterMap;
     std::vector<PackedVaryingRegister> mRegisterList;
+    std::vector<PackedVarying> mPackedVaryings;
 
     PackMode mPackMode;
 };

src/libANGLE/VaryingPacking_unittest.cpp

@@ -7,11 +7,13 @@
 //   Tests for ANGLE's internal varying packing algorithm.
 //
 
-#include "libANGLE/VaryingPacking.h"
-
 #include <gtest/gtest.h>
+// 'None' is defined as 'struct None {};' in
+// third_party/googletest/src/googletest/include/gtest/internal/gtest-type-util.h.
+// But 'None' is also define as a numberic constant 0L in <X11/X.h>.
+// So we need to include gtest first to avoid such conflict.
 
-#include "libANGLE/Program.h"
+#include "libANGLE/VaryingPacking.h"
 
 using namespace gl;