Revert constant splitting heuristics change as it breaks gmscore builds.
R=sgjesse@google.com
Change-Id: I5efdacc973749c28b0d2210032119b70d0f6b65b
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
index 23b6b70..6f94b1e 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/IRConverter.java
@@ -459,7 +459,7 @@
codeRewriter.commonSubexpressionElimination(code);
codeRewriter.simplifyArrayConstruction(code);
codeRewriter.rewriteMoveResult(code);
- codeRewriter.splitRangeInvokeConstants(code);
+ codeRewriter.splitConstants(code);
codeRewriter.foldConstants(code);
codeRewriter.rewriteSwitch(code);
codeRewriter.simplifyIf(code);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index 408ebc7..e1c0ccb 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -63,6 +63,7 @@
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
+import java.util.IdentityHashMap;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
@@ -651,35 +652,68 @@
assert code.isConsistentSSA();
}
- // Split constants that flow into ranged invokes. This gives the register allocator more
- // freedom in assigning register to ranged invokes which can greatly reduce the number
- // of register needed (and thereby code size as well).
- public void splitRangeInvokeConstants(IRCode code) {
+ // Constants are canonicalized in the entry block. We split some of them when it is likely
+ // that having them canonicalized in the entry block will lead to poor code quality.
+ public void splitConstants(IRCode code) {
for (BasicBlock block : code.blocks) {
- InstructionListIterator it = block.listIterator();
- while (it.hasNext()) {
- Instruction current = it.next();
- if (current.isInvoke() && current.asInvoke().requiredArgumentRegisters() > 5) {
- Invoke invoke = current.asInvoke();
- it.previous();
- Map<ConstNumber, ConstNumber> oldToNew = new HashMap<>();
- for (int i = 0; i < invoke.inValues().size(); i++) {
- Value value = invoke.inValues().get(i);
- if (value.isConstNumber() && value.numberOfUsers() > 1) {
- ConstNumber definition = value.getConstInstruction().asConstNumber();
- Value originalValue = definition.outValue();
- ConstNumber newNumber = oldToNew.get(definition);
- if (newNumber == null) {
- newNumber = ConstNumber.copyOf(code, definition);
- it.add(newNumber);
- oldToNew.put(definition, newNumber);
- }
- invoke.inValues().set(i, newNumber.outValue());
- originalValue.removeUser(invoke);
- newNumber.outValue().addUser(invoke);
+ // Split constants that flow into phis. It is likely that these constants will have moves
+ // generated for them anyway and we might as well insert a const instruction in the right
+ // predecessor block.
+ splitPhiConstants(code, block);
+ // Split constants that flow into ranged invokes. This gives the register allocator more
+ // freedom in assigning register to ranged invokes which can greatly reduce the number
+ // of register needed (and thereby code size as well).
+ splitRangedInvokeConstants(code, block);
+ }
+ }
+
+ private void splitRangedInvokeConstants(IRCode code, BasicBlock block) {
+ InstructionListIterator it = block.listIterator();
+ while (it.hasNext()) {
+ Instruction current = it.next();
+ if (current.isInvoke() && current.asInvoke().requiredArgumentRegisters() > 5) {
+ Invoke invoke = current.asInvoke();
+ it.previous();
+ Map<ConstNumber, ConstNumber> oldToNew = new HashMap<>();
+ for (int i = 0; i < invoke.inValues().size(); i++) {
+ Value value = invoke.inValues().get(i);
+ if (value.isConstNumber() && value.numberOfUsers() > 1) {
+ ConstNumber definition = value.getConstInstruction().asConstNumber();
+ Value originalValue = definition.outValue();
+ ConstNumber newNumber = oldToNew.get(definition);
+ if (newNumber == null) {
+ newNumber = ConstNumber.copyOf(code, definition);
+ it.add(newNumber);
+ oldToNew.put(definition, newNumber);
}
+ invoke.inValues().set(i, newNumber.outValue());
+ originalValue.removeUser(invoke);
+ newNumber.outValue().addUser(invoke);
}
- it.next();
+ }
+ it.next();
+ }
+ }
+ }
+
+ private void splitPhiConstants(IRCode code, BasicBlock block) {
+ for (int i = 0; i < block.getPredecessors().size(); i++) {
+ Map<ConstNumber, ConstNumber> oldToNew = new IdentityHashMap<>();
+ BasicBlock predecessor = block.getPredecessors().get(i);
+ for (Phi phi : block.getPhis()) {
+ Value operand = phi.getOperand(i);
+ if (!operand.isPhi() && operand.isConstNumber()) {
+ ConstNumber definition = operand.getConstInstruction().asConstNumber();
+ ConstNumber newNumber = oldToNew.get(definition);
+ Value originalValue = definition.outValue();
+ if (newNumber == null) {
+ newNumber = ConstNumber.copyOf(code, definition);
+ oldToNew.put(definition, newNumber);
+ insertConstantInBlock(newNumber, predecessor);
+ }
+ phi.getOperands().set(i, newNumber.outValue());
+ originalValue.removePhiUser(phi);
+ newNumber.outValue().addPhiUser(phi);
}
}
}
@@ -696,7 +730,7 @@
List<Instruction> toInsertInThisBlock = new ArrayList<>();
while (it.hasNext()) {
Instruction instruction = it.next();
- if (instruction.isConstNumber() && instruction.outValue().numberOfAllUsers() > 0) {
+ if (instruction.isConstNumber()) {
// Collect the blocks for all users of the constant.
List<BasicBlock> userBlocks = new LinkedList<>();
for (Instruction user : instruction.outValue().uniqueUsers()) {
diff --git a/src/test/java/com/android/tools/r8/smali/IfSimplificationTest.java b/src/test/java/com/android/tools/r8/smali/IfSimplificationTest.java
index 919f569..6b4a431 100644
--- a/src/test/java/com/android/tools/r8/smali/IfSimplificationTest.java
+++ b/src/test/java/com/android/tools/r8/smali/IfSimplificationTest.java
@@ -180,8 +180,8 @@
":return",
" return v0");
DexCode code = method.getCode().asDexCode();
- assertEquals(10, code.instructions.length);
- assertTrue(code.instructions[9] instanceof Return);
+ assertEquals(12, code.instructions.length);
+ assertTrue(code.instructions[11] instanceof Return);
}
@Test
@@ -443,6 +443,6 @@
DexCode code = method.getCode().asDexCode();
// TODO(sgjesse): Maybe this test is too fragile, as it leaves quite a lot of code, so the
// expectation might need changing with other optimizations.
- assertEquals(27, code.instructions.length);
+ assertEquals(29, code.instructions.length);
}
}
diff --git a/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java b/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
index e6bfbd0..f10add0 100644
--- a/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
+++ b/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
@@ -7,6 +7,7 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertTrue;
+import com.android.tools.r8.R8;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.code.Const;
import com.android.tools.r8.code.Const4;
@@ -291,11 +292,12 @@
DexCode code = method.getCode().asDexCode();
if (key == 0) {
assertEquals(5, code.instructions.length);
- assertTrue(code.instructions[2] instanceof IfEqz);
+ assertTrue(code.instructions[0] instanceof IfEqz);
} else {
assertEquals(6, code.instructions.length);
- assertTrue(some16BitConst(code.instructions[2]));
- assertTrue(code.instructions[3] instanceof IfEq);
+ assertTrue(some16BitConst(code.instructions[0]));
+ assertTrue(code.instructions[1] instanceof IfEq);
+ assertTrue(code.instructions[2] instanceof Const4);
}
}
@@ -349,9 +351,9 @@
DexEncodedMethod method = getMethod(app, signature);
DexCode code = method.getCode().asDexCode();
if (twoCaseWillUsePackedSwitch(key1, key2)) {
- assertTrue(code.instructions[3] instanceof PackedSwitch);
+ assertTrue(code.instructions[0] instanceof PackedSwitch);
} else {
- assertTrue(code.instructions[3] instanceof SparseSwitch);
+ assertTrue(code.instructions[0] instanceof SparseSwitch);
}
}
@@ -421,23 +423,10 @@
MethodSignature signature = new MethodSignature("Test", "test", "int", ImmutableList.of("int"));
DexEncodedMethod method = getMethod(app, signature);
DexCode code = method.getCode().asDexCode();
- int packedSwithsCount = 0;
- int sparseSwithsCount = 0;
- for (Instruction instruction : code.instructions) {
- if (instruction instanceof PackedSwitch) {
- packedSwithsCount++;
- }
- if (instruction instanceof SparseSwitch) {
- sparseSwithsCount++;
- }
- }
-
if (keyStep <= 2) {
- assert packedSwithsCount == 1;
- assert sparseSwithsCount == 0;
+ assertTrue(code.instructions[0] instanceof PackedSwitch);
} else {
- assert packedSwithsCount == 0;
- assert sparseSwithsCount == 1;
+ assertTrue(code.instructions[0] instanceof SparseSwitch);
}
}
