Prune no-op statements with a single traverser

src/compiler.gypi

@@ -97,10 +97,8 @@
             'compiler/translator/PoolAlloc.cpp',
             'compiler/translator/PoolAlloc.h',
             'compiler/translator/Pragma.h',
-            'compiler/translator/PruneEmptyDeclarations.cpp',
-            'compiler/translator/PruneEmptyDeclarations.h',
-            'compiler/translator/PrunePureLiteralStatements.cpp',
-            'compiler/translator/PrunePureLiteralStatements.h',
+            'compiler/translator/PruneNoOps.cpp',
+            'compiler/translator/PruneNoOps.h',
             'compiler/translator/QualifierTypes.h',
             'compiler/translator/QualifierTypes.cpp',
             'compiler/translator/RecordConstantPrecision.cpp',

src/compiler/translator/Compiler.cpp

@@ -25,7 +25,7 @@
 #include "compiler/translator/IsASTDepthBelowLimit.h"
 #include "compiler/translator/OutputTree.h"
 #include "compiler/translator/ParseContext.h"
-#include "compiler/translator/PruneEmptyDeclarations.h"
+#include "compiler/translator/PruneNoOps.h"
 #include "compiler/translator/RegenerateStructNames.h"
 #include "compiler/translator/RemoveArrayLengthMethod.h"
 #include "compiler/translator/RemoveInvariantDeclaration.h"
@@ -375,17 +375,21 @@ TIntermBlock *TCompiler::compileTreeImpl(const char *const shaderStrings[],
         if (success && (compileOptions & SH_LIMIT_EXPRESSION_COMPLEXITY))
             success = limitExpressionComplexity(root);
 
-        // Prune empty declarations to work around driver bugs and to keep declaration output
-        // simple. We want to do this before most other operations since TIntermDeclaration nodes
-        // without any children are tricky to work with.
+        // We prune no-ops to work around driver bugs and to keep AST processing and output simple.
+        // The following kinds of no-ops are pruned:
+        //   1. Empty declarations "int;".
+        //   2. Literal statements: "1.0;". The ESSL output doesn't define a default precision
+        //      for float, so float literal statements would end up with no precision which is
+        //      invalid ESSL.
+        // After this empty declarations are not allowed in the AST.
         if (success)
-            PruneEmptyDeclarations(root);
+            PruneNoOps(root);
 
         // In case the last case inside a switch statement is a certain type of no-op, GLSL
         // compilers in drivers may not accept it. In this case we clean up the dead code from the
-        // end of switch statements. This is also required because PruneEmptyDeclarations may have
-        // left switch statements that only contained an empty declaration inside the final case in
-        // an invalid state. Relies on that PruneEmptyDeclarations has already been run.
+        // end of switch statements. This is also required because PruneNoOps may have left switch
+        // statements that only contained an empty declaration inside the final case in an invalid
+        // state. Relies on that PruneNoOps has already been run.
         if (success)
             RemoveNoOpCasesFromEndOfSwitchStatements(root, &symbolTable);
 

src/compiler/translator/IntermNode.h

@@ -216,6 +216,7 @@ class TIntermLoop : public TIntermNode
     TIntermTyped *getExpression() { return mExpr; }
     TIntermBlock *getBody() { return mBody; }
 
+    void setInit(TIntermNode *init) { mInit = init; }
     void setCondition(TIntermTyped *condition) { mCond = condition; }
     void setExpression(TIntermTyped *expression) { mExpr = expression; }
     void setBody(TIntermBlock *body) { mBody = body; }

src/compiler/translator/PruneEmptyDeclarations.h

-//
-// Copyright (c) 2002-2015 The ANGLE Project Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-//
-// The PruneEmptyDeclarations function prunes unnecessary empty declarations and declarators from
-// the AST.
-
-#ifndef COMPILER_TRANSLATOR_PRUNEEMPTYDECLARATIONS_H_
-#define COMPILER_TRANSLATOR_PRUNEEMPTYDECLARATIONS_H_
-
-namespace sh
-{
-class TIntermBlock;
-
-void PruneEmptyDeclarations(TIntermBlock *root);
-}
-
-#endif  // COMPILER_TRANSLATOR_PRUNEEMPTYDECLARATIONS_H_

src/compiler/translator/PruneEmptyDeclarations.cpp → src/compiler/translator/PruneNoOps.cpp

@@ -3,10 +3,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// The PruneEmptyDeclarations function prunes unnecessary empty declarations and declarators from
-// the AST.
+// PruneNoOps.cpp: The PruneNoOps function prunes:
+//   1. Empty declarations "int;". Empty declarators will be pruned as well, so for example:
+//        int , a;
+//      is turned into
+//        int a;
+//   2. Literal statements: "1.0;". The ESSL output doesn't define a default precision for float,
+//      so float literal statements would end up with no precision which is invalid ESSL.
 
-#include "compiler/translator/PruneEmptyDeclarations.h"
+#include "compiler/translator/PruneNoOps.h"
 
 #include "compiler/translator/IntermTraverse.h"
 
@@ -16,29 +21,45 @@ namespace sh
 namespace
 {
 
-class PruneEmptyDeclarationsTraverser : private TIntermTraverser
+bool IsNoOp(TIntermNode *node)
+{
+    if (node->getAsConstantUnion() != nullptr)
+    {
+        return true;
+    }
+    bool isEmptyDeclaration = node->getAsDeclarationNode() != nullptr &&
+                              node->getAsDeclarationNode()->getSequence()->empty();
+    if (isEmptyDeclaration)
+    {
+        return true;
+    }
+    return false;
+}
+
+class PruneNoOpsTraverser : private TIntermTraverser
 {
   public:
     static void apply(TIntermBlock *root);
 
   private:
-    PruneEmptyDeclarationsTraverser();
+    PruneNoOpsTraverser();
     bool visitDeclaration(Visit, TIntermDeclaration *node) override;
+    bool visitBlock(Visit visit, TIntermBlock *node) override;
+    bool visitLoop(Visit visit, TIntermLoop *loop) override;
 };
 
-void PruneEmptyDeclarationsTraverser::apply(TIntermBlock *root)
+void PruneNoOpsTraverser::apply(TIntermBlock *root)
 {
-    PruneEmptyDeclarationsTraverser prune;
+    PruneNoOpsTraverser prune;
     root->traverse(&prune);
     prune.updateTree();
 }
 
-PruneEmptyDeclarationsTraverser::PruneEmptyDeclarationsTraverser()
-    : TIntermTraverser(true, false, false)
+PruneNoOpsTraverser::PruneNoOpsTraverser() : TIntermTraverser(true, false, false)
 {
 }
 
-bool PruneEmptyDeclarationsTraverser::visitDeclaration(Visit, TIntermDeclaration *node)
+bool PruneNoOpsTraverser::visitDeclaration(Visit, TIntermDeclaration *node)
 {
     TIntermSequence *sequence = node->getSequence();
     if (sequence->size() >= 1)
@@ -61,23 +82,22 @@ bool PruneEmptyDeclarationsTraverser::visitDeclaration(Visit, TIntermDeclaration
             }
             else if (sym->getBasicType() != EbtStruct)
             {
-                // Single struct declarations may just declare the struct type and no variables, so
-                // they should not be pruned. If there are entirely empty non-struct declarations,
-                // they result in TIntermDeclaration nodes without any children in the parsing
-                // stage. This will be handled further down in the code.
+                // If there are entirely empty non-struct declarations, they result in
+                // TIntermDeclaration nodes without any children in the parsing stage. These are
+                // handled in visitBlock and visitLoop.
                 UNREACHABLE();
             }
             else if (sym->getType().getQualifier() != EvqGlobal &&
                      sym->getType().getQualifier() != EvqTemporary)
             {
-                // We've hit an empty struct declaration with a qualifier, for example like
-                // this:
-                // const struct a { int i; };
-                // NVIDIA GL driver version 367.27 doesn't accept this kind of declarations, so
-                // we convert the declaration to a regular struct declaration. This is okay,
-                // since ESSL 1.00 spec section 4.1.8 says about structs that "The optional
-                // qualifiers only apply to any declarators, and are not part of the type being
-                // defined for name."
+                // Single struct declarations may just declare the struct type and no variables, so
+                // they should not be pruned. Here we handle an empty struct declaration with a
+                // qualifier, for example like this:
+                //   const struct a { int i; };
+                // NVIDIA GL driver version 367.27 doesn't accept this kind of declarations, so we
+                // convert the declaration to a regular struct declaration. This is okay, since ESSL
+                // 1.00 spec section 4.1.8 says about structs that "The optional qualifiers only
+                // apply to any declarators, and are not part of the type being defined for name."
 
                 if (mInGlobalScope)
                 {
@@ -90,31 +110,47 @@ bool PruneEmptyDeclarationsTraverser::visitDeclaration(Visit, TIntermDeclaration
             }
         }
     }
-    else
+    return false;
+}
+
+bool PruneNoOpsTraverser::visitBlock(Visit visit, TIntermBlock *node)
+{
+    TIntermSequence *statements = node->getSequence();
+
+    for (TIntermNode *statement : *statements)
     {
-        // We have a declaration with no declarators.
-        // The declaration may be either inside a block or in a loop init expression.
-        TIntermBlock *parentAsBlock = getParentNode()->getAsBlock();
-        ASSERT(parentAsBlock != nullptr || getParentNode()->getAsLoopNode() != nullptr);
-        if (parentAsBlock)
+        if (IsNoOp(statement))
         {
             TIntermSequence emptyReplacement;
             mMultiReplacements.push_back(
-                NodeReplaceWithMultipleEntry(parentAsBlock, node, emptyReplacement));
-        }
-        else
-        {
-            queueReplacement(nullptr, OriginalNode::IS_DROPPED);
+                NodeReplaceWithMultipleEntry(node, statement, emptyReplacement));
         }
     }
-    return false;
+
+    return true;
+}
+
+bool PruneNoOpsTraverser::visitLoop(Visit visit, TIntermLoop *loop)
+{
+    TIntermTyped *expr = loop->getExpression();
+    if (expr != nullptr && IsNoOp(expr))
+    {
+        loop->setExpression(nullptr);
+    }
+    TIntermNode *init = loop->getInit();
+    if (init != nullptr && IsNoOp(init))
+    {
+        loop->setInit(nullptr);
+    }
+
+    return true;
 }
 
 }  // namespace
 
-void PruneEmptyDeclarations(TIntermBlock *root)
+void PruneNoOps(TIntermBlock *root)
 {
-    PruneEmptyDeclarationsTraverser::apply(root);
+    PruneNoOpsTraverser::apply(root);
 }
 
 }  // namespace sh

src/compiler/translator/PruneNoOps.h

+//
+// Copyright (c) 2002-2015 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// PruneNoOps.h: The PruneNoOps function prunes:
+//   1. Empty declarations "int;". Empty declarators will be pruned as well, so for example:
+//        int , a;
+//      is turned into
+//        int a;
+//   2. Literal statements: "1.0;". The ESSL output doesn't define a default precision for float,
+//      so float literal statements would end up with no precision which is invalid ESSL.
+
+#ifndef COMPILER_TRANSLATOR_PRUNENOOPS_H_
+#define COMPILER_TRANSLATOR_PRUNENOOPS_H_
+
+namespace sh
+{
+class TIntermBlock;
+
+void PruneNoOps(TIntermBlock *root);
+}
+
+#endif  // COMPILER_TRANSLATOR_PRUNENOOPS_H_

src/compiler/translator/PrunePureLiteralStatements.cpp

-//
-// Copyright (c) 2017 The ANGLE Project Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-//
-// PrunePureLiteralStatements.cpp: Removes statements that are literals and nothing else.
-
-#include "compiler/translator/PrunePureLiteralStatements.h"
-
-#include "compiler/translator/IntermTraverse.h"
-
-namespace sh
-{
-
-namespace
-{
-
-class PrunePureLiteralStatementsTraverser : public TIntermTraverser
-{
-  public:
-    PrunePureLiteralStatementsTraverser() : TIntermTraverser(true, false, false) {}
-
-    bool visitBlock(Visit visit, TIntermBlock *node) override
-    {
-        TIntermSequence *statements = node->getSequence();
-        if (statements == nullptr)
-        {
-            return false;
-        }
-
-        // Empty case statements at the end of a switch are invalid: if the last statements
-        // of a block was a pure literal, also delete all the case statements directly preceding it.
-        bool deleteCaseStatements = false;
-        for (int i = static_cast<int>(statements->size()); i-- > 0;)
-        {
-            TIntermNode *statement = (*statements)[i];
-
-            if (statement->getAsConstantUnion() != nullptr)
-            {
-                TIntermSequence emptyReplacement;
-                mMultiReplacements.push_back(
-                    NodeReplaceWithMultipleEntry(node, statement, emptyReplacement));
-
-                if (i == static_cast<int>(statements->size()) - 1)
-                {
-                    deleteCaseStatements = true;
-                }
-
-                continue;
-            }
-
-            if (deleteCaseStatements)
-            {
-                if (statement->getAsCaseNode() != nullptr)
-                {
-                    TIntermSequence emptyReplacement;
-                    mMultiReplacements.push_back(
-                        NodeReplaceWithMultipleEntry(node, statement, emptyReplacement));
-                }
-                else
-                {
-                    deleteCaseStatements = false;
-                }
-            }
-        }
-
-        return true;
-    }
-
-    bool visitLoop(Visit visit, TIntermLoop *loop) override
-    {
-        TIntermTyped *expr = loop->getExpression();
-        if (expr != nullptr && expr->getAsConstantUnion() != nullptr)
-        {
-            loop->setExpression(nullptr);
-        }
-
-        return true;
-    }
-};
-
-}  // namespace
-
-void PrunePureLiteralStatements(TIntermNode *root)
-{
-    PrunePureLiteralStatementsTraverser prune;
-    root->traverse(&prune);
-    prune.updateTree();
-}
-
-}  // namespace sh

src/compiler/translator/PrunePureLiteralStatements.h

-//
-// Copyright (c) 2017 The ANGLE Project Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-//
-// PrunePureLiteralStatements.h: Removes statements that are literals and nothing else.
-
-#ifndef COMPILER_TRANSLATOR_PRUNEPURELITERALSTATEMENTS_H_
-#define COMPILER_TRANSLATOR_PRUNEPURELITERALSTATEMENTS_H_
-
-namespace sh
-{
-class TIntermNode;
-
-void PrunePureLiteralStatements(TIntermNode *root);
-}
-
-#endif  // COMPILER_TRANSLATOR_PRUNEPURELITERALSTATEMENTS_H_

src/compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.cpp

@@ -36,11 +36,13 @@ bool IsEmptyBlock(TIntermNode *node)
     // here. Note that declarations for struct types do contain a nameless child node.
     ASSERT(node->getAsDeclarationNode() == nullptr ||
            !node->getAsDeclarationNode()->getSequence()->empty());
+    // Pure literal statements should also already be pruned.
+    ASSERT(node->getAsConstantUnion() == nullptr);
     return false;
 }
 
 // Return true if all statements in "statements" starting from index i consist only of empty blocks
-// and empty declarations. Returns true also if there are no statements.
+// and no-op statements. Returns true also if there are no statements.
 bool AreEmptyBlocks(TIntermSequence *statements, size_t i)
 {
     for (; i < statements->size(); ++i)

src/compiler/translator/RemoveSwitchFallThrough.cpp

@@ -97,10 +97,10 @@ void RemoveSwitchFallThroughTraverser::visitSymbol(TIntermSymbol *node)
 
 void RemoveSwitchFallThroughTraverser::visitConstantUnion(TIntermConstantUnion *node)
 {
-    // Conditions of case labels are not traversed, so this is some other constant
-    // Could be just a statement like "0;"
-    mPreviousCase->getSequence()->push_back(node);
-    mLastStatementWasBreak = false;
+    // Conditions of case labels are not traversed, so this is a constant statement like "0;".
+    // These are no-ops so there's no need to add them back to the statement list. Should have
+    // already been pruned out of the AST, in fact.
+    UNREACHABLE();
 }
 
 bool RemoveSwitchFallThroughTraverser::visitDeclaration(Visit, TIntermDeclaration *node)

src/compiler/translator/TranslatorESSL.cpp

@@ -8,7 +8,6 @@
 
 #include "compiler/translator/BuiltInFunctionEmulatorGLSL.h"
 #include "compiler/translator/EmulatePrecision.h"
-#include "compiler/translator/PrunePureLiteralStatements.h"
 #include "compiler/translator/RecordConstantPrecision.h"
 #include "compiler/translator/OutputESSL.h"
 #include "angle_gl.h"
@@ -34,10 +33,6 @@ void TranslatorESSL::translate(TIntermBlock *root,
                                ShCompileOptions compileOptions,
                                PerformanceDiagnostics * /*perfDiagnostics*/)
 {
-    // The ESSL output doesn't define a default precision for float, so float literal statements
-    // end up with no precision which is invalid ESSL.
-    PrunePureLiteralStatements(root);
-
     TInfoSinkBase &sink = getInfoSink().obj;
 
     int shaderVer = getShaderVersion();