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

diff --git a/compiler/optimizing/code_generator_arm.cc b/compiler/optimizing/code_generator_arm.cc
index 2b1131d..3f28e64 100644 (file)
--- 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<Register>();
-        __ 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<Register>();
 
       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 c410fa8..2fe464d 100644 (file)
--- 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 (file)
index 0000000..e69de29

diff --git a/test/514-shifts/info.txt b/test/514-shifts/info.txt
new file mode 100644 (file)
index 0000000..eb93c5f
--- /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 (file)
index 0000000..6c44eab
--- /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;
+}