Merge "Add DupDupDupPeephole for replacing 3 x dup with dup + dup2"
diff --git a/src/main/java/com/android/tools/r8/R8Command.java b/src/main/java/com/android/tools/r8/R8Command.java
index 3ac587e..b5eafcd 100644
--- a/src/main/java/com/android/tools/r8/R8Command.java
+++ b/src/main/java/com/android/tools/r8/R8Command.java
@@ -723,9 +723,6 @@
internal.enableInheritanceClassInDexDistributor = isOptimizeMultidexForLinearAlloc();
- // TODO(b/117916645): Re-enable horizontal class merging by deleting this line.
- internal.enableHorizontalClassMerging = false;
-
return internal;
}
}
diff --git a/src/main/java/com/android/tools/r8/graph/DexClass.java b/src/main/java/com/android/tools/r8/graph/DexClass.java
index 9ceaf09..10007ca 100644
--- a/src/main/java/com/android/tools/r8/graph/DexClass.java
+++ b/src/main/java/com/android/tools/r8/graph/DexClass.java
@@ -431,6 +431,24 @@
return type.isExternalizable(appInfo);
}
+ public boolean classInitializationMayHaveSideEffects(AppInfo appInfo) {
+ if (hasNonTrivialClassInitializer()) {
+ return true;
+ }
+ if (defaultValuesForStaticFieldsMayTriggerAllocation()) {
+ return true;
+ }
+ for (DexType iface : interfaces.values) {
+ if (iface.classInitializationMayHaveSideEffects(appInfo)) {
+ return true;
+ }
+ }
+ if (superType != null && superType.classInitializationMayHaveSideEffects(appInfo)) {
+ return true;
+ }
+ return false;
+ }
+
public boolean defaultValuesForStaticFieldsMayTriggerAllocation() {
return Arrays.stream(staticFields())
.anyMatch(field -> !field.getStaticValue().mayTriggerAllocation());
diff --git a/src/main/java/com/android/tools/r8/graph/DexType.java b/src/main/java/com/android/tools/r8/graph/DexType.java
index 5e08b64..f49115a 100644
--- a/src/main/java/com/android/tools/r8/graph/DexType.java
+++ b/src/main/java/com/android/tools/r8/graph/DexType.java
@@ -108,6 +108,11 @@
return implementedInterfaces(appInfo).contains(appInfo.dexItemFactory.serializableType);
}
+ public boolean classInitializationMayHaveSideEffects(AppInfo appInfo) {
+ DexClass clazz = appInfo.definitionFor(this);
+ return clazz == null || clazz.classInitializationMayHaveSideEffects(appInfo);
+ }
+
public boolean isUnknown() {
return hierarchyLevel == UNKNOWN_LEVEL;
}
@@ -187,6 +192,10 @@
return self == other;
}
+ public Set<DexType> allImmediateSubtypes() {
+ return directSubtypes;
+ }
+
/**
* Apply the given function to all classes that directly extend this class.
* <p>
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/InterfaceMethodRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/InterfaceMethodRewriter.java
index 82e3bff..0f6dbd7 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/InterfaceMethodRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/InterfaceMethodRewriter.java
@@ -491,16 +491,18 @@
if (!reportedMissing.add(missing)) {
return;
}
+ DexMethod originalReferencedFrom =
+ converter.graphLense().getOriginalMethodSignature(referencedFrom);
StringBuilder builder = new StringBuilder();
builder
.append("Type `")
.append(missing.toSourceString())
.append("` was not found, ")
.append("it is required for default or static interface methods desugaring of `")
- .append(referencedFrom.toSourceString())
+ .append(originalReferencedFrom.toSourceString())
.append("`");
options.reporter.warning(
- new StringDiagnostic(builder.toString(), getMethodOrigin(referencedFrom)));
+ new StringDiagnostic(builder.toString(), getMethodOrigin(originalReferencedFrom)));
}
private Origin getMethodOrigin(DexMethod method) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
index 479c3e4..ed92f64 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/DefaultInliningOracle.java
@@ -128,31 +128,14 @@
if (target.getOptimizationInfo().triggersClassInitBeforeAnySideEffect()) {
return true;
}
- return classInitializationHasNoSideffects(targetHolder);
- }
-
- /**
- * Check for class initializer side effects when loading this class, as inlining might remove the
- * load operation.
- * <p>
- * See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.5.
- * <p>
- * For simplicity, we are conservative and consider all interfaces, not only the ones with default
- * methods.
- */
- private boolean classInitializationHasNoSideffects(DexType classToCheck) {
- DexClass clazz = inliner.appView.appInfo().definitionFor(classToCheck);
- if ((clazz == null)
- || clazz.hasNonTrivialClassInitializer()
- || clazz.defaultValuesForStaticFieldsMayTriggerAllocation()) {
- return false;
- }
- for (DexType iface : clazz.interfaces.values) {
- if (!classInitializationHasNoSideffects(iface)) {
- return false;
- }
- }
- return clazz.superType == null || classInitializationHasNoSideffects(clazz.superType);
+ // Check for class initializer side effects when loading this class, as inlining might remove
+ // the load operation.
+ //
+ // See https://docs.oracle.com/javase/specs/jvms/se9/html/jvms-5.html#jvms-5.5.
+ //
+ // For simplicity, we are conservative and consider all interfaces, not only the ones with
+ // default methods.
+ return !clazz.classInitializationMayHaveSideEffects(appView.appInfo());
}
private synchronized boolean isDoubleInliningTarget(DexEncodedMethod candidate) {
diff --git a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
index 906451e..1cb5d93 100644
--- a/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
+++ b/src/main/java/com/android/tools/r8/shaking/StaticClassMerger.java
@@ -90,9 +90,6 @@
if (appView.appInfo().neverMerge.contains(clazz.type)) {
return false;
}
- if (clazz.accessFlags.isInterface()) {
- return false;
- }
if (clazz.staticFields().length + clazz.directMethods().length + clazz.virtualMethods().length
== 0) {
return false;
@@ -121,6 +118,15 @@
|| appView.appInfo().noSideEffects.keySet().contains(method))) {
return false;
}
+ if (clazz.classInitializationMayHaveSideEffects(appView.appInfo())) {
+ // This could have a negative impact on inlining.
+ //
+ // See {@link com.android.tools.r8.ir.optimize.DefaultInliningOracle#canInlineStaticInvoke}
+ // for further details.
+ //
+ // Note that this will be true for all classes that inherit from or implement a library class.
+ return false;
+ }
return true;
}
@@ -266,7 +272,9 @@
DexMethod originalMethod =
methodMapping.inverse().getOrDefault(sourceMethod.method, sourceMethod.method);
- methodMapping.put(originalMethod, sourceMethodAfterMove.method);
+ methodMapping.forcePut(originalMethod, sourceMethodAfterMove.method);
+
+ existingMethods.add(equivalence.wrap(sourceMethodAfterMove.method));
}
return result;
@@ -297,7 +305,9 @@
DexField originalField =
fieldMapping.inverse().getOrDefault(sourceField.field, sourceField.field);
- fieldMapping.put(originalField, sourceFieldAfterMove.field);
+ fieldMapping.forcePut(originalField, sourceFieldAfterMove.field);
+
+ existingFields.add(equivalence.wrap(sourceFieldAfterMove.field));
}
return result;
diff --git a/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java b/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java
new file mode 100644
index 0000000..4a331a7
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/classmerging/InliningAfterStaticClassMergerTest.java
@@ -0,0 +1,105 @@
+// Copyright (c) 2018, 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.classmerging;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.FoundClassSubject;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+/**
+ * This test checks that the static class inliner does not merge classes in situations where a merge
+ * operation could lead to a regression in code size because of reduced inlining opportunities.
+ */
+@RunWith(Parameterized.class)
+public class InliningAfterStaticClassMergerTest extends TestBase {
+
+ // A class that implements a library class.
+ static class StaticMergeCandidateA implements Cloneable {
+
+ // Cannot be inlined into TestClass.main() because the static initialization of this class could
+ // have side-effects; in order for R8 to be conservative, library classes are treated as if
+ // their static initialization could have side-effects.
+ public static String m() {
+ return "StaticMergeCandidateA.m()";
+ }
+ }
+
+ static class StaticMergeCandidateB {
+
+ // Can be inlined into TestClass.main() because the static initialization of this class has no
+ // side-effects.
+ public static String m() {
+ return "StaticMergeCandidateB.m()";
+ }
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ System.out.println(StaticMergeCandidateA.m());
+ System.out.print(StaticMergeCandidateB.m());
+ }
+ }
+
+ private Backend backend;
+
+ public InliningAfterStaticClassMergerTest(Backend backend) {
+ this.backend = backend;
+ }
+
+ @Parameters(name = "Backend: {0}")
+ public static Backend[] data() {
+ return Backend.values();
+ }
+
+ @Test
+ public void testMethodsAreInlined() throws Exception {
+ String expected =
+ StringUtils.joinLines("StaticMergeCandidateA.m()", "StaticMergeCandidateB.m()");
+
+ testForJvm().addTestClasspath().run(TestClass.class).assertSuccessWithOutput(expected);
+
+ CodeInspector inspector =
+ testForR8(backend)
+ .addProgramClasses(
+ InliningAfterStaticClassMergerTest.TestClass.class,
+ InliningAfterStaticClassMergerTest.StaticMergeCandidateA.class,
+ InliningAfterStaticClassMergerTest.StaticMergeCandidateB.class)
+ .addKeepMainRule(TestClass.class)
+ .addOptionsModification(options -> options.enableMinification = false)
+ .run(TestClass.class)
+ .assertSuccessWithOutput(expected)
+ .inspector();
+
+ // Check that StaticMergeCandidateB has been removed.
+ List<FoundClassSubject> classes =
+ inspector.allClasses().stream()
+ .filter(clazz -> clazz.getOriginalName().contains("StaticMergeCandidate"))
+ .collect(Collectors.toList());
+ assertEquals(1, classes.size());
+ assertEquals(StaticMergeCandidateA.class.getTypeName(), classes.get(0).getOriginalName());
+
+ // Check that StaticMergeCandidateB.m() has not been moved into StaticMergeCandidateA, because
+ // that would disable inlining of it.
+ assertEquals(1, classes.get(0).allMethods().size());
+
+ // Check that the test class only has a main method.
+ ClassSubject classSubject = inspector.clazz(TestClass.class);
+ assertThat(classSubject, isPresent());
+ assertEquals(1, classSubject.allMethods().size());
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerTest.java b/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerTest.java
index fff28c9..2d2788c 100644
--- a/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/StaticClassMergerTest.java
@@ -78,12 +78,10 @@
inspector.allClasses().stream()
.filter(clazz -> clazz.getOriginalName().contains("StaticMergeCandidate"))
.collect(Collectors.toList());
- // TODO(b/117916645): Revert back to 1 here once the issue is fixed.
- assertEquals(2, classes.size());
+ assertEquals(1, classes.size());
// Check that the remaining static merge candidate has two methods.
FoundClassSubject remaining = classes.get(0);
- // TODO(b/117916645): Revert back to 2 here once the issue is fixed.
- assertEquals(1, remaining.allMethods().size());
+ assertEquals(2, remaining.allMethods().size());
}
}
