ART: Fix NullCheckElimination, BBCombine, and SplitBlock

NullCheckElimination had one issue and one assumption that could be
broken:
-It ignored that compiler temps may hold references.
-Assumed there are no phi nodes even though algorithm can be run even
after phi nodes are inserted.

BBCombine also had issue in that it did not properly maintain the
instruction links. The logic has been updated to use utility methods.

SplitBlock has an issue with being called after extended instructions
are inserted. Namely, in the case in question, it was called after
SpecialMethodInliner was through and although it was doing semantically
correct thing, it was hitting dcheck due to the kMirOpNullCheck.

Change-Id: Id5863ddb0762064e74bf1d9173b8db5cb47cf3b9
Signed-off-by: Razvan A Lupusoru <razvan.a.lupusoru@intel.com>

diff --git a/compiler/dex/mir_graph.cc b/compiler/dex/mir_graph.cc
index f69d63c..8190f95 100644 (file)
--- a/compiler/dex/mir_graph.cc
+++ b/compiler/dex/mir_graph.cc
@@ -258,8 +258,6 @@ BasicBlock* MIRGraph::SplitBlock(DexOffset code_offset,
   DCHECK(insn != orig_block->first_mir_insn);
   DCHECK(insn == bottom_block->first_mir_insn);
   DCHECK_EQ(insn->offset, bottom_block->start_offset);
-  DCHECK(static_cast<int>(insn->dalvikInsn.opcode) == kMirOpCheck ||
-         !MIR::DecodedInstruction::IsPseudoMirOp(insn->dalvikInsn.opcode));
   DCHECK_EQ(dex_pc_to_block_map_[insn->offset], orig_block->id);
   // Scan the "bottom" instructions, remapping them to the
   // newly created "bottom" block.

diff --git a/compiler/dex/mir_optimization.cc b/compiler/dex/mir_optimization.cc
index d025d08..bb7ee89 100644 (file)
--- a/compiler/dex/mir_optimization.cc
+++ b/compiler/dex/mir_optimization.cc
@@ -801,17 +801,18 @@ void MIRGraph::CombineBlocks(class BasicBlock* bb) {
     BasicBlock* bb_next = GetBasicBlock(bb->fall_through);
     DCHECK(!bb_next->catch_entry);
     DCHECK_EQ(bb_next->predecessors.size(), 1u);
-    // Overwrite the kMirOpCheck insn with the paired opcode.
+
+    // Now move instructions from bb_next to bb. Start off with doing a sanity check
+    // that kMirOpCheck's throw instruction is first one in the bb_next.
     DCHECK_EQ(bb_next->first_mir_insn, throw_insn);
-    *bb->last_mir_insn = *throw_insn;
-    // And grab the rest of the instructions from bb_next.
-    bb->last_mir_insn = bb_next->last_mir_insn;
-    throw_insn->next = nullptr;
-    bb_next->last_mir_insn = throw_insn;
-    // Mark acquired instructions as belonging to bb.
-    for (MIR* insn = mir; insn != nullptr; insn = insn->next) {
-      insn->bb = bb->id;
-    }
+    // Now move all instructions (throw instruction to last one) from bb_next to bb.
+    MIR* last_to_move = bb_next->last_mir_insn;
+    bb_next->RemoveMIRList(throw_insn, last_to_move);
+    bb->InsertMIRListAfter(bb->last_mir_insn, throw_insn, last_to_move);
+    // The kMirOpCheck instruction is not needed anymore.
+    mir->dalvikInsn.opcode = static_cast<Instruction::Code>(kMirOpNop);
+    bb->RemoveMIR(mir);
+
     // Before we overwrite successors, remove their predecessor links to bb.
     bb_next->ErasePredecessor(bb->id);
     if (bb->taken != NullBasicBlockId) {
@@ -891,7 +892,7 @@ bool MIRGraph::EliminateNullChecksGate() {
 
   DCHECK(temp_scoped_alloc_.get() == nullptr);
   temp_scoped_alloc_.reset(ScopedArenaAllocator::Create(&cu_->arena_stack));
-  temp_.nce.num_vregs = GetNumOfCodeVRs();
+  temp_.nce.num_vregs = GetNumOfCodeAndTempVRs();
   temp_.nce.work_vregs_to_check = new (temp_scoped_alloc_.get()) ArenaBitVector(
       temp_scoped_alloc_.get(), temp_.nce.num_vregs, false, kBitMapNullCheck);
   temp_.nce.ending_vregs_to_check_matrix = static_cast<ArenaBitVector**>(
@@ -979,7 +980,10 @@ bool MIRGraph::EliminateNullChecks(BasicBlock* bb) {
   for (MIR* mir = bb->first_mir_insn; mir != NULL; mir = mir->next) {
     uint64_t df_attributes = GetDataFlowAttributes(mir);
 
-    DCHECK_EQ(df_attributes & DF_NULL_TRANSFER_N, 0u);  // No Phis yet.
+    if ((df_attributes & DF_NULL_TRANSFER_N) != 0u) {
+      // The algorithm was written in a phi agnostic way.
+      continue;
+    }
 
     // Might need a null check?
     if (df_attributes & DF_HAS_NULL_CHKS) {