Fix Binop rewriter
- TBR red bots
- Issues with sub with left constant
Change-Id: Ieb7cbb0db0c5857c9adffbdb229b73fdee1569de
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/passes/BinopRewriter.java b/src/main/java/com/android/tools/r8/ir/conversion/passes/BinopRewriter.java
index f642a95..b95f09d 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/passes/BinopRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/passes/BinopRewriter.java
@@ -275,25 +275,26 @@
if (binop.outValue().hasDebugUsers()) {
return;
}
- ConstNumber constLeft = getConstNumber(binop.leftValue());
- ConstNumber constRight = getConstNumber(binop.rightValue());
- if ((constLeft != null && constRight != null) || (constLeft == null && constRight == null)) {
+ ConstNumber constBLeft = getConstNumber(binop.leftValue());
+ ConstNumber constBRight = getConstNumber(binop.rightValue());
+ if ((constBLeft != null && constBRight != null)
+ || (constBLeft == null && constBRight == null)) {
return;
}
- Value otherValue = constLeft == null ? binop.leftValue() : binop.rightValue();
+ Value otherValue = constBLeft == null ? binop.leftValue() : binop.rightValue();
if (otherValue.isPhi() || !otherValue.getDefinition().isBinop()) {
return;
}
Binop prevBinop = otherValue.getDefinition().asBinop();
- ConstNumber prevConstLeft = getConstNumber(prevBinop.leftValue());
- ConstNumber prevConstRight = getConstNumber(prevBinop.rightValue());
- if ((prevConstLeft != null && prevConstRight != null)
- || (prevConstLeft == null && prevConstRight == null)) {
+ ConstNumber constALeft = getConstNumber(prevBinop.leftValue());
+ ConstNumber constARight = getConstNumber(prevBinop.rightValue());
+ if ((constALeft != null && constARight != null)
+ || (constALeft == null && constARight == null)) {
return;
}
- ConstNumber constB = constLeft == null ? constRight : constLeft;
- ConstNumber constA = prevConstLeft == null ? prevConstRight : prevConstLeft;
- Value input = prevConstLeft == null ? prevBinop.leftValue() : prevBinop.rightValue();
+ ConstNumber constB = constBLeft == null ? constBRight : constBLeft;
+ ConstNumber constA = constALeft == null ? constARight : constALeft;
+ Value input = constALeft == null ? prevBinop.leftValue() : prevBinop.rightValue();
// We have two successive binops so that a,b constants, x the input and a * x * b.
if (prevBinop.getClass() == binop.getClass()) {
if (binopDescriptor.associativeAndCommutative) {
@@ -304,15 +305,16 @@
instantiateBinop(code, input, newConst, binopDescriptor));
} else if (binopDescriptor.isShift()) {
// x shift: a shift: b => x shift: (a + b) where a + b is a constant.
- if (constRight != null && prevConstRight != null) {
+ if (constBRight != null && constARight != null) {
Value newConst = addNewConstNumber(code, iterator, constB, constA, BinopDescriptor.ADD);
iterator.replaceCurrentInstruction(
instantiateBinop(code, input, newConst, binopDescriptor));
}
- } else if (binop.isSub()) {
+ } else if (binop.isSub() && constBRight != null) {
// a - x - b => (a - b) - x where (a - b) is a constant.
// x - a - b => x - (a + b) where (a + b) is a constant.
- if (prevConstRight == null) {
+ // We ignore b - (x - a) and b - (a - x) with constBRight != null.
+ if (constARight == null) {
Value newConst = addNewConstNumber(code, iterator, constA, constB, BinopDescriptor.SUB);
iterator.replaceCurrentInstruction(
instantiateBinop(code, newConst, input, BinopDescriptor.SUB));
@@ -323,16 +325,17 @@
}
}
} else {
- if (binop.isSub() && prevBinop.isAdd()) {
+ if (binop.isSub() && prevBinop.isAdd() && constBRight != null) {
// x + a - b => x + (a - b) where (a - b) is a constant.
// a + x - b => x + (a - b) where (a - b) is a constant.
+ // We ignore b - (x + a) and b - (a + x) with constBRight != null.
Value newConst = addNewConstNumber(code, iterator, constA, constB, BinopDescriptor.SUB);
iterator.replaceCurrentInstruction(
instantiateBinop(code, newConst, input, BinopDescriptor.ADD));
} else if (binop.isAdd() && prevBinop.isSub()) {
// x - a + b => x - (a - b) where (a - b) is a constant.
// a - x + b => (a + b) - x where (a + b) is a constant.
- if (prevConstLeft == null) {
+ if (constALeft == null) {
Value newConst = addNewConstNumber(code, iterator, constA, constB, BinopDescriptor.SUB);
iterator.replaceCurrentInstruction(
instantiateBinop(code, input, newConst, BinopDescriptor.SUB));
diff --git a/src/test/java/com/android/tools/r8/ir/AssociativeIntTest.java b/src/test/java/com/android/tools/r8/ir/AssociativeIntTest.java
index 8a6f9c0..84e8e70 100644
--- a/src/test/java/com/android/tools/r8/ir/AssociativeIntTest.java
+++ b/src/test/java/com/android/tools/r8/ir/AssociativeIntTest.java
@@ -75,6 +75,18 @@
"-37",
"-2147483642",
"-2147483643",
+ "-1",
+ "-41",
+ "-2147483646",
+ "-2147483647",
+ "25",
+ "-15",
+ "-2147483620",
+ "-2147483621",
+ "3",
+ "43",
+ "-2147483648",
+ "-2147483647",
"3",
"43",
"-2147483648",
@@ -152,7 +164,7 @@
for (FoundMethodSubject method :
clazz.allMethods(m -> m.getParameters().size() > 0 && m.getParameter(0).is("int"))) {
assertEquals(
- 1,
+ method.getOriginalName().contains("NotSimplified") ? 2 : 1,
method
.streamInstructions()
.filter(i -> i.isIntArithmeticBinop() || i.isIntLogicalBinop())
@@ -228,6 +240,18 @@
subAdd(42);
subAdd(Integer.MAX_VALUE);
subAdd(Integer.MIN_VALUE);
+ addSubNotSimplified_1(2);
+ addSubNotSimplified_1(42);
+ addSubNotSimplified_1(Integer.MAX_VALUE);
+ addSubNotSimplified_1(Integer.MIN_VALUE);
+ addSubNotSimplified_2(2);
+ addSubNotSimplified_2(42);
+ addSubNotSimplified_2(Integer.MAX_VALUE);
+ addSubNotSimplified_2(Integer.MIN_VALUE);
+ addSubNotSimplified_3(2);
+ addSubNotSimplified_3(42);
+ addSubNotSimplified_3(Integer.MAX_VALUE);
+ addSubNotSimplified_3(Integer.MIN_VALUE);
addSub2(2);
addSub2(42);
addSub2(Integer.MAX_VALUE);
@@ -342,6 +366,21 @@
}
@NeverInline
+ public static void addSubNotSimplified_1(int x) {
+ System.out.println(14 - (x + 13));
+ }
+
+ @NeverInline
+ public static void addSubNotSimplified_2(int x) {
+ System.out.println(14 - (x - 13));
+ }
+
+ @NeverInline
+ public static void addSubNotSimplified_3(int x) {
+ System.out.println(14 - (13 - x));
+ }
+
+ @NeverInline
public static void subAdd(int x) {
System.out.println(3 - x + 2);
}
diff --git a/src/test/java/com/android/tools/r8/ir/AssociativeLongTest.java b/src/test/java/com/android/tools/r8/ir/AssociativeLongTest.java
index 22ea7e8..1d041b8 100644
--- a/src/test/java/com/android/tools/r8/ir/AssociativeLongTest.java
+++ b/src/test/java/com/android/tools/r8/ir/AssociativeLongTest.java
@@ -155,7 +155,7 @@
1,
method
.streamInstructions()
- .filter(i -> i.isIntArithmeticBinop() || i.isLongLogicalBinop())
+ .filter(i -> i.isLongArithmeticBinop() || i.isLongLogicalBinop())
.count());
}
}
