Remove catch handler deduplication from LinearScanRegisterAllocator
- the optimizations is mostly subsumed by the improvement suffix sharing and identical basic block sharing.
- the code size regression is because this optimizations was not considering a bug in Android N to P devices.
Change-Id: I2170237e6b512e9f9f457474edf538340a5b6720
diff --git a/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java b/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
index fac67df..7bd8312 100644
--- a/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
+++ b/src/main/java/com/android/tools/r8/ir/regalloc/LinearScanRegisterAllocator.java
@@ -373,7 +373,6 @@
insertRangeInvokeMoves();
insertInitializedThisMove();
ImmutableList<BasicBlock> blocks = computeLivenessInformation();
- dedupCatchHandlerBlocks();
timing.end();
timing.begin("Allocate");
performAllocation();
@@ -3491,77 +3490,6 @@
code.blocks.forEach(BasicBlock::clearUserInfo);
}
- private void dedupCatchHandlerBlocks() {
- List<BasicBlock> candidateBlocks = new ArrayList<>();
- for (BasicBlock block : code.getBlocks()) {
- if (block.hasUniquePredecessor()
- && block.getUniquePredecessor().hasCatchSuccessor(block)
- && liveAtEntrySets.get(block).isEmpty()
- && block.size() <= 2) {
- candidateBlocks.add(block);
- }
- }
- if (candidateBlocks.isEmpty()) {
- return;
- }
- Set<BasicBlock> removedBlocks = Sets.newIdentityHashSet();
- for (BasicBlock candidateBlock : candidateBlocks) {
- assert !removedBlocks.contains(candidateBlock);
- BasicBlock equivalentBlock = null;
- for (BasicBlock block : candidateBlocks) {
- if (block == candidateBlock || removedBlocks.contains(block)) {
- continue;
- }
- if (isEquivalentCatchHandlers(candidateBlock, block)) {
- equivalentBlock = block;
- break;
- }
- }
- if (equivalentBlock == null) {
- continue;
- }
- assert !candidateBlock.hasCatchHandlers();
- removedBlocks.add(candidateBlock);
- for (BasicBlock tryBlock : candidateBlock.getPredecessors()) {
- tryBlock.replaceSuccessor(candidateBlock, equivalentBlock);
- if (!equivalentBlock.getPredecessors().contains(tryBlock)) {
- equivalentBlock.getMutablePredecessors().add(tryBlock);
- }
- }
- for (BasicBlock successor : candidateBlock.getSuccessors()) {
- int index = successor.getPredecessors().indexOf(candidateBlock);
- successor.getMutablePredecessors().remove(index);
- for (Phi phi : successor.getPhis()) {
- phi.removeOperand(index);
- }
- }
- }
- code.removeBlocks(removedBlocks);
- }
-
- // TODO(b/153139043): Generalize this. Maybe use BasicBlock subsumption.
- private boolean isEquivalentCatchHandlers(BasicBlock block, BasicBlock other) {
- assert liveAtEntrySets.get(block).isEmpty();
- assert liveAtEntrySets.get(other).isEmpty();
- if (block.size() != other.size() || block.size() > 2) {
- return false;
- }
- if (block.size() == 2) {
- if (!block.entry().isMoveException() || !other.entry().isMoveException()) {
- return false;
- }
- }
- if (block.exit().isGoto()
- && other.exit().isGoto()
- && block.getUniqueNormalSuccessor() == other.getUniqueNormalSuccessor()) {
- return true;
- }
- if (block.exit().isReturn() && other.exit().isReturn()) {
- return true;
- }
- return false;
- }
-
// Rewrites casts on the form "lhs = (T) rhs" into "(T) rhs" and replaces the uses of lhs by rhs.
// This transformation helps to ensure that we do not insert unnecessary moves in bridge methods
// with an invoke-range instruction, since all the arguments to the invoke-range instruction will
diff --git a/src/test/java8/regress/com/android/tools/r8/regress/b120164595/B120164595.java b/src/test/java8/regress/com/android/tools/r8/regress/b120164595/B120164595.java
index e63bcbb..9d6a405 100644
--- a/src/test/java8/regress/com/android/tools/r8/regress/b120164595/B120164595.java
+++ b/src/test/java8/regress/com/android/tools/r8/regress/b120164595/B120164595.java
@@ -4,16 +4,26 @@
package com.android.tools.r8.regress.b120164595;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
-import com.android.tools.r8.CompilationFailedException;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestCompileResult;
-import com.android.tools.r8.ToolHelper.DexVm;
-import com.android.tools.r8.ToolHelper.ProcessResult;
-import java.io.IOException;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.ToolHelper.DexVm.Version;
+import com.android.tools.r8.graph.DexCode;
+import com.android.tools.r8.graph.DexCode.TryHandler;
+import com.android.tools.r8.utils.AndroidApiLevel;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
/**
* Regression test for art issue with multi catch-handlers.
@@ -43,34 +53,60 @@
}
}
+@RunWith(Parameterized.class)
public class B120164595 extends TestBase {
- @Test
- public void testD8()
- throws IOException, CompilationFailedException {
- TestCompileResult d8Result = testForD8().addProgramClasses(TestClass.class).compile();
- checkArt(d8Result);
+
+ @Parameter(0)
+ public TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withDexRuntimes().withAllApiLevels().build();
}
@Test
- public void testR8()
- throws IOException, CompilationFailedException {
- TestCompileResult r8Result = testForR8(Backend.DEX)
- .addProgramClasses(TestClass.class)
- .addKeepClassAndMembersRules(TestClass.class)
- .compile();
- checkArt(r8Result);
+ public void testD8() throws Exception {
+ TestCompileResult<?, ?> d8Result =
+ testForD8(parameters.getBackend())
+ .addProgramClasses(TestClass.class)
+ .setMinApi(parameters)
+ .compile();
+ checkResult(d8Result);
}
- private void checkArt(TestCompileResult result) throws IOException {
- ProcessResult artResult =
- runOnArtRaw(
- result.app,
- TestClass.class.getCanonicalName(),
- builder -> {
- builder.appendArtOption("-Xusejit:true");
- },
- DexVm.fromVersion(DexVm.Version.last()));
- assertEquals(0, artResult.exitCode);
- assertFalse(artResult.stderr.contains("Expected NullPointerException"));
+ @Test
+ public void testR8() throws Exception {
+ TestCompileResult<?, ?> r8Result =
+ testForR8(parameters.getBackend())
+ .addProgramClasses(TestClass.class)
+ .addKeepClassAndMembersRules(TestClass.class)
+ .setMinApi(parameters)
+ .compile();
+ checkResult(r8Result);
+ }
+
+ private void checkResult(TestCompileResult<?, ?> result) throws Exception {
+ result.inspect(
+ inspector -> {
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+ MethodSubject methodSubject = classSubject.uniqueMethodWithOriginalName("toBeOptimized");
+ assertThat(methodSubject, isPresent());
+ DexCode code = methodSubject.getMethod().getCode().asDexCode();
+ assertEquals(1, code.getHandlers().length);
+ TryHandler handler = code.getHandlers()[0];
+ assertEquals(2, handler.pairs.length);
+ if (parameters.getApiLevel().isLessThan(AndroidApiLevel.Q)) {
+ assertNotEquals(handler.pairs[0].addr, handler.pairs[1].addr);
+ } else {
+ assertEquals(handler.pairs[0].addr, handler.pairs[1].addr);
+ }
+ });
+ result
+ .applyIf(
+ parameters.getDexRuntimeVersion().isNewerThanOrEqual(Version.V7_0_0),
+ r -> r.addVmArguments("-Xusejit:true"))
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithEmptyOutput();
}
}
