From ad3359e77357cc5ce29ce529ab2ed9d0d8401da4 Mon Sep 17 00:00:00 2001 From: Nicolas Geoffray Date: Mon, 22 Jun 2015 23:12:45 +0100 Subject: [PATCH] Do not overwrite an input register in shift operations. 'second_reg' is an input register that can survive the instruction. Instead use the output register as a temporary result. bug:21667432 (cherry picked from commit a4f3581da73b83484a30ab499c4f8ad43b378dab) Change-Id: Ic1f399964911b8a9fc57352130c92b2a0a1b8e0d --- compiler/optimizing/code_generator_arm.cc | 42 ++++++------ compiler/optimizing/code_generator_arm.h | 1 + test/514-shifts/expected.txt | 0 test/514-shifts/info.txt | 2 + test/514-shifts/src/Main.java | 106 ++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 20 deletions(-) create mode 100644 test/514-shifts/expected.txt create mode 100644 test/514-shifts/info.txt create mode 100644 test/514-shifts/src/Main.java diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc index 2b1131d65..3f28e64b4 100644 --- a/compiler/optimizing/code_generator_arm.cc +++ b/compiler/optimizing/code_generator_arm.cc @@ -2458,7 +2458,9 @@ void LocationsBuilderARM::HandleShift(HBinaryOperation* op) { case Primitive::kPrimInt: { locations->SetInAt(0, Location::RequiresRegister()); locations->SetInAt(1, Location::RegisterOrConstant(op->InputAt(1))); - locations->SetOut(Location::RequiresRegister(), Location::kNoOutputOverlap); + // Make the output overlap, as it will be used to hold the masked + // second input. + locations->SetOut(Location::RequiresRegister(), Location::kOutputOverlap); break; } case Primitive::kPrimLong: { @@ -2489,13 +2491,13 @@ void InstructionCodeGeneratorARM::HandleShift(HBinaryOperation* op) { // Arm doesn't mask the shift count so we need to do it ourselves. if (second.IsRegister()) { Register second_reg = second.AsRegister(); - __ and_(second_reg, second_reg, ShifterOperand(kMaxIntShiftValue)); + __ and_(out_reg, second_reg, ShifterOperand(kMaxIntShiftValue)); if (op->IsShl()) { - __ Lsl(out_reg, first_reg, second_reg); + __ Lsl(out_reg, first_reg, out_reg); } else if (op->IsShr()) { - __ Asr(out_reg, first_reg, second_reg); + __ Asr(out_reg, first_reg, out_reg); } else { - __ Lsr(out_reg, first_reg, second_reg); + __ Lsr(out_reg, first_reg, out_reg); } } else { int32_t cst = second.GetConstant()->AsIntConstant()->GetValue(); @@ -2524,44 +2526,44 @@ void InstructionCodeGeneratorARM::HandleShift(HBinaryOperation* op) { Register second_reg = second.AsRegister(); if (op->IsShl()) { + __ and_(o_l, second_reg, ShifterOperand(kMaxLongShiftValue)); // Shift the high part - __ and_(second_reg, second_reg, ShifterOperand(63)); - __ Lsl(o_h, high, second_reg); + __ Lsl(o_h, high, o_l); // Shift the low part and `or` what overflew on the high part - __ rsb(temp, second_reg, ShifterOperand(32)); + __ rsb(temp, o_l, ShifterOperand(kArmBitsPerWord)); __ Lsr(temp, low, temp); __ orr(o_h, o_h, ShifterOperand(temp)); // If the shift is > 32 bits, override the high part - __ subs(temp, second_reg, ShifterOperand(32)); + __ subs(temp, o_l, ShifterOperand(kArmBitsPerWord)); __ it(PL); __ Lsl(o_h, low, temp, false, PL); // Shift the low part - __ Lsl(o_l, low, second_reg); + __ Lsl(o_l, low, o_l); } else if (op->IsShr()) { + __ and_(o_h, second_reg, ShifterOperand(kMaxLongShiftValue)); // Shift the low part - __ and_(second_reg, second_reg, ShifterOperand(63)); - __ Lsr(o_l, low, second_reg); + __ Lsr(o_l, low, o_h); // Shift the high part and `or` what underflew on the low part - __ rsb(temp, second_reg, ShifterOperand(32)); + __ rsb(temp, o_h, ShifterOperand(kArmBitsPerWord)); __ Lsl(temp, high, temp); __ orr(o_l, o_l, ShifterOperand(temp)); // If the shift is > 32 bits, override the low part - __ subs(temp, second_reg, ShifterOperand(32)); + __ subs(temp, o_h, ShifterOperand(kArmBitsPerWord)); __ it(PL); __ Asr(o_l, high, temp, false, PL); // Shift the high part - __ Asr(o_h, high, second_reg); + __ Asr(o_h, high, o_h); } else { + __ and_(o_h, second_reg, ShifterOperand(kMaxLongShiftValue)); // same as Shr except we use `Lsr`s and not `Asr`s - __ and_(second_reg, second_reg, ShifterOperand(63)); - __ Lsr(o_l, low, second_reg); - __ rsb(temp, second_reg, ShifterOperand(32)); + __ Lsr(o_l, low, o_h); + __ rsb(temp, o_h, ShifterOperand(kArmBitsPerWord)); __ Lsl(temp, high, temp); __ orr(o_l, o_l, ShifterOperand(temp)); - __ subs(temp, second_reg, ShifterOperand(32)); + __ subs(temp, o_h, ShifterOperand(kArmBitsPerWord)); __ it(PL); __ Lsr(o_l, high, temp, false, PL); - __ Lsr(o_h, high, second_reg); + __ Lsr(o_h, high, o_h); } break; } diff --git a/compiler/optimizing/code_generator_arm.h b/compiler/optimizing/code_generator_arm.h index c410fa80b..2fe464daf 100644 --- a/compiler/optimizing/code_generator_arm.h +++ b/compiler/optimizing/code_generator_arm.h @@ -32,6 +32,7 @@ class SlowPathCodeARM; // Use a local definition to prevent copying mistakes. static constexpr size_t kArmWordSize = kArmPointerSize; +static constexpr size_t kArmBitsPerWord = kArmWordSize * kBitsPerByte; static constexpr Register kParameterCoreRegisters[] = { R1, R2, R3 }; static constexpr size_t kParameterCoreRegistersLength = arraysize(kParameterCoreRegisters); diff --git a/test/514-shifts/expected.txt b/test/514-shifts/expected.txt new file mode 100644 index 000000000..e69de29bb diff --git a/test/514-shifts/info.txt b/test/514-shifts/info.txt new file mode 100644 index 000000000..eb93c5f15 --- /dev/null +++ b/test/514-shifts/info.txt @@ -0,0 +1,2 @@ +Regression test for optimizing that used to miscompile +shifts on ARM. diff --git a/test/514-shifts/src/Main.java b/test/514-shifts/src/Main.java new file mode 100644 index 000000000..6c44eaba2 --- /dev/null +++ b/test/514-shifts/src/Main.java @@ -0,0 +1,106 @@ +/* + * Copyright (C) 2015 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. + */ + +class Main { + public static void main(String[] args) { + testIntShiftRight(); + testIntShiftLeft(); + testIntUnsignedShiftRight(); + testLongShiftRight(); + testLongShiftLeft(); + testLongUnsignedShiftRight(); + } + + public static void testIntShiftLeft() { + int a = myField; + int b = myOtherField << a; + if (b != -2147483648) { + throw new Error("Expected -2147483648, got " + b); + } + if (a != 0xFFF) { + throw new Error("Expected 0xFFF, got " + a); + } + } + + public static void testIntShiftRight() { + int a = myField; + int b = myOtherField >> a; + if (b != 0) { + throw new Error("Expected 0, got " + b); + } + if (a != 0xFFF) { + throw new Error("Expected 0xFFF, got " + a); + } + } + + public static void testIntUnsignedShiftRight() { + int a = myField; + int b = myOtherField >>> a; + if (b != 0) { + throw new Error("Expected 0, got " + b); + } + if (a != 0xFFF) { + throw new Error("Expected 0xFFF, got " + a); + } + } + + public static void testLongShiftLeft() { + long a = myLongField; + long b = myOtherLongField << a; + if (b != 0x2468ACF13579BDEL) { + throw new Error("Expected 0x2468ACF13579BDEL, got " + b); + } + // The int conversion will be GVN'ed with the one required + // by Java specification of long shift left. + if ((int)a != 0x41) { + throw new Error("Expected 0x41, got " + a); + } + } + + public static void testLongShiftRight() { + long a = myLongField; + long b = myOtherLongField >> a; + if (b != 0x91A2B3C4D5E6F7L) { + throw new Error("Expected 0x91A2B3C4D5E6F7L, got " + b); + } + // The int conversion will be GVN'ed with the one required + // by Java specification of long shift right. + if ((int)a != 0x41) { + throw new Error("Expected 0x41, got " + a); + } + } + + public static void testLongUnsignedShiftRight() { + long a = myLongField; + long b = myOtherLongField >>> a; + if (b != 0x91A2B3C4D5E6F7L) { + throw new Error("Expected 0x91A2B3C4D5E6F7L, got " + b); + } + // The int conversion will be GVN'ed with the one required + // by Java specification of long shift right. + if ((int)a != 0x41) { + throw new Error("Expected 0x41, got " + a); + } + } + + static int myField = 0xFFF; + static int myOtherField = 0x1; + + // Use a value that will need to be masked before doing the shift. + // The maximum shift is 0x3F. + static long myLongField = 0x41; + static long myOtherLongField = 0x123456789abcdefL; +} -- 2.11.0