Quick: Fix optimizations for empty if blocks.
If a block ending with if-eqz or if-nez has the same "taken"
and "fallthrough", we cannot assume that the value has been
checked against zero in one of the succesors. This affects
the null check elimination pass as well as GVN. Refactor all
those checks to a single function in BasicBlock and check
that the "taken" and "falthrough" are different when needed.
Bug: 21614284
Change-Id: I8c6ac23e96cdaf5984786a555ebbd28110f095cb
diff --git a/compiler/dex/global_value_numbering.cc b/compiler/dex/global_value_numbering.cc
index e2b9987..94ba4fa 100644
--- a/compiler/dex/global_value_numbering.cc
+++ b/compiler/dex/global_value_numbering.cc
@@ -160,20 +160,10 @@
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::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 @@
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 bd2f187..c514f75 100644
--- a/compiler/dex/global_value_numbering.h
+++ b/compiler/dex/global_value_numbering.h
@@ -194,7 +194,9 @@
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 f038397..dbe9062 100644
--- a/compiler/dex/mir_graph.h
+++ b/compiler/dex/mir_graph.h
@@ -452,6 +452,21 @@
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 645511e..727d0fd 100644
--- a/compiler/dex/mir_optimization.cc
+++ b/compiler/dex/mir_optimization.cc
@@ -978,18 +978,12 @@
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 7be78b7..6c00c82 100644
--- a/test/800-smali/expected.txt
+++ b/test/800-smali/expected.txt
@@ -19,4 +19,5 @@
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 0000000..3cb1bd0
--- /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 2180186..ab4457e 100644
--- a/test/800-smali/src/Main.java
+++ b/test/800-smali/src/Main.java
@@ -86,6 +86,8 @@
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() {