Translator: Fix dead-code-elimination corner case
The DCE code had a corner case bug where a switch case containing
multiple DCE'ed `break`s followed by another case would cause some of
the DCE'ed statements in the first case to be doubly pruned, failing on
an assertion.
This was due to the fact that visitBlock() was asking traversal to
continue if a new case was visited while pruning nodes, but the
traversal does not take into account that some statements need to be
skipped and so would also visit the pruned statements. If the pruned
statements contain a branch instruction, they get re-pruned.
The visitBlock() function that does the pruning is reworked so that it
more clearly traverses the statements.
Bug: chromium:1237200
Change-Id: Ib078c2ea73ade756c7d7ef5a5c489fa53c39f352
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3077659
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Tim Van Patten <timvp@google.com>
Commit-Queue: Shahbaz Youssefi <syoussefi@chromium.org>
diff --git a/src/compiler/translator/tree_ops/PruneNoOps.cpp b/src/compiler/translator/tree_ops/PruneNoOps.cpp
index 15ce791..a10a84c 100644
--- a/src/compiler/translator/tree_ops/PruneNoOps.cpp
+++ b/src/compiler/translator/tree_ops/PruneNoOps.cpp
@@ -136,57 +136,43 @@
bool PruneNoOpsTraverser::visitBlock(Visit visit, TIntermBlock *node)
{
- if (visit == PreVisit)
- {
- return true;
- }
+ ASSERT(visit == PreVisit);
- TIntermSequence *statements = node->getSequence();
- const size_t lastChildIndex = getLastTraversedChildIndex(visit);
+ TIntermSequence &statements = *node->getSequence();
TIntermSequence emptyReplacement;
- // If a branch is visited, prune the rest of the statements.
- if (mIsBranchVisited)
+ // Visit each statement in the block one by one. Once a branch is visited (break, continue,
+ // return or discard), drop the rest of the statements.
+ for (size_t statementIndex = 0; statementIndex < statements.size(); ++statementIndex)
{
- for (size_t removeIndex = lastChildIndex + 1; removeIndex < statements->size();
- ++removeIndex)
- {
- TIntermNode *statement = (*statements)[removeIndex];
+ TIntermNode *statement = statements[statementIndex];
- // If the statement is a switch case label, stop pruning and continue visiting the
- // children.
- if (statement->getAsCaseNode() != nullptr)
- {
- mIsBranchVisited = false;
- return true;
- }
-
- mMultiReplacements.emplace_back(node, statement, std::move(emptyReplacement));
- }
-
- // If the parent is a block, this is a nested block without any condition (like if, loop or
- // switch), so the rest of the parent block should also be pruned. Otherwise the parent
- // block should be unaffected.
- if (getParentNode()->getAsBlock() == nullptr)
+ // If the statement is a switch case label, stop pruning and continue visiting the children.
+ if (statement->getAsCaseNode() != nullptr)
{
mIsBranchVisited = false;
}
- // Don't visit the pruned children.
- return false;
- }
-
- // If the statement is a noop, prune it.
- if (!statements->empty())
- {
- TIntermNode *statement = (*statements)[lastChildIndex];
- if (IsNoOp(statement))
+ // If a branch is visited, prune the statement. If the statement is a no-op, also prune it.
+ if (mIsBranchVisited || IsNoOp(statement))
{
mMultiReplacements.emplace_back(node, statement, std::move(emptyReplacement));
+ continue;
}
+
+ // Visit the statement if not pruned.
+ statement->traverse(this);
}
- return true;
+ // If the parent is a block and mIsBranchVisited is set, this is a nested block without any
+ // condition (like if, loop or switch), so the rest of the parent block should also be pruned.
+ // Otherwise the parent block should be unaffected.
+ if (mIsBranchVisited && getParentNode()->getAsBlock() == nullptr)
+ {
+ mIsBranchVisited = false;
+ }
+
+ return false;
}
bool PruneNoOpsTraverser::visitLoop(Visit visit, TIntermLoop *loop)
diff --git a/src/tests/gl_tests/GLSLTest.cpp b/src/tests/gl_tests/GLSLTest.cpp
index 697ec5e..1cfbd32 100644
--- a/src/tests/gl_tests/GLSLTest.cpp
+++ b/src/tests/gl_tests/GLSLTest.cpp
@@ -9629,6 +9629,37 @@
EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
}
+// Regression test based on fuzzer issue. If a case has statements that are pruned, and those
+// pruned statements in turn have branches, and another case follows, a prior implementation of
+// dead-code elimination doubly pruned some statements.
+TEST_P(GLSLTest_ES3, DeadCodeBranchInPrunedStatementsInCaseBeforeAnotherCase)
+{
+ constexpr char kFS[] = R"(#version 300 es
+precision mediump float;
+out vec4 color;
+void main()
+{
+ color = vec4(0, 1, 0, 1);
+ switch(0)
+ {
+ case 0:
+ break;
+ break;
+ color = vec4(1, 0, 0, 1); // The bug was pruning this statement twice
+ default:
+ color = vec4(0, 0, 1, 1);
+ break;
+ }
+})";
+
+ ANGLE_GL_PROGRAM(program, essl3_shaders::vs::Simple(), kFS);
+
+ drawQuad(program, essl3_shaders::PositionAttrib(), 0.5f);
+ EXPECT_GL_NO_ERROR();
+
+ EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
// Test shader with all resources (default uniform, UBO, SSBO, image, sampler and atomic counter) to
// make sure they are all linked ok. The front-end sorts these resources and traverses the list of
// "uniforms" to find the range for each resource. A bug there was causing some resource ranges to
