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
diff --git a/compiler/optimizing/instruction_simplifier.cc b/compiler/optimizing/instruction_simplifier.cc
index d39e9ad..ae778b4 100644
--- a/compiler/optimizing/instruction_simplifier.cc
+++ b/compiler/optimizing/instruction_simplifier.cc
@@ -621,8 +621,7 @@
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 @@
// 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 @@
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 5d4bb7a..667b35f 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 @@
}
// (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 @@
/// 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 @@
/// 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 @@
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();
}
}