Merge "Quick: Fix optimizations for empty if blocks."

7 files changed, 56 insertions, 34 deletions

diff --git a/compiler/dex/global_value_numbering.cc b/compiler/dex/global_value_numbering.cc
index e2b99871c8..94ba4fad2a 100644
--- a/compiler/dex/global_value_numbering.cc
+++ b/compiler/dex/global_value_numbering.cc
@@ -160,20 +160,10 @@ uint16_t GlobalValueNumbering::GetArrayLocation(uint16_t base, uint16_t index) {
   return location;
 }
 
-bool GlobalValueNumbering::HasNullCheckLastInsn(const BasicBlock* pred_bb,
-                                                BasicBlockId succ_id) {
-  if (pred_bb->block_type != kDalvikByteCode || pred_bb->last_mir_insn == nullptr) {
-    return false;
-  }
-  Instruction::Code last_opcode = pred_bb->last_mir_insn->dalvikInsn.opcode;
-  return ((last_opcode == Instruction::IF_EQZ && pred_bb->fall_through == succ_id) ||
-      (last_opcode == Instruction::IF_NEZ && pred_bb->taken == succ_id));
-}
-
 bool GlobalValueNumbering::NullCheckedInAllPredecessors(
     const ScopedArenaVector<uint16_t>& merge_names) const {
   // Implicit parameters:
-  //   - *work_lvn: the LVN for which we're checking predecessors.
+  //   - *work_lvn_: the LVN for which we're checking predecessors.
   //   - merge_lvns_: the predecessor LVNs.
   DCHECK_EQ(merge_lvns_.size(), merge_names.size());
   for (size_t i = 0, size = merge_lvns_.size(); i != size; ++i) {
@@ -198,7 +188,7 @@ bool GlobalValueNumbering::NullCheckedInAllPredecessors(
 bool GlobalValueNumbering::DivZeroCheckedInAllPredecessors(
     const ScopedArenaVector<uint16_t>& merge_names) const {
   // Implicit parameters:
-  //   - *work_lvn: the LVN for which we're checking predecessors.
+  //   - *work_lvn_: the LVN for which we're checking predecessors.
   //   - merge_lvns_: the predecessor LVNs.
   DCHECK_EQ(merge_lvns_.size(), merge_names.size());
   for (size_t i = 0, size = merge_lvns_.size(); i != size; ++i) {
@@ -217,15 +207,11 @@ bool GlobalValueNumbering::IsBlockEnteredOnTrue(uint16_t cond, BasicBlockId bb_i
   if (bb->predecessors.size() == 1u) {
     BasicBlockId pred_id = bb->predecessors[0];
     BasicBlock* pred_bb = mir_graph_->GetBasicBlock(pred_id);
-    if (pred_bb->last_mir_insn != nullptr) {
-      Instruction::Code opcode = pred_bb->last_mir_insn->dalvikInsn.opcode;
-      if ((opcode == Instruction::IF_NEZ && pred_bb->taken == bb_id) ||
-          (opcode == Instruction::IF_EQZ && pred_bb->fall_through == bb_id)) {
-        DCHECK(lvns_[pred_id] != nullptr);
-        uint16_t operand = lvns_[pred_id]->GetSregValue(pred_bb->last_mir_insn->ssa_rep->uses[0]);
-        if (operand == cond) {
-          return true;
-        }
+    if (pred_bb->BranchesToSuccessorOnlyIfNotZero(bb_id)) {
+      DCHECK(lvns_[pred_id] != nullptr);
+      uint16_t operand = lvns_[pred_id]->GetSregValue(pred_bb->last_mir_insn->ssa_rep->uses[0]);
+      if (operand == cond) {
+        return true;
       }
     }
   }
diff --git a/compiler/dex/global_value_numbering.h b/compiler/dex/global_value_numbering.h
index bd2f187d17..c514f75dcc 100644
--- a/compiler/dex/global_value_numbering.h
+++ b/compiler/dex/global_value_numbering.h
@@ -194,7 +194,9 @@ class GlobalValueNumbering : public DeletableArenaObject<kArenaAllocMisc> {
     return mir_graph_->GetBasicBlock(bb_id);
   }
 
-  static bool HasNullCheckLastInsn(const BasicBlock* pred_bb, BasicBlockId succ_id);
+  static bool HasNullCheckLastInsn(const BasicBlock* pred_bb, BasicBlockId succ_id) {
+    return pred_bb->BranchesToSuccessorOnlyIfNotZero(succ_id);
+  }
 
   bool NullCheckedInAllPredecessors(const ScopedArenaVector<uint16_t>& merge_names) const;
 
diff --git a/compiler/dex/mir_graph.h b/compiler/dex/mir_graph.h
index f038397e1e..dbe906280f 100644
--- a/compiler/dex/mir_graph.h
+++ b/compiler/dex/mir_graph.h
@@ -452,6 +452,21 @@ class BasicBlock : public DeletableArenaObject<kArenaAllocBB> {
   MIR* GetFirstNonPhiInsn();
 
   /**
+   * @brief Checks whether the block ends with if-nez or if-eqz that branches to
+   *        the given successor only if the register in not zero.
+   */
+  bool BranchesToSuccessorOnlyIfNotZero(BasicBlockId succ_id) const {
+    if (last_mir_insn == nullptr) {
+      return false;
+    }
+    Instruction::Code last_opcode = last_mir_insn->dalvikInsn.opcode;
+    return ((last_opcode == Instruction::IF_EQZ && fall_through == succ_id) ||
+        (last_opcode == Instruction::IF_NEZ && taken == succ_id)) &&
+        // Make sure the other successor isn't the same (empty if), b/21614284.
+        (fall_through != taken);
+  }
+
+  /**
    * @brief Used to obtain the next MIR that follows unconditionally.
    * @details The implementation does not guarantee that a MIR does not
    * follow even if this method returns nullptr.
diff --git a/compiler/dex/mir_optimization.cc b/compiler/dex/mir_optimization.cc
index 645511ed9f..727d0fd759 100644
--- a/compiler/dex/mir_optimization.cc
+++ b/compiler/dex/mir_optimization.cc
@@ -978,18 +978,12 @@ bool MIRGraph::EliminateNullChecks(BasicBlock* bb) {
       BasicBlock* pred_bb = GetBasicBlock(pred_id);
       DCHECK(pred_bb != nullptr);
       MIR* null_check_insn = nullptr;
-      if (pred_bb->block_type == kDalvikByteCode) {
-        // Check to see if predecessor had an explicit null-check.
-        MIR* last_insn = pred_bb->last_mir_insn;
-        if (last_insn != nullptr) {
-          Instruction::Code last_opcode = last_insn->dalvikInsn.opcode;
-          if ((last_opcode == Instruction::IF_EQZ && pred_bb->fall_through == bb->id) ||
-              (last_opcode == Instruction::IF_NEZ && pred_bb->taken == bb->id)) {
-            // Remember the null check insn if there's no other predecessor requiring null check.
-            if (!copied_first || !vregs_to_check->IsBitSet(last_insn->dalvikInsn.vA)) {
-              null_check_insn = last_insn;
-            }
-          }
+      // Check to see if predecessor had an explicit null-check.
+      if (pred_bb->BranchesToSuccessorOnlyIfNotZero(bb->id)) {
+        // Remember the null check insn if there's no other predecessor requiring null check.
+        if (!copied_first || !vregs_to_check->IsBitSet(pred_bb->last_mir_insn->dalvikInsn.vA)) {
+          null_check_insn = pred_bb->last_mir_insn;
+          DCHECK(null_check_insn != nullptr);
         }
       }
       if (!copied_first) {
diff --git a/test/800-smali/expected.txt b/test/800-smali/expected.txt
index 7be78b7160..6c00c82983 100644
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -19,4 +19,5 @@ b/20224106
 b/17410612
 b/21863767
 b/21873167
+b/21614284
 Done!
diff --git a/test/800-smali/smali/b_21614284.smali b/test/800-smali/smali/b_21614284.smali
new file mode 100644
index 0000000000..3cb1bd0ce2
--- /dev/null
+++ b/test/800-smali/smali/b_21614284.smali
@@ -0,0 +1,22 @@
+.class public LB21614284;
+.super Ljava/lang/Object;
+
+.field private a:I
+
+.method public constructor <init>()V
+    .registers 2
+    invoke-direct {p0}, Ljava/lang/Object;-><init>()V
+    const v0, 42
+    iput v0, p0, LB21614284;->a:I
+    return-void
+.end method
+
+.method public static test(LB21614284;)I
+    .registers 2
+    # Empty if, testing p0.
+    if-nez p0, :label
+    :label
+    # p0 still needs a null check.
+    iget v0, p0, LB21614284;->a:I
+    return v0
+.end method
diff --git a/test/800-smali/src/Main.java b/test/800-smali/src/Main.java
index 2180186ac5..ab4457e919 100644
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -86,6 +86,8 @@ public class Main {
         testCases.add(new TestCase("b/21863767", "B21863767", "run", null, null,
                 null));
         testCases.add(new TestCase("b/21873167", "B21873167", "test", null, null, null));
+        testCases.add(new TestCase("b/21614284", "B21614284", "test", new Object[] { null },
+            new NullPointerException(), null));
     }
 
     public void runTests() {