From 3f51e7d942c22edaab3a7e703a1e6a2dd6a26f77 Mon Sep 17 00:00:00 2001 From: Jean Christophe Beyler Date: Thu, 4 Sep 2014 08:34:28 -0700 Subject: [PATCH] ART: Fix x86_64 GenSelect case when destination is Ref Reference in x86_64 is a 64-bit solo register. As a result, the invocation of OpRegImm results in an error when Select opcode of the kind: ref = boolean ? null : null; because opRegImm does not support 64-bit destination for OpMov. The case above is only possible for ref because no one other constant except null is possible. Bug: 17327895 Change-Id: I7541e744ec1c8619711712fd17be72764efcf3a8 Signed-off-by: Serguei Katkov --- compiler/dex/quick/x86/int_x86.cc | 99 ++++++++++++++++------------- test/003-omnibus-opcodes/expected.txt | 1 + test/003-omnibus-opcodes/src/GenSelect.java | 28 ++++++++ test/003-omnibus-opcodes/src/Main.java | 1 + 4 files changed, 84 insertions(+), 45 deletions(-) create mode 100644 test/003-omnibus-opcodes/src/GenSelect.java diff --git a/compiler/dex/quick/x86/int_x86.cc b/compiler/dex/quick/x86/int_x86.cc index a745339ca..ef2d9a681 100755 --- a/compiler/dex/quick/x86/int_x86.cc +++ b/compiler/dex/quick/x86/int_x86.cc @@ -274,7 +274,6 @@ void X86Mir2Lir::GenSelect(BasicBlock* bb, MIR* mir) { // Avoid using float regs here. RegisterClass src_reg_class = rl_src.ref ? kRefReg : kCoreReg; RegisterClass result_reg_class = rl_dest.ref ? kRefReg : kCoreReg; - rl_src = LoadValue(rl_src, src_reg_class); ConditionCode ccode = mir->meta.ccode; // The kMirOpSelect has two variants, one for constants and one for moves. @@ -283,58 +282,68 @@ void X86Mir2Lir::GenSelect(BasicBlock* bb, MIR* mir) { if (is_constant_case) { int true_val = mir->dalvikInsn.vB; int false_val = mir->dalvikInsn.vC; - rl_result = EvalLoc(rl_dest, result_reg_class, true); - /* - * For ccode == kCondEq: - * - * 1) When the true case is zero and result_reg is not same as src_reg: - * xor result_reg, result_reg - * cmp $0, src_reg - * mov t1, $false_case - * cmovnz result_reg, t1 - * 2) When the false case is zero and result_reg is not same as src_reg: - * xor result_reg, result_reg - * cmp $0, src_reg - * mov t1, $true_case - * cmovz result_reg, t1 - * 3) All other cases (we do compare first to set eflags): - * cmp $0, src_reg - * mov result_reg, $false_case - * mov t1, $true_case - * cmovz result_reg, t1 - */ - // FIXME: depending on how you use registers you could get a false != mismatch when dealing - // with different views of the same underlying physical resource (i.e. solo32 vs. solo64). - const bool result_reg_same_as_src = - (rl_src.location == kLocPhysReg && rl_src.reg.GetRegNum() == rl_result.reg.GetRegNum()); - const bool true_zero_case = (true_val == 0 && false_val != 0 && !result_reg_same_as_src); - const bool false_zero_case = (false_val == 0 && true_val != 0 && !result_reg_same_as_src); - const bool catch_all_case = !(true_zero_case || false_zero_case); - - if (true_zero_case || false_zero_case) { - OpRegReg(kOpXor, rl_result.reg, rl_result.reg); - } + // simplest strange case + if (true_val == false_val) { + rl_result = EvalLoc(rl_dest, result_reg_class, true); + LoadConstantNoClobber(rl_result.reg, true_val); + } else { + // TODO: use GenSelectConst32 and handle additional opcode patterns such as + // "cmp; setcc; movzx" or "cmp; sbb r0,r0; and r0,$mask; add r0,$literal". + rl_src = LoadValue(rl_src, src_reg_class); + rl_result = EvalLoc(rl_dest, result_reg_class, true); + /* + * For ccode == kCondEq: + * + * 1) When the true case is zero and result_reg is not same as src_reg: + * xor result_reg, result_reg + * cmp $0, src_reg + * mov t1, $false_case + * cmovnz result_reg, t1 + * 2) When the false case is zero and result_reg is not same as src_reg: + * xor result_reg, result_reg + * cmp $0, src_reg + * mov t1, $true_case + * cmovz result_reg, t1 + * 3) All other cases (we do compare first to set eflags): + * cmp $0, src_reg + * mov result_reg, $false_case + * mov t1, $true_case + * cmovz result_reg, t1 + */ + // FIXME: depending on how you use registers you could get a false != mismatch when dealing + // with different views of the same underlying physical resource (i.e. solo32 vs. solo64). + const bool result_reg_same_as_src = + (rl_src.location == kLocPhysReg && rl_src.reg.GetRegNum() == rl_result.reg.GetRegNum()); + const bool true_zero_case = (true_val == 0 && false_val != 0 && !result_reg_same_as_src); + const bool false_zero_case = (false_val == 0 && true_val != 0 && !result_reg_same_as_src); + const bool catch_all_case = !(true_zero_case || false_zero_case); + + if (true_zero_case || false_zero_case) { + OpRegReg(kOpXor, rl_result.reg, rl_result.reg); + } - if (true_zero_case || false_zero_case || catch_all_case) { - OpRegImm(kOpCmp, rl_src.reg, 0); - } + if (true_zero_case || false_zero_case || catch_all_case) { + OpRegImm(kOpCmp, rl_src.reg, 0); + } - if (catch_all_case) { - OpRegImm(kOpMov, rl_result.reg, false_val); - } + if (catch_all_case) { + OpRegImm(kOpMov, rl_result.reg, false_val); + } - if (true_zero_case || false_zero_case || catch_all_case) { - ConditionCode cc = true_zero_case ? NegateComparison(ccode) : ccode; - int immediateForTemp = true_zero_case ? false_val : true_val; - RegStorage temp1_reg = AllocTypedTemp(false, result_reg_class); - OpRegImm(kOpMov, temp1_reg, immediateForTemp); + if (true_zero_case || false_zero_case || catch_all_case) { + ConditionCode cc = true_zero_case ? NegateComparison(ccode) : ccode; + int immediateForTemp = true_zero_case ? false_val : true_val; + RegStorage temp1_reg = AllocTypedTemp(false, result_reg_class); + OpRegImm(kOpMov, temp1_reg, immediateForTemp); - OpCondRegReg(kOpCmov, cc, rl_result.reg, temp1_reg); + OpCondRegReg(kOpCmov, cc, rl_result.reg, temp1_reg); - FreeTemp(temp1_reg); + FreeTemp(temp1_reg); + } } } else { + rl_src = LoadValue(rl_src, src_reg_class); RegLocation rl_true = mir_graph_->GetSrc(mir, 1); RegLocation rl_false = mir_graph_->GetSrc(mir, 2); rl_true = LoadValue(rl_true, result_reg_class); diff --git a/test/003-omnibus-opcodes/expected.txt b/test/003-omnibus-opcodes/expected.txt index a62c4986b..b591a7a02 100644 --- a/test/003-omnibus-opcodes/expected.txt +++ b/test/003-omnibus-opcodes/expected.txt @@ -72,4 +72,5 @@ UnresTest1... UnresTest2... UnresTest2 done InternedString.run +null Done! diff --git a/test/003-omnibus-opcodes/src/GenSelect.java b/test/003-omnibus-opcodes/src/GenSelect.java new file mode 100644 index 000000000..7e3481b38 --- /dev/null +++ b/test/003-omnibus-opcodes/src/GenSelect.java @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2014 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +public class GenSelect { + public static String test(boolean b) { + String str1 = null; + String str2 = null; + String res = b ? str1 : str2; + return res; + } + + public static void run() { + System.out.println(test(true)); + } +} diff --git a/test/003-omnibus-opcodes/src/Main.java b/test/003-omnibus-opcodes/src/Main.java index 25050df8f..a30ec15c6 100644 --- a/test/003-omnibus-opcodes/src/Main.java +++ b/test/003-omnibus-opcodes/src/Main.java @@ -70,6 +70,7 @@ public class Main { th.printStackTrace(); } InternedString.run(); + GenSelect.run(); } public static void assertTrue(boolean condition) { -- 2.11.0