From 9f7687cb5c1390ec4bcc2f8fa10dbee33aff3d6a Mon Sep 17 00:00:00 2001 From: Vladimir Marko Date: Fri, 19 Jun 2015 12:58:22 +0100 Subject: [PATCH] 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 (cherry picked from commit f11c420c448baffac6a70ac0884d481ab347e257) Change-Id: I062e0042de3470ce8680b586487b9c7acbd206bc --- compiler/dex/global_value_numbering.cc | 28 +++++++--------------------- compiler/dex/global_value_numbering.h | 4 +++- compiler/dex/mir_graph.h | 15 +++++++++++++++ compiler/dex/mir_optimization.cc | 18 ++++++------------ test/800-smali/expected.txt | 1 + test/800-smali/smali/b_21614284.smali | 22 ++++++++++++++++++++++ test/800-smali/src/Main.java | 2 ++ 7 files changed, 56 insertions(+), 34 deletions(-) create mode 100644 test/800-smali/smali/b_21614284.smali diff --git a/compiler/dex/global_value_numbering.cc b/compiler/dex/global_value_numbering.cc index e2b99871c..94ba4fad2 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& 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& 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 bd2f187d1..c514f75dc 100644 --- a/compiler/dex/global_value_numbering.h +++ b/compiler/dex/global_value_numbering.h @@ -194,7 +194,9 @@ class GlobalValueNumbering : public DeletableArenaObject { 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& merge_names) const; diff --git a/compiler/dex/mir_graph.h b/compiler/dex/mir_graph.h index f038397e1..dbe906280 100644 --- a/compiler/dex/mir_graph.h +++ b/compiler/dex/mir_graph.h @@ -452,6 +452,21 @@ class BasicBlock : public DeletableArenaObject { 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 645511ed9..727d0fd75 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 c5728b560..e2075da91 100644 --- a/test/800-smali/expected.txt +++ b/test/800-smali/expected.txt @@ -17,4 +17,5 @@ MoveExceptionOnEntry EmptySparseSwitch b/20224106 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 000000000..3cb1bd0ce --- /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 ()V + .registers 2 + invoke-direct {p0}, Ljava/lang/Object;->()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 7196de145..4c6da6349 100644 --- a/test/800-smali/src/Main.java +++ b/test/800-smali/src/Main.java @@ -82,6 +82,8 @@ public class Main { testCases.add(new TestCase("b/20224106", "B20224106", "run", null, new VerifyError(), 0)); 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() { -- 2.11.0