Clean up switch/case pruning code

src/compiler.gypi

@@ -123,8 +123,6 @@
             'compiler/translator/RemoveArrayLengthMethod.h',
             'compiler/translator/RemoveInvariantDeclaration.cpp',
             'compiler/translator/RemoveInvariantDeclaration.h',
-            'compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.cpp',
-            'compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.h',
             'compiler/translator/RemovePow.cpp',
             'compiler/translator/RemovePow.h',
             'compiler/translator/RemoveUnreferencedVariables.cpp',

src/compiler/translator/Compiler.cpp

@@ -31,7 +31,6 @@
 #include "compiler/translator/RegenerateStructNames.h"
 #include "compiler/translator/RemoveArrayLengthMethod.h"
 #include "compiler/translator/RemoveInvariantDeclaration.h"
-#include "compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.h"
 #include "compiler/translator/RemovePow.h"
 #include "compiler/translator/RemoveUnreferencedVariables.h"
 #include "compiler/translator/RewriteDoWhile.h"
@@ -466,13 +465,6 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
     // After this empty declarations are not allowed in the AST.
     PruneNoOps(root, &symbolTable);
 
-    // 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 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.
-    RemoveNoOpCasesFromEndOfSwitchStatements(root, &symbolTable);
-
     // Create the function DAG and check there is no recursion
     if (!initCallDag(root))
     {
@@ -585,6 +577,12 @@ bool TCompiler::checkAndSimplifyAST(TIntermBlock *root,
 
     RemoveUnreferencedVariables(root, &symbolTable);
 
+    // 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 PruneNoOps or RemoveUnreferencedVariables may have
+    // left switch statements that only contained an empty declaration inside the final case in an
+    // invalid state. Relies on that PruneNoOps and RemoveUnreferencedVariables have already been
+    // run.
     PruneEmptyCases(root);
 
     // Built-in function emulation needs to happen after validateLimitations pass.

src/compiler/translator/PruneEmptyCases.cpp

@@ -17,6 +17,38 @@ namespace sh
 namespace
 {
 
+bool AreEmptyBlocks(TIntermSequence *statements);
+
+bool IsEmptyBlock(TIntermNode *node)
+{
+    TIntermBlock *asBlock = node->getAsBlock();
+    if (asBlock)
+    {
+        return AreEmptyBlocks(asBlock->getSequence());
+    }
+    // Empty declarations should have already been pruned, otherwise they would need to be handled
+    // 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" consist only of empty blocks and no-op statements.
+// Returns true also if there are no statements.
+bool AreEmptyBlocks(TIntermSequence *statements)
+{
+    for (size_t i = 0u; i < statements->size(); ++i)
+    {
+        if (!IsEmptyBlock(statements->at(i)))
+        {
+            return false;
+        }
+    }
+    return true;
+}
+
 class PruneEmptyCasesTraverser : private TIntermTraverser
 {
   public:
@@ -40,31 +72,31 @@ PruneEmptyCasesTraverser::PruneEmptyCasesTraverser() : TIntermTraverser(true, fa
 
 bool PruneEmptyCasesTraverser::visitSwitch(Visit visit, TIntermSwitch *node)
 {
+    // This may mutate the statementList, but that's okay, since traversal has not yet reached
+    // there.
     TIntermBlock *statementList = node->getStatementList();
     TIntermSequence *statements = statementList->getSequence();
 
-    // Iterate block children in reverse order. Cases that are only followed by other cases are
-    // pruned.
-    size_t i                  = statements->size();
-    bool emptySwitchStatement = true;
+    // Iterate block children in reverse order. Cases that are only followed by other cases or empty
+    // blocks are marked for pruning.
+    size_t i                       = statements->size();
+    size_t lastNoOpInStatementList = i;
     while (i > 0)
     {
         --i;
         TIntermNode *statement = statements->at(i);
-        if (statement->getAsCaseNode())
+        if (statement->getAsCaseNode() || IsEmptyBlock(statement))
         {
-            TIntermSequence emptyReplacement;
-            mMultiReplacements.push_back(
-                NodeReplaceWithMultipleEntry(statementList, statement, emptyReplacement));
+            lastNoOpInStatementList = i;
         }
         else
         {
-            emptySwitchStatement = false;
             break;
         }
     }
-    if (emptySwitchStatement)
+    if (lastNoOpInStatementList == 0)
     {
+        // Remove the entire switch statement, extracting the init expression if needed.
         TIntermTyped *init = node->getInit();
         if (init->hasSideEffects())
         {
@@ -79,6 +111,10 @@ bool PruneEmptyCasesTraverser::visitSwitch(Visit visit, TIntermSwitch *node)
         }
         return false;
     }
+    if (lastNoOpInStatementList < statements->size())
+    {
+        statements->erase(statements->begin() + lastNoOpInStatementList, statements->end());
+    }
 
     return true;
 }

src/compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.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.
-//
-// RemoveNoOpCasesFromEndOfSwitchStatements.cpp: Clean up cases from the end of a switch statement
-// that only contain no-ops.
-
-#include "compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.h"
-
-#include "compiler/translator/IntermNode.h"
-#include "compiler/translator/IntermNode_util.h"
-#include "compiler/translator/IntermTraverse.h"
-#include "compiler/translator/SymbolTable.h"
-
-namespace sh
-{
-
-namespace
-{
-
-bool AreEmptyBlocks(TIntermSequence *statements, size_t i);
-
-bool IsEmptyBlock(TIntermNode *node)
-{
-    TIntermBlock *asBlock = node->getAsBlock();
-    if (asBlock)
-    {
-        if (asBlock->getSequence()->empty())
-        {
-            return true;
-        }
-        return AreEmptyBlocks(asBlock->getSequence(), 0u);
-    }
-    // Empty declarations should have already been pruned, otherwise they would need to be handled
-    // 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 no-op statements. Returns true also if there are no statements.
-bool AreEmptyBlocks(TIntermSequence *statements, size_t i)
-{
-    for (; i < statements->size(); ++i)
-    {
-        if (!IsEmptyBlock(statements->at(i)))
-        {
-            return false;
-        }
-    }
-    return true;
-}
-
-void RemoveNoOpCasesFromEndOfStatementList(TIntermBlock *statementList, TSymbolTable *symbolTable)
-{
-    TIntermSequence *statements = statementList->getSequence();
-
-    bool foundDeadCase = false;
-    do
-    {
-        if (statements->empty())
-        {
-            return;
-        }
-
-        // Find the last case label.
-        size_t i = statements->size();
-        while (i > 0u && !(*statements)[i - 1]->getAsCaseNode())
-        {
-            --i;
-        }
-        // Now i is the index of the first statement following the last label inside the switch
-        // statement.
-        ASSERT(i > 0u);
-
-        foundDeadCase = AreEmptyBlocks(statements, i);
-        if (foundDeadCase)
-        {
-            statements->erase(statements->begin() + (i - 1u), statements->end());
-        }
-    } while (foundDeadCase);
-}
-
-class RemoveNoOpCasesFromEndOfSwitchTraverser : public TIntermTraverser
-{
-  public:
-    RemoveNoOpCasesFromEndOfSwitchTraverser(TSymbolTable *symbolTable)
-        : TIntermTraverser(true, false, false, symbolTable)
-    {
-    }
-
-    bool visitSwitch(Visit visit, TIntermSwitch *node) override;
-};
-
-bool RemoveNoOpCasesFromEndOfSwitchTraverser::visitSwitch(Visit visit, TIntermSwitch *node)
-{
-    // Here we may mutate the statement list, but it's safe since traversal has not yet reached
-    // there.
-    RemoveNoOpCasesFromEndOfStatementList(node->getStatementList(), mSymbolTable);
-    // Handle also nested switch statements.
-    return true;
-}
-
-}  // anonymous namespace
-
-void RemoveNoOpCasesFromEndOfSwitchStatements(TIntermBlock *root, TSymbolTable *symbolTable)
-{
-    RemoveNoOpCasesFromEndOfSwitchTraverser traverser(symbolTable);
-    root->traverse(&traverser);
-}
-
-}  // namespace sh

src/compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.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.
-//
-// RemoveNoOpCasesFromEndOfSwitchStatements.h: Clean up cases from the end of a switch statement
-// that only contain no-ops.
-
-#ifndef COMPILER_TRANSLATOR_REMOVENOOPCASESFROMENDOFSWITCHSTATEMENTS_H_
-#define COMPILER_TRANSLATOR_REMOVENOOPCASESFROMENDOFSWITCHSTATEMENTS_H_
-
-namespace sh
-{
-class TIntermBlock;
-class TSymbolTable;
-
-void RemoveNoOpCasesFromEndOfSwitchStatements(TIntermBlock *root, TSymbolTable *symbolTable);
-
-}  // namespace sh
-
-#endif  // COMPILER_TRANSLATOR_REMOVENOOPCASESFROMENDOFSWITCHSTATEMENTS_H_

src/compiler/translator/TranslatorHLSL.cpp

@@ -13,8 +13,8 @@
 #include "compiler/translator/ExpandIntegerPowExpressions.h"
 #include "compiler/translator/IntermNodePatternMatcher.h"
 #include "compiler/translator/OutputHLSL.h"
+#include "compiler/translator/PruneEmptyCases.h"
 #include "compiler/translator/RemoveDynamicIndexing.h"
-#include "compiler/translator/RemoveNoOpCasesFromEndOfSwitchStatements.h"
 #include "compiler/translator/RewriteElseBlocks.h"
 #include "compiler/translator/RewriteTexelFetchOffset.h"
 #include "compiler/translator/RewriteUnaryMinusOperatorInt.h"
@@ -93,13 +93,10 @@ void TranslatorHLSL::translate(TIntermBlock *root,
     sh::BreakVariableAliasingInInnerLoops(root);
 
     // WrapSwitchStatementsInBlocks should be called after any AST transformations that might
-    // introduce variable declarations inside the main scope of any switch statement.
-    if (WrapSwitchStatementsInBlocks(root))
-    {
-        // The WrapSwitchStatementsInBlocks step might introduce new no-op cases to the end of
-        // switch statements, so make sure to clean up the AST.
-        RemoveNoOpCasesFromEndOfSwitchStatements(root, &getSymbolTable());
-    }
+    // introduce variable declarations inside the main scope of any switch statement. It cannot
+    // result in no-op cases at the end of switch statements, because unreferenced variables
+    // have already been pruned.
+    WrapSwitchStatementsInBlocks(root);
 
     bool precisionEmulation =
         getResources().WEBGL_debug_shader_precision && getPragma().debugShaderPrecision;

src/compiler/translator/WrapSwitchStatementsInBlocks.cpp

@@ -40,16 +40,9 @@ namespace
 class WrapSwitchStatementsInBlocksTraverser : public TIntermTraverser
 {
   public:
-    WrapSwitchStatementsInBlocksTraverser() : TIntermTraverser(true, false, false), mDidWrap(false)
-    {
-    }
+    WrapSwitchStatementsInBlocksTraverser() : TIntermTraverser(true, false, false) {}
 
     bool visitSwitch(Visit visit, TIntermSwitch *node) override;
-
-    bool didWrap() const { return mDidWrap; }
-
-  private:
-    bool mDidWrap;
 };
 
 bool WrapSwitchStatementsInBlocksTraverser::visitSwitch(Visit, TIntermSwitch *node)
@@ -88,6 +81,9 @@ bool WrapSwitchStatementsInBlocksTraverser::visitSwitch(Visit, TIntermSwitch *no
                 node->getStatementList(), declaration, emptyReplacement));
 
             declarationInBlock->appendDeclarator(declaratorAsSymbol->deepCopy());
+            // The declaration can't be the last statement inside the switch since unused variables
+            // should already have been pruned.
+            ASSERT(declaration != statementList->back());
         }
         else
         {
@@ -111,7 +107,6 @@ bool WrapSwitchStatementsInBlocksTraverser::visitSwitch(Visit, TIntermSwitch *no
 
     wrapperBlock->appendStatement(node);
     queueReplacement(wrapperBlock, OriginalNode::BECOMES_CHILD);
-    mDidWrap = true;
 
     // Should be fine to process multiple switch statements, even nesting ones in the same
     // traversal.
@@ -121,12 +116,11 @@ bool WrapSwitchStatementsInBlocksTraverser::visitSwitch(Visit, TIntermSwitch *no
 }  // anonymous namespace
 
 // Wrap switch statements in the AST into blocks when needed.
-bool WrapSwitchStatementsInBlocks(TIntermBlock *root)
+void WrapSwitchStatementsInBlocks(TIntermBlock *root)
 {
     WrapSwitchStatementsInBlocksTraverser traverser;
     root->traverse(&traverser);
     traverser.updateTree();
-    return traverser.didWrap();
 }
 
 }  // namespace sh

src/compiler/translator/WrapSwitchStatementsInBlocks.h

@@ -15,7 +15,7 @@ namespace sh
 class TIntermBlock;
 
 // Wrap switch statements in the AST into blocks when needed. Returns true if the AST was changed.
-bool WrapSwitchStatementsInBlocks(TIntermBlock *root);
+void WrapSwitchStatementsInBlocks(TIntermBlock *root);
 
 }  // namespace sh
 

src/tests/compiler_tests/PruneEmptyCases_test.cpp

@@ -40,6 +40,8 @@ TEST_F(PruneEmptyCasesTest, SwitchStatementWithOnlyNoOps)
             switch (i)
             {
                 case 0:
+                case 1:
+                    { {} }
                     int j;
                     1;
             }
@@ -66,6 +68,8 @@ TEST_F(PruneEmptyCasesTest, SwitchStatementWithOnlyNoOpsAndInitWithSideEffect)
             switch (++i)
             {
                 case 0:
+                case 1:
+                    { {} }
                     int j;
                     1;
             }
@@ -97,6 +101,8 @@ TEST_F(PruneEmptyCasesTest, SwitchStatementLastCaseOnlyNoOps)
                     my_FragColor = vec4(0);
                     break;
                 case 1:
+                case 2:
+                    { {} }
                     int j;
                     1;
             }