Enum unboxing: Not move methods to merged classes
Bug: 165227525
Bug: 160939354
Change-Id: I2189557bde66528026a0a635eac07c25041efce1
diff --git a/src/main/java/com/android/tools/r8/graph/AppView.java b/src/main/java/com/android/tools/r8/graph/AppView.java
index 75fe24e..14cec54 100644
--- a/src/main/java/com/android/tools/r8/graph/AppView.java
+++ b/src/main/java/com/android/tools/r8/graph/AppView.java
@@ -397,6 +397,15 @@
return collection;
}
+ public boolean hasBeenMerged(DexProgramClass clazz) {
+ // TODO(b/165227525): Add support for the horizontal class merger here.
+ if (horizontallyMergedLambdaClasses != null
+ && horizontallyMergedLambdaClasses.hasBeenMerged(clazz)) {
+ return true;
+ }
+ return verticallyMergedClasses != null && verticallyMergedClasses.hasBeenMerged(clazz);
+ }
+
// Get the result of horizontal lambda class merging. Returns null if horizontal lambda class
// merging has not been run.
public HorizontallyMergedLambdaClasses horizontallyMergedLambdaClasses() {
diff --git a/src/main/java/com/android/tools/r8/graph/classmerging/HorizontallyMergedLambdaClasses.java b/src/main/java/com/android/tools/r8/graph/classmerging/HorizontallyMergedLambdaClasses.java
index 1facc45..fc8e49b 100644
--- a/src/main/java/com/android/tools/r8/graph/classmerging/HorizontallyMergedLambdaClasses.java
+++ b/src/main/java/com/android/tools/r8/graph/classmerging/HorizontallyMergedLambdaClasses.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.graph.classmerging;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import java.util.Set;
@@ -27,4 +28,9 @@
}
return true;
}
+
+ @Override
+ public boolean hasBeenMerged(DexProgramClass clazz) {
+ return sources.contains(clazz.type);
+ }
}
diff --git a/src/main/java/com/android/tools/r8/graph/classmerging/MergedClasses.java b/src/main/java/com/android/tools/r8/graph/classmerging/MergedClasses.java
index 7b9bba3..66f9f6a 100644
--- a/src/main/java/com/android/tools/r8/graph/classmerging/MergedClasses.java
+++ b/src/main/java/com/android/tools/r8/graph/classmerging/MergedClasses.java
@@ -5,9 +5,12 @@
package com.android.tools.r8.graph.classmerging;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
public interface MergedClasses {
boolean verifyAllSourcesPruned(AppView<AppInfoWithLiveness> appView);
+
+ boolean hasBeenMerged(DexProgramClass clazz);
}
diff --git a/src/main/java/com/android/tools/r8/graph/classmerging/MergedClassesCollection.java b/src/main/java/com/android/tools/r8/graph/classmerging/MergedClassesCollection.java
index ad7c71d..1ced397 100644
--- a/src/main/java/com/android/tools/r8/graph/classmerging/MergedClassesCollection.java
+++ b/src/main/java/com/android/tools/r8/graph/classmerging/MergedClassesCollection.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.graph.classmerging;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import java.util.ArrayList;
import java.util.List;
@@ -24,4 +25,14 @@
}
return true;
}
+
+ @Override
+ public boolean hasBeenMerged(DexProgramClass clazz) {
+ for (MergedClasses mergedClasses : collection) {
+ if (mergedClasses.hasBeenMerged(clazz)) {
+ return true;
+ }
+ }
+ return false;
+ }
}
diff --git a/src/main/java/com/android/tools/r8/graph/classmerging/VerticallyMergedClasses.java b/src/main/java/com/android/tools/r8/graph/classmerging/VerticallyMergedClasses.java
index 944d680..d4c730a 100644
--- a/src/main/java/com/android/tools/r8/graph/classmerging/VerticallyMergedClasses.java
+++ b/src/main/java/com/android/tools/r8/graph/classmerging/VerticallyMergedClasses.java
@@ -5,6 +5,7 @@
package com.android.tools.r8.graph.classmerging;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.google.common.collect.ImmutableList;
@@ -53,4 +54,9 @@
}
return true;
}
+
+ @Override
+ public boolean hasBeenMerged(DexProgramClass clazz) {
+ return hasBeenMergedIntoSubtype(clazz.type);
+ }
}
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 6cbf350..510757b 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
@@ -106,6 +106,7 @@
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ListMultimap;
+import com.google.common.collect.Sets;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -743,7 +744,7 @@
postMethodProcessorBuilder,
executorService,
feedback,
- classStaticizer == null ? Collections.emptySet() : classStaticizer.getCandidates());
+ classStaticizer == null ? Sets.newIdentityHashSet() : classStaticizer.getCandidates());
}
if (!options.debug) {
new TrivialFieldAccessReprocessor(appView.withLiveness(), postMethodProcessorBuilder)
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java
index 12a23f1..ddfb486 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/enums/EnumUnboxer.java
@@ -436,13 +436,13 @@
// We are trying to select a host for the enum unboxing methods, but multiple candidates are
// available. We need to pick one of the two classes and the result has to be deterministic.
// We follow the heuristics, in order:
- // 1. don't pick a class from hostToAvoidIfPossible if possible
+ // 1. don't pick a class from hostToAvoidIfPossible, or merged, if possible
// 2. pick the class with the least number of methods
// 3. pick the first class name in alphabetical order for determinism.
private DexProgramClass selectHost(
DexProgramClass class1, DexProgramClass class2, Set<DexType> hostsToAvoidIfPossible) {
- boolean avoid1 = hostsToAvoidIfPossible.contains(class1.type);
- boolean avoid2 = hostsToAvoidIfPossible.contains(class2.type);
+ boolean avoid1 = appView.hasBeenMerged(class1) || hostsToAvoidIfPossible.contains(class1.type);
+ boolean avoid2 = appView.hasBeenMerged(class2) || hostsToAvoidIfPossible.contains(class2.type);
if (avoid1 && !avoid2) {
return class2;
}
diff --git a/src/test/java/com/android/tools/r8/enumunboxing/EnumUnboxingVerticalClassMergeTest.java b/src/test/java/com/android/tools/r8/enumunboxing/EnumUnboxingVerticalClassMergeTest.java
new file mode 100644
index 0000000..d8396cb
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/enumunboxing/EnumUnboxingVerticalClassMergeTest.java
@@ -0,0 +1,113 @@
+// Copyright (c) 2020, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+package com.android.tools.r8.enumunboxing;
+
+import static org.junit.Assert.assertFalse;
+
+import com.android.tools.r8.NeverClassInline;
+import com.android.tools.r8.NeverInline;
+import com.android.tools.r8.R8TestRunResult;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class EnumUnboxingVerticalClassMergeTest extends EnumUnboxingTestBase {
+
+ private final TestParameters parameters;
+ private final boolean enumValueOptimization;
+ private final EnumKeepRules enumKeepRules;
+
+ @Parameterized.Parameters(name = "{0} valueOpt: {1} keep: {2}")
+ public static List<Object[]> data() {
+ return enumUnboxingTestParameters();
+ }
+
+ public EnumUnboxingVerticalClassMergeTest(
+ TestParameters parameters, boolean enumValueOptimization, EnumKeepRules enumKeepRules) {
+ this.parameters = parameters;
+ this.enumValueOptimization = enumValueOptimization;
+ this.enumKeepRules = enumKeepRules;
+ }
+
+ @Test
+ public void testEnumUnboxing() throws Exception {
+ R8TestRunResult run =
+ testForR8(parameters.getBackend())
+ .addInnerClasses(EnumUnboxingVerticalClassMergeTest.class)
+ .addKeepMainRule(Main.class)
+ .addKeepRules(enumKeepRules.getKeepRules())
+ .enableNeverClassInliningAnnotations()
+ .enableInliningAnnotations()
+ .noMinification() // For assertions.
+ .addOptionsModification(opt -> enableEnumOptions(opt, enumValueOptimization))
+ .allowDiagnosticInfoMessages()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::assertVerticalClassMerged)
+ .inspectDiagnosticMessages(
+ m ->
+ assertEnumIsUnboxed(
+ UnboxableEnum.class,
+ EnumUnboxingVerticalClassMergeTest.class.getSimpleName(),
+ m))
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccess();
+ assertLines2By2Correct(run.getStdOut());
+ }
+
+ private void assertVerticalClassMerged(CodeInspector codeInspector) {
+ assertFalse(codeInspector.clazz(Merge1.class).isPresent());
+ }
+
+ static class Main {
+ public static void main(String[] args) {
+ Merge1 merge = new Merge2();
+ merge.print();
+ System.out.println("print");
+ merge.printDefault();
+ System.out.println("print default");
+ UnboxableEnum.A.enumPrint();
+ System.out.println("0");
+ UnboxableEnum.B.enumPrint();
+ System.out.println("1");
+ }
+
+ @NeverInline
+ static void test(UnboxableEnum e) {
+ System.out.println(e.ordinal());
+ }
+ }
+
+ static class Merge2 extends Merge1 {
+ @Override
+ public void print() {
+ System.out.println("print");
+ }
+ }
+
+ abstract static class Merge1 {
+ abstract void print();
+
+ void printDefault() {
+ System.out.println("print default");
+ }
+ }
+
+ @NeverClassInline
+ enum UnboxableEnum {
+ A,
+ B,
+ C;
+
+ @NeverInline
+ void enumPrint() {
+ Main.test(this);
+ }
+ }
+}
