From 21cb657159b3e93cc888685ade83f8fc519290be Mon Sep 17 00:00:00 2001
From: Vladimir Marko <vmarko@google.com>
Date: Mon, 8 Jun 2015 16:39:02 +0100
Subject: [PATCH] Quick: Fix LoopRepeatingTopologicalSortIterator.

Always push the loop head on the loop head stack. This fixes
a bug where we failed to return to an unnatural loop head to
recalculate its GVN data.

Bug: 17410955

(cherry picked from commit 67c8c942e9dfcabd548351db75e6d3b8b5165afa)

Change-Id: I44b9a17cbcd7307d1cc70ac564b99e29803723c5
---
 compiler/dex/dataflow_iterator-inl.h | 12 +++++--
 compiler/dex/mir_graph_test.cc       | 69 ++++++++++++++++++++++++++++++++++++
 compiler/dex/mir_optimization.cc     |  3 +-
 3 files changed, 80 insertions(+), 4 deletions(-)

diff --git a/compiler/dex/dataflow_iterator-inl.h b/compiler/dex/dataflow_iterator-inl.h
index 83dfc2884..e9402e39e 100644
--- a/compiler/dex/dataflow_iterator-inl.h
+++ b/compiler/dex/dataflow_iterator-inl.h
@@ -181,10 +181,16 @@ inline BasicBlock* LoopRepeatingTopologicalSortIterator::Next(bool had_change) {
     idx_ += 1;
     BasicBlock* bb = mir_graph_->GetBasicBlock((*block_id_list_)[idx]);
     DCHECK(bb != nullptr);
+    if ((*loop_ends_)[idx] != 0u) {
+      // If bb->visited is false, the loop needs to be processed from scratch.
+      // Otherwise we mark it as recalculating; for a natural loop we will not
+      // need to recalculate any block in the loop anyway, and for unnatural
+      // loops we will recalculate the loop head only if one of its predecessors
+      // actually changes.
+      bool recalculating = bb->visited;
+      loop_head_stack_->push_back(std::make_pair(idx, recalculating));
+    }
     if (!bb->visited) {
-      if ((*loop_ends_)[idx] != 0u) {
-        loop_head_stack_->push_back(std::make_pair(idx, false));  // Not recalculating.
-      }
       return bb;
     }
   }
diff --git a/compiler/dex/mir_graph_test.cc b/compiler/dex/mir_graph_test.cc
index b3ad0407e..49b7511b4 100644
--- a/compiler/dex/mir_graph_test.cc
+++ b/compiler/dex/mir_graph_test.cc
@@ -15,6 +15,7 @@
  */
 
 #include "compiler_ir.h"
+#include "dataflow_iterator-inl.h"
 #include "mir_graph.h"
 #include "gtest/gtest.h"
 
@@ -374,4 +375,72 @@ TEST_F(TopologicalSortOrderTest, LoopWithTwoEntryPoints) {
   CheckLoopEnds(loop_ends);
 }
 
+TEST_F(TopologicalSortOrderTest, UnnaturalLoops) {
+  const BBDef bbs[] = {
+      DEF_BB(kNullBlock, DEF_SUCC0(), DEF_PRED0()),
+      DEF_BB(kEntryBlock, DEF_SUCC1(3), DEF_PRED0()),
+      DEF_BB(kExitBlock, DEF_SUCC0(), DEF_PRED1(10)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC2(5, 4), DEF_PRED1(1)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC1(5), DEF_PRED2(11, 3)),  // Unnatural loop head (top-level).
+      DEF_BB(kDalvikByteCode, DEF_SUCC1(6), DEF_PRED2(3, 4)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC2(9, 7), DEF_PRED1(5)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC1(8), DEF_PRED1(6)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC1(9), DEF_PRED2(10, 7)),  // Unnatural loop head (nested).
+      DEF_BB(kDalvikByteCode, DEF_SUCC1(10), DEF_PRED2(6, 8)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC2(8, 11), DEF_PRED1(9)),
+      DEF_BB(kDalvikByteCode, DEF_SUCC2(4, 2), DEF_PRED1(10)),
+  };
+  const BasicBlockId expected_order[] = {
+      1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 2
+  };
+  const uint16_t loop_ends[] = {
+      0, 0, 10, 0, 0, 0, 9, 0, 0, 0, 0,
+  };
+
+  PrepareBasicBlocks(bbs);
+  ComputeTopologicalSortOrder();
+  CheckOrder(expected_order);
+  CheckLoopEnds(loop_ends);
+
+  const std::pair<BasicBlockId, bool> expected_and_change[] = {
+      { 1, false },
+      { 3, false },
+      { 4, true },    // Initial run of the outer loop.
+      { 5, true },
+      { 6, true },
+      { 7, true },
+      { 8, true },    // Initial run of the inner loop.
+      { 9, true },
+      { 10, true },
+      { 8, true },    // Recalculation of the inner loop - changed.
+      { 9, true },
+      { 10, true },
+      { 8, false },   // Recalculation of the inner loop - unchanged.
+      { 11, true },
+      { 4, true },    // Recalculation of the outer loop - changed.
+      { 5, true },
+      { 6, true },
+      { 7, false },   // No change: skip inner loop head because inputs are unchanged.
+      { 9, true },
+      { 10, true },
+      { 8, true },    // Recalculation of the inner loop - changed.
+      { 9, true },
+      { 10, true },
+      { 8, false },   // Recalculation of the inner loop - unchanged.
+      { 11, true },
+      { 4, false },   // Recalculation of the outer loop - unchanged.
+      { 2, false },
+  };
+  size_t pos = 0;
+  LoopRepeatingTopologicalSortIterator iter(cu_.mir_graph.get());
+  bool change = false;
+  for (BasicBlock* bb = iter.Next(change); bb != nullptr; bb = iter.Next(change)) {
+    ASSERT_NE(arraysize(expected_and_change), pos);
+    ASSERT_EQ(expected_and_change[pos].first, bb->id) << pos;
+    change = expected_and_change[pos].second;
+    ++pos;
+  }
+  ASSERT_EQ(arraysize(expected_and_change), pos);
+}
+
 }  // namespace art
diff --git a/compiler/dex/mir_optimization.cc b/compiler/dex/mir_optimization.cc
index 7b1ec398d..645511ed9 100644
--- a/compiler/dex/mir_optimization.cc
+++ b/compiler/dex/mir_optimization.cc
@@ -1790,7 +1790,8 @@ bool MIRGraph::EliminateSuspendChecks(BasicBlock* bb) {
         pred_mask_union |= pred_mask;
       }
     }
-    DCHECK_EQ(((1u << (IsLoopHead(bb->id) ? bb->nesting_depth - 1u: bb->nesting_depth)) - 1u),
+    // DCHECK_EQ() may not hold for unnatural loop heads, so use DCHECK_GE().
+    DCHECK_GE(((1u << (IsLoopHead(bb->id) ? bb->nesting_depth - 1u: bb->nesting_depth)) - 1u),
               pred_mask_union);
     suspend_checks_in_loops &= pred_mask_union;
   }
-- 
2.11.0