Clean up redundant initialization of gl_Position

src/compiler/translator/Compiler.cpp

@@ -226,6 +226,7 @@ TShHandleBase::~TShHandleBase()
 
 TCompiler::TCompiler(sh::GLenum type, ShShaderSpec spec, ShShaderOutput output)
     : variablesCollected(false),
+      mGLPositionInitialized(false),
       shaderType(type),
       shaderSpec(spec),
       outputType(output),
@@ -431,12 +432,6 @@ TIntermBlock *TCompiler::compileTreeImpl(const char *const shaderStrings[],
             DeclareAndInitBuiltinsForInstancedMultiview(root, getNumViews());
         }
 
-        // gl_Position is always written in compatibility output mode
-        if (success && shaderType == GL_VERTEX_SHADER &&
-            ((compileOptions & SH_INIT_GL_POSITION) ||
-             (outputType == SH_GLSL_COMPATIBILITY_OUTPUT)))
-            initializeGLPosition(root);
-
         // This pass might emit short circuits so keep it before the short circuit unfolding
         if (success && (compileOptions & SH_REWRITE_DO_WHILE_LOOPS))
             RewriteDoWhile(root, getTemporaryIndex());
@@ -487,6 +482,17 @@ TIntermBlock *TCompiler::compileTreeImpl(const char *const shaderStrings[],
             }
         }
 
+        // gl_Position is always written in compatibility output mode.
+        // It may have been already initialized among other output variables, in that case we don't
+        // need to initialize it twice.
+        if (success && shaderType == GL_VERTEX_SHADER && !mGLPositionInitialized &&
+            ((compileOptions & SH_INIT_GL_POSITION) ||
+             (outputType == SH_GLSL_COMPATIBILITY_OUTPUT)))
+        {
+            initializeGLPosition(root);
+            mGLPositionInitialized = true;
+        }
+
         // Removing invariant declarations must be done after collecting variables.
         // Otherwise, built-in invariant declarations don't apply.
         if (success && RemoveInvariant(shaderType, shaderVersion, outputType, compileOptions))
@@ -727,6 +733,7 @@ void TCompiler::clearResults()
     varyings.clear();
     interfaceBlocks.clear();
     variablesCollected = false;
+    mGLPositionInitialized = false;
 
     mNumViews = -1;
 
@@ -964,6 +971,11 @@ void TCompiler::initializeOutputVariables(TIntermBlock *root)
         for (auto var : varyings)
         {
             list.push_back(var);
+            if (var.name == "gl_Position")
+            {
+                ASSERT(!mGLPositionInitialized);
+                mGLPositionInitialized = true;
+            }
         }
     }
     else

src/compiler/translator/Compiler.h

@@ -187,6 +187,8 @@ class TCompiler : public TShHandleBase
 
     bool variablesCollected;
 
+    bool mGLPositionInitialized;
+
     // Removes unused function declarations and prototypes from the AST
     class UnusedPredicate;
     bool pruneUnusedFunctions(TIntermBlock *root);

src/tests/compiler_tests/InitOutputVariables_test.cpp

@@ -82,65 +82,6 @@ ExpectedLValues CreateIndexedLValueNodeList(const TString &lValueName,
     return expected;
 }
 
-void ReleaseExpectedLValuesMemory(ExpectedLValues &expectedLValues)
-{
-    for (size_t i = 0u; i < expectedLValues.size(); ++i)
-    {
-        delete expectedLValues[i];
-    }
-    expectedLValues.clear();
-}
-
-class ConstantsAreZerosTraverser final : public TIntermTraverser
-{
-  public:
-    ConstantsAreZerosTraverser() : TIntermTraverser(true, false, false), mAllConstantsAreZeros(true)
-    {
-    }
-
-    void visitConstantUnion(TIntermConstantUnion *node)
-    {
-        if (!mAllConstantsAreZeros)
-        {
-            return;
-        }
-
-        const TType &type = node->getType();
-        size_t objectSize = type.getObjectSize();
-        for (size_t i = 0u; i < objectSize && mAllConstantsAreZeros; ++i)
-        {
-            switch (type.getBasicType())
-            {
-                case EbtFloat:
-                    mAllConstantsAreZeros = (node->getFConst(i) == 0.0f);
-                    break;
-                case EbtInt:
-                    mAllConstantsAreZeros = (node->getIConst(i) == 0);
-                    break;
-                case EbtUInt:
-                    mAllConstantsAreZeros = (node->getUConst(i) == 0u);
-                    break;
-                default:
-                    // Cannot handle.
-                    mAllConstantsAreZeros = false;
-            }
-        }
-    }
-
-    bool areAllConstantsZeros() const { return mAllConstantsAreZeros; }
-
-  private:
-    bool mAllConstantsAreZeros;
-};
-
-// Returns true if all visited constants in subtree are zeros.
-bool AreAllConstantsInSubtreeZeros(TIntermNode *subtree)
-{
-    ConstantsAreZerosTraverser traverser;
-    subtree->traverse(&traverser);
-    return traverser.areAllConstantsZeros();
-}
-
 // VerifyOutputVariableInitializers traverses the subtree covering main and collects the lvalues in
 // assignments for which the rvalue is an expression containing only zero constants.
 class VerifyOutputVariableInitializers final : public TIntermTraverser
@@ -159,7 +100,7 @@ class VerifyOutputVariableInitializers final : public TIntermTraverser
 
     bool visitBinary(Visit visit, TIntermBinary *node) override
     {
-        if (node->getOp() == EOpAssign && AreAllConstantsInSubtreeZeros(node->getRight()))
+        if (node->getOp() == EOpAssign && IsZero(node->getRight()))
         {
             mCandidateLValues.push_back(node->getLeft());
             return false;
@@ -234,10 +175,15 @@ class FindStructByName final : public TIntermTraverser
 class InitOutputVariablesWebGL2Test : public ShaderCompileTreeTest
 {
   public:
-    InitOutputVariablesWebGL2Test()
+    void SetUp() override
     {
         mExtraCompileOptions |= SH_VARIABLES;
         mExtraCompileOptions |= SH_INIT_OUTPUT_VARIABLES;
+        if (getShaderType() == GL_VERTEX_SHADER)
+        {
+            mExtraCompileOptions |= SH_INIT_GL_POSITION;
+        }
+        ShaderCompileTreeTest::SetUp();
     }
 
   protected:
@@ -302,7 +248,6 @@ TEST_F(InitOutputVariablesWebGL2VertexShaderTest, OutputAllQualifiers)
         CreateLValueNode("out3", TType(EbtFloat, EbpMedium, EvqCentroidOut)),
         CreateLValueNode("out4", TType(EbtFloat, EbpMedium, EvqSmoothOut))};
     EXPECT_TRUE(verifier.areAllExpectedLValuesFound(expectedLValues));
-    ReleaseExpectedLValuesMemory(expectedLValues);
 }
 
 // Test the initialization of an output array in a vertex shader.
@@ -320,7 +265,6 @@ TEST_F(InitOutputVariablesWebGL2VertexShaderTest, OutputArray)
     ExpectedLValues expectedLValues =
         CreateIndexedLValueNodeList("out1", TType(EbtFloat, EbpMedium, EvqVertexOut), 2);
     EXPECT_TRUE(verifier.areAllExpectedLValuesFound(expectedLValues));
-    ReleaseExpectedLValuesMemory(expectedLValues);
 }
 
 // Test the initialization of a struct output variable in a vertex shader.
@@ -402,7 +346,6 @@ TEST_F(InitOutputVariablesWebGL2FragmentShaderTest, FragData)
         CreateIndexedLValueNodeList("gl_FragData", TType(EbtFloat, EbpMedium, EvqFragData, 4), 1);
     EXPECT_TRUE(verifier.isExpectedLValueFound(expectedLValues[0]));
     EXPECT_EQ(1u, verifier.getCandidates().size());
-    ReleaseExpectedLValuesMemory(expectedLValues);
 }
 
 // Test the initialization of gl_FragData in a WebGL1 ESSL1 fragment shader. Only writes to
@@ -421,7 +364,6 @@ TEST_F(InitOutputVariablesWebGL1FragmentShaderTest, FragData)
         CreateIndexedLValueNodeList("gl_FragData", TType(EbtFloat, EbpMedium, EvqFragData, 4), 1);
     EXPECT_TRUE(verifier.isExpectedLValueFound(expectedLValues[0]));
     EXPECT_EQ(1u, verifier.getCandidates().size());
-    ReleaseExpectedLValuesMemory(expectedLValues);
 }
 
 // Test the initialization of gl_FragData in a WebGL1 ESSL1 fragment shader with GL_EXT_draw_buffers
@@ -442,7 +384,43 @@ TEST_F(InitOutputVariablesWebGL1FragmentShaderTest, FragDataWithDrawBuffersExtEn
     EXPECT_TRUE(verifier.isExpectedLValueFound(expectedLValues[0]));
     EXPECT_TRUE(verifier.isExpectedLValueFound(expectedLValues[1]));
     EXPECT_EQ(2u, verifier.getCandidates().size());
-    ReleaseExpectedLValuesMemory(expectedLValues);
 }
 
-}  // namespace sh
+// Test that gl_Position is initialized once in case it is not statically used and both
\ No newline at end of file
+// SH_INIT_OUTPUT_VARIABLES and SH_INIT_GL_POSITION flags are set.
+TEST_F(InitOutputVariablesWebGL2VertexShaderTest, InitGLPositionWhenNotStaticallyUsed)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision highp float;\n"
+        "void main() {\n"
+        "}\n";
+    compileAssumeSuccess(shaderString);
+    VerifyOutputVariableInitializers verifier(mASTRoot);
+
+    TIntermTyped *glPosition =
+        CreateLValueNode("gl_Position", TType(EbtFloat, EbpHigh, EvqPosition, 4));
+    EXPECT_TRUE(verifier.isExpectedLValueFound(glPosition));
+    EXPECT_EQ(1u, verifier.getCandidates().size());
+}
+
+// Test that gl_Position is initialized once in case it is statically used and both
+// SH_INIT_OUTPUT_VARIABLES and SH_INIT_GL_POSITION flags are set.
+TEST_F(InitOutputVariablesWebGL2VertexShaderTest, InitGLPositionOnceWhenStaticallyUsed)
+{
+    const std::string &shaderString =
+        "#version 300 es\n"
+        "precision highp float;\n"
+        "void main() {\n"
+        "    gl_Position = vec4(1.0);\n"
+        "}\n";
+    compileAssumeSuccess(shaderString);
+    VerifyOutputVariableInitializers verifier(mASTRoot);
+
+    TIntermTyped *glPosition =
+        CreateLValueNode("gl_Position", TType(EbtFloat, EbpHigh, EvqPosition, 4));
+    EXPECT_TRUE(verifier.isExpectedLValueFound(glPosition));
+    EXPECT_EQ(1u, verifier.getCandidates().size());
+}
+
+}  // namespace sh

src/tests/test_utils/ShaderCompileTreeTest.cpp

@@ -11,7 +11,103 @@
 
 #include "compiler/translator/TranslatorESSL.h"
 
-using namespace sh;
+namespace sh
+{
+
+namespace
+{
+
+// Checks that the node traversed is a zero node. It can be made out of multiple constructors and
+// constant union nodes as long as there's no arithmetic involved and all constants are zero.
+class OnlyContainsZeroConstantsTraverser final : public TIntermTraverser
+{
+  public:
+    OnlyContainsZeroConstantsTraverser()
+        : TIntermTraverser(true, false, false), mOnlyContainsConstantZeros(true)
+    {
+    }
+
+    bool visitUnary(Visit, TIntermUnary *node) override
+    {
+        mOnlyContainsConstantZeros = false;
+        return false;
+    }
+
+    bool visitBinary(Visit, TIntermBinary *node) override
+    {
+        mOnlyContainsConstantZeros = false;
+        return false;
+    }
+
+    bool visitTernary(Visit, TIntermTernary *node) override
+    {
+        mOnlyContainsConstantZeros = false;
+        return false;
+    }
+
+    bool visitSwizzle(Visit, TIntermSwizzle *node) override
+    {
+        mOnlyContainsConstantZeros = false;
+        return false;
+    }
+
+    bool visitAggregate(Visit, TIntermAggregate *node) override
+    {
+        if (node->getOp() != EOpConstruct)
+        {
+            mOnlyContainsConstantZeros = false;
+            return false;
+        }
+        return true;
+    }
+
+    void visitSymbol(TIntermSymbol *node) override { mOnlyContainsConstantZeros = false; }
+
+    void visitConstantUnion(TIntermConstantUnion *node) override
+    {
+        if (!mOnlyContainsConstantZeros)
+        {
+            return;
+        }
+
+        const TType &type = node->getType();
+        size_t objectSize = type.getObjectSize();
+        for (size_t i = 0u; i < objectSize && mOnlyContainsConstantZeros; ++i)
+        {
+            bool isZero = false;
+            switch (type.getBasicType())
+            {
+                case EbtFloat:
+                    isZero = (node->getFConst(i) == 0.0f);
+                    break;
+                case EbtInt:
+                    isZero = (node->getIConst(i) == 0);
+                    break;
+                case EbtUInt:
+                    isZero = (node->getUConst(i) == 0u);
+                    break;
+                case EbtBool:
+                    isZero = (node->getBConst(i) == false);
+                    break;
+                default:
+                    // Cannot handle.
+                    break;
+            }
+            if (!isZero)
+            {
+                mOnlyContainsConstantZeros = false;
+                return;
+            }
+        }
+    }
+
+    bool onlyContainsConstantZeros() const { return mOnlyContainsConstantZeros; }
+
+  private:
+    bool mOnlyContainsConstantZeros;
+};
+
+}  // anonymous namespace
 
 void ShaderCompileTreeTest::SetUp()
 {
@@ -61,4 +157,17 @@ const std::vector<sh::Uniform> ShaderCompileTreeTest::getUniforms()
 {
     ASSERT(mExtraCompileOptions & SH_VARIABLES);
     return mTranslator->getUniforms();
-}
+}
\ No newline at end of file
+
+bool IsZero(TIntermNode *node)
+{
+    if (!node->getAsTyped())
+    {
+        return false;
+    }
+    OnlyContainsZeroConstantsTraverser traverser;
+    node->traverse(&traverser);
+    return traverser.onlyContainsConstantZeros();
+}
+
+}  // namespace sh

src/tests/test_utils/ShaderCompileTreeTest.h

@@ -19,6 +19,7 @@ namespace sh
 {
 
 class TIntermBlock;
+class TIntermNode;
 class TranslatorESSL;
 
 class ShaderCompileTreeTest : public testing::Test
@@ -54,6 +55,10 @@ class ShaderCompileTreeTest : public testing::Test
     TPoolAllocator mAllocator;
 };
 
+// Returns true if the node is some kind of a zero node - either constructor or a constant union
+// node.
+bool IsZero(TIntermNode *node);
+
 }  // namespace sh
 
 #endif  // TESTS_TEST_UTILS_SHADER_COMPILE_TREE_TEST_H_