Don't change Add/Xor into Ror for constant 0

The TryReplaceWithRotateRegisterNegPattern method was providing
the wrong optimization for the constant 0. Shifting using a
negative value is odd as it only takes into account the 5 least
significant bits. This means that a shift by -31 is the same as
a shift of 1 (-30 with 2, and so on). Said method was taking
advantage of this to perform an optimization, but it failed to
realize that this doesn't work with 0. In the end, this
optimization was basically an HAdd or a HXor instruction.

As a note, it provided the right result for HOr, just because
the result of the HOr instruction would be a no-op.

Bug: 325899471
Bug: 324445644
Fixes: 325899471
Fixes: 324445644
Test: art/test/testrunner/testrunner.py --host --64 --optimizing -b
Change-Id: Iaf1385beec85d563539b99b28b5debb04c23bff8

2 files changed, 80 insertions, 21 deletions

diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc
index d39e9ad32f..ae778b421a 100644
--- a/compiler/optimizing/instruction_simplifier.cc
+++ b/compiler/optimizing/instruction_simplifier.cc
@@ -621,8 +621,7 @@ bool InstructionSimplifierVisitor::TryReplaceWithRotateConstantPattern(HBinaryOp
   size_t rdist = Int64FromConstant(ushr->GetRight()->AsConstant());
   size_t ldist = Int64FromConstant(shl->GetRight()->AsConstant());
   if (((ldist + rdist) & (reg_bits - 1)) == 0) {
-    ReplaceRotateWithRor(op, ushr, shl);
-    return true;
+    return ReplaceRotateWithRor(op, ushr, shl);
   }
   return false;
 }
@@ -643,6 +642,10 @@ bool InstructionSimplifierVisitor::TryReplaceWithRotateConstantPattern(HBinaryOp
 //    OP   dst, dst, tmp
 // with
 //    Ror  dst, x,   d
+//
+// Requires `d` to be non-zero for the HAdd and HXor case. If `d` is 0 the shifts and rotate are
+// no-ops and the `OP` is never executed. This is fine for HOr since the result is the same, but the
+// result is different for HAdd and HXor.
 bool InstructionSimplifierVisitor::TryReplaceWithRotateRegisterNegPattern(HBinaryOperation* op,
                                                                           HUShr* ushr,
                                                                           HShl* shl) {
@@ -650,11 +653,20 @@ bool InstructionSimplifierVisitor::TryReplaceWithRotateRegisterNegPattern(HBinar
   DCHECK(ushr->GetRight()->IsNeg() || shl->GetRight()->IsNeg());
   bool neg_is_left = shl->GetRight()->IsNeg();
   HNeg* neg = neg_is_left ? shl->GetRight()->AsNeg() : ushr->GetRight()->AsNeg();
-  // And the shift distance being negated is the distance being shifted the other way.
-  if (neg->InputAt(0) == (neg_is_left ? ushr->GetRight() : shl->GetRight())) {
-    ReplaceRotateWithRor(op, ushr, shl);
+  HInstruction* value = neg->InputAt(0);
+
+  // The shift distance being negated is the distance being shifted the other way.
+  if (value != (neg_is_left ? ushr->GetRight() : shl->GetRight())) {
+    return false;
   }
-  return false;
+
+  const bool needs_non_zero_value = !op->IsOr();
+  if (needs_non_zero_value) {
+    if (!value->IsConstant() || value->AsConstant()->IsArithmeticZero()) {
+      return false;
+    }
+  }
+  return ReplaceRotateWithRor(op, ushr, shl);
 }
 
 // Try replacing code looking like (x >>> d OP x << (#bits - d)):
diff --git a/test/557-checker-instruct-simplifier-ror/src/Main.java b/test/557-checker-instruct-simplifier-ror/src/Main.java
index 5d4bb7ab33..667b35f5d9 100644
--- a/test/557-checker-instruct-simplifier-ror/src/Main.java
+++ b/test/557-checker-instruct-simplifier-ror/src/Main.java
@@ -503,6 +503,7 @@ public class Main {
   }
 
   //  (j << distance) + (j >>> -distance)
+  // We can't perform the optimization as distance might be `0`, resulting in the wrong value.
 
   /// CHECK-START: long Main.rol_long_reg_v_negv_add(long, int) instruction_simplifier (before)
   /// CHECK:          <<ArgValue:j\d+>>     ParameterValue
@@ -516,19 +517,17 @@ public class Main {
   /// CHECK-START: long Main.rol_long_reg_v_negv_add(long, int) instruction_simplifier (after)
   /// CHECK:          <<ArgValue:j\d+>>     ParameterValue
   /// CHECK:          <<ArgDistance:i\d+>>  ParameterValue
-  /// CHECK:          <<Neg:i\d+>>          Neg [<<ArgDistance>>]
-  /// CHECK:          <<Ror:j\d+>>          Ror [<<ArgValue>>,<<Neg>>]
-  /// CHECK:                                Return [<<Ror>>]
-
-  /// CHECK-START: long Main.rol_long_reg_v_negv_add(long, int) instruction_simplifier (after)
-  /// CHECK-NOT:  Add
-  /// CHECK-NOT:  Shl
-  /// CHECK-NOT:  UShr
+  /// CHECK-DAG:      <<Neg:i\d+>>          Neg [<<ArgDistance>>]
+  /// CHECK-DAG:      <<UShr:j\d+>>         UShr [<<ArgValue>>,<<Neg>>]
+  /// CHECK-DAG:      <<Shl:j\d+>>          Shl [<<ArgValue>>,<<ArgDistance>>]
+  /// CHECK:          <<Add:j\d+>>          Add [<<Shl>>,<<UShr>>]
+  /// CHECK:                                Return [<<Add>>]
   public static long rol_long_reg_v_negv_add(long value, int distance) {
     return (value << distance) + (value >>> -distance);
   }
 
   //  (j << distance) ^ (j >>> -distance)
+  // We can't perform the optimization as distance might be `0`, resulting in the wrong value.
 
   /// CHECK-START: long Main.rol_long_reg_v_negv_xor(long, int) instruction_simplifier (before)
   /// CHECK:          <<ArgValue:j\d+>>     ParameterValue
@@ -542,18 +541,60 @@ public class Main {
   /// CHECK-START: long Main.rol_long_reg_v_negv_xor(long, int) instruction_simplifier (after)
   /// CHECK:          <<ArgValue:j\d+>>     ParameterValue
   /// CHECK:          <<ArgDistance:i\d+>>  ParameterValue
-  /// CHECK:          <<Neg:i\d+>>          Neg [<<ArgDistance>>]
-  /// CHECK:          <<Ror:j\d+>>          Ror [<<ArgValue>>,<<Neg>>]
-  /// CHECK:                                Return [<<Ror>>]
+  /// CHECK-DAG:      <<Neg:i\d+>>          Neg [<<ArgDistance>>]
+  /// CHECK-DAG:      <<UShr:j\d+>>         UShr [<<ArgValue>>,<<Neg>>]
+  /// CHECK-DAG:      <<Shl:j\d+>>          Shl [<<ArgValue>>,<<ArgDistance>>]
+  /// CHECK:          <<Xor:j\d+>>          Xor [<<Shl>>,<<UShr>>]
+  /// CHECK:                                Return [<<Xor>>]
 
-  /// CHECK-START: long Main.rol_long_reg_v_negv_xor(long, int) instruction_simplifier (after)
-  /// CHECK-NOT:  Xor
-  /// CHECK-NOT:  Shl
-  /// CHECK-NOT:  UShr
   public static long rol_long_reg_v_negv_xor(long value, int distance) {
     return (value << distance) ^ (value >>> -distance);
   }
 
+  /// CHECK-START: void Main.$noinline$testDontOptimizeAddIntoRotate_Int() disassembly (after)
+  /// CHECK-NOT: Ror
+  public static void $noinline$testDontOptimizeAddIntoRotate_Int() {
+      int distance = returnFalse() ? 1 : 0;
+      int value = -512667375;
+      int expected_result = 2 * value;
+      int result = (value >>> distance) + (value << -distance);
+      assertIntEquals(expected_result, result);
+  }
+
+  /// CHECK-START: void Main.$noinline$testDontOptimizeAddIntoRotate_Long() disassembly (after)
+  /// CHECK-NOT: Ror
+  public static void $noinline$testDontOptimizeAddIntoRotate_Long() {
+      int distance = returnFalse() ? 1 : 0;
+      long value = -512667375L;
+      long expected_result = 2L * value;
+      long result = (value >>> distance) + (value << -distance);
+      assertLongEquals(expected_result, result);
+  }
+
+  /// CHECK-START: void Main.$noinline$testDontOptimizeXorIntoRotate_Int() disassembly (after)
+  /// CHECK-NOT: Ror
+  public static void $noinline$testDontOptimizeXorIntoRotate_Int() {
+      int distance = returnFalse() ? 1 : 0;
+      int value = -512667375;
+      int expected_result = 0;
+      int result = (value >>> distance) ^ (value << -distance);
+      assertIntEquals(expected_result, result);
+  }
+
+  /// CHECK-START: void Main.$noinline$testDontOptimizeXorIntoRotate_Long() disassembly (after)
+  /// CHECK-NOT: Ror
+  public static void $noinline$testDontOptimizeXorIntoRotate_Long() {
+      int distance = returnFalse() ? 1 : 0;
+      long value = -512667375L;
+      long expected_result = 0;
+      long result = (value >>> distance) ^ (value << -distance);
+      assertLongEquals(expected_result, result);
+  }
+
+  static boolean returnFalse() {
+    return false;
+  }
+
   public static void main(String[] args) {
     assertIntEquals(2, ror_int_constant_c_c(8));
     assertIntEquals(2, ror_int_constant_c_c_0(8));
@@ -581,5 +622,11 @@ public class Main {
 
     assertLongEquals(32L, rol_long_reg_v_negv_add(8L, 2));
     assertLongEquals(32L, rol_long_reg_v_negv_xor(8L, 2));
+
+    $noinline$testDontOptimizeAddIntoRotate_Int();
+    $noinline$testDontOptimizeAddIntoRotate_Long();
+
+    $noinline$testDontOptimizeXorIntoRotate_Int();
+    $noinline$testDontOptimizeXorIntoRotate_Long();
   }
 }