Remove old constant splitting heuristics that doesn't help.
We changed the way we move around constant instructions and
after that change this heuristics hurts more than it helps.
Getting rid of it is nice. We should just let the register
allocator deal with this.
Change-Id: I142765cf73cd3057beae1c24ce2340defce3b92e
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 6f94b1e..23b6b70 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.splitConstants(code);
+ codeRewriter.splitRangeInvokeConstants(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 e1c0ccb..48a64b8 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,7 +63,6 @@
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;
@@ -652,68 +651,35 @@
assert code.isConsistentSSA();
}
- // 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) {
+ // 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) {
for (BasicBlock block : code.blocks) {
- // 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);
+ 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);
}
- invoke.inValues().set(i, newNumber.outValue());
- originalValue.removeUser(invoke);
- newNumber.outValue().addUser(invoke);
}
- }
- 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);
+ it.next();
}
}
}
@@ -730,7 +696,7 @@
List<Instruction> toInsertInThisBlock = new ArrayList<>();
while (it.hasNext()) {
Instruction instruction = it.next();
- if (instruction.isConstNumber()) {
+ if (instruction.isConstNumber() && instruction.outValue().numberOfAllUsers() != 0) {
// Collect the blocks for all users of the constant.
List<BasicBlock> userBlocks = new LinkedList<>();
for (Instruction user : instruction.outValue().uniqueUsers()) {
diff --git a/src/main/java/com/android/tools/r8/ir/regalloc/LiveIntervals.java b/src/main/java/com/android/tools/r8/ir/regalloc/LiveIntervals.java
index 12db3d9..9a810d9 100644
--- a/src/main/java/com/android/tools/r8/ir/regalloc/LiveIntervals.java
+++ b/src/main/java/com/android/tools/r8/ir/regalloc/LiveIntervals.java
@@ -96,10 +96,22 @@
// these computations. We use the unadjusted real register number to make sure that
// isRematerializable for the same intervals does not change from one phase of
// compilation to the next.
+ if (getMaxNonSpilledRegister() == NO_REGISTER) {
+ assert allSplitsAreSpilled();
+ return true;
+ }
int max = registerAllocator.unadjustedRealRegisterFromAllocated(getMaxNonSpilledRegister());
return max < Constants.U8BIT_MAX;
}
+ private boolean allSplitsAreSpilled() {
+ assert isSpilled();
+ for (LiveIntervals splitChild : splitChildren) {
+ assert splitChild.isSpilled();
+ }
+ return true;
+ }
+
public boolean isSpilledAndRematerializable(LinearScanRegisterAllocator allocator) {
return isSpilled() && isRematerializable(allocator);
}
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 6b4a431..919f569 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(12, code.instructions.length);
- assertTrue(code.instructions[11] instanceof Return);
+ assertEquals(10, code.instructions.length);
+ assertTrue(code.instructions[9] 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(29, code.instructions.length);
+ assertEquals(27, 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 f10add0..7f89da7 100644
--- a/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
+++ b/src/test/java/com/android/tools/r8/smali/SwitchRewritingTest.java
@@ -7,7 +7,6 @@
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;
@@ -292,12 +291,11 @@
DexCode code = method.getCode().asDexCode();
if (key == 0) {
assertEquals(5, code.instructions.length);
- assertTrue(code.instructions[0] instanceof IfEqz);
+ assertTrue(code.instructions[2] instanceof IfEqz);
} else {
assertEquals(6, code.instructions.length);
- assertTrue(some16BitConst(code.instructions[0]));
- assertTrue(code.instructions[1] instanceof IfEq);
- assertTrue(code.instructions[2] instanceof Const4);
+ assertTrue(some16BitConst(code.instructions[2]));
+ assertTrue(code.instructions[3] instanceof IfEq);
}
}
@@ -351,9 +349,9 @@
DexEncodedMethod method = getMethod(app, signature);
DexCode code = method.getCode().asDexCode();
if (twoCaseWillUsePackedSwitch(key1, key2)) {
- assertTrue(code.instructions[0] instanceof PackedSwitch);
+ assertTrue(code.instructions[3] instanceof PackedSwitch);
} else {
- assertTrue(code.instructions[0] instanceof SparseSwitch);
+ assertTrue(code.instructions[3] instanceof SparseSwitch);
}
}
@@ -423,10 +421,22 @@
MethodSignature signature = new MethodSignature("Test", "test", "int", ImmutableList.of("int"));
DexEncodedMethod method = getMethod(app, signature);
DexCode code = method.getCode().asDexCode();
+ int packedSwitchCount = 0;
+ int sparseSwitchCount = 0;
+ for (Instruction instruction : code.instructions) {
+ if (instruction instanceof PackedSwitch) {
+ packedSwitchCount++;
+ }
+ if (instruction instanceof SparseSwitch) {
+ sparseSwitchCount++;
+ }
+ }
if (keyStep <= 2) {
- assertTrue(code.instructions[0] instanceof PackedSwitch);
+ assertEquals(1, packedSwitchCount);
+ assertEquals(0, sparseSwitchCount);
} else {
- assertTrue(code.instructions[0] instanceof SparseSwitch);
+ assertEquals(0, packedSwitchCount);
+ assertEquals(1, sparseSwitchCount);
}
}
