Remove vertical class merger policy to ensure valid force inlining
Fixes: b/321909687
Change-Id: Id41516a09dcde40ec22575e222ee0f293969ed4b
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/MethodOptimizationFeedback.java b/src/main/java/com/android/tools/r8/ir/conversion/MethodOptimizationFeedback.java
index 8f4c441..c2fb0e6 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/MethodOptimizationFeedback.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/MethodOptimizationFeedback.java
@@ -23,8 +23,6 @@
public interface MethodOptimizationFeedback {
- void markForceInline(DexEncodedMethod method);
-
void markInlinedIntoSingleCallSite(DexEncodedMethod method);
void markMethodCannotBeKept(DexEncodedMethod method);
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackDelayed.java b/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackDelayed.java
index f4de742..8add480 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackDelayed.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackDelayed.java
@@ -162,11 +162,6 @@
// METHOD OPTIMIZATION INFO:
@Override
- public void markForceInline(DexEncodedMethod method) {
- getMethodOptimizationInfoForUpdating(method).markForceInline();
- }
-
- @Override
public synchronized void markInlinedIntoSingleCallSite(DexEncodedMethod method) {
getMethodOptimizationInfoForUpdating(method).markInlinedIntoSingleCallSite();
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackIgnore.java b/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackIgnore.java
index 05a1cdc..bb8f841 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackIgnore.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackIgnore.java
@@ -55,9 +55,6 @@
// METHOD OPTIMIZATION INFO:
@Override
- public void markForceInline(DexEncodedMethod method) {}
-
- @Override
public void markInlinedIntoSingleCallSite(DexEncodedMethod method) {}
@Override
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackSimple.java b/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackSimple.java
index 339d9eb6..34bb23b 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackSimple.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/info/OptimizationFeedbackSimple.java
@@ -83,11 +83,6 @@
}
@Override
- public void markForceInline(DexEncodedMethod method) {
- // Ignored.
- }
-
- @Override
public void markInlinedIntoSingleCallSite(DexEncodedMethod method) {
method.getMutableOptimizationInfo().markInlinedIntoSingleCallSite();
}
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerPolicyExecutor.java b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerPolicyExecutor.java
index 264441e..c82450b 100644
--- a/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerPolicyExecutor.java
+++ b/src/main/java/com/android/tools/r8/verticalclassmerging/VerticalClassMergerPolicyExecutor.java
@@ -29,7 +29,6 @@
import com.android.tools.r8.verticalclassmerging.policies.NoNestedMergingPolicy;
import com.android.tools.r8.verticalclassmerging.policies.NoNonSerializableClassIntoSerializableClassPolicy;
import com.android.tools.r8.verticalclassmerging.policies.NoServiceInterfacesPolicy;
-import com.android.tools.r8.verticalclassmerging.policies.SafeConstructorInliningPolicy;
import com.android.tools.r8.verticalclassmerging.policies.SameApiReferenceLevelPolicy;
import com.android.tools.r8.verticalclassmerging.policies.SameFeatureSplitPolicy;
import com.android.tools.r8.verticalclassmerging.policies.SameMainDexGroupPolicy;
@@ -76,7 +75,6 @@
new NoServiceInterfacesPolicy(appView),
new NoAnnotationClassesPolicy(),
new NoNonSerializableClassIntoSerializableClassPolicy(appView),
- new SafeConstructorInliningPolicy(appView),
new NoEnclosingMethodAttributesPolicy(),
new NoInnerClassAttributesPolicy(),
new SameNestPolicy(),
diff --git a/src/main/java/com/android/tools/r8/verticalclassmerging/policies/SafeConstructorInliningPolicy.java b/src/main/java/com/android/tools/r8/verticalclassmerging/policies/SafeConstructorInliningPolicy.java
deleted file mode 100644
index 15bcf11..0000000
--- a/src/main/java/com/android/tools/r8/verticalclassmerging/policies/SafeConstructorInliningPolicy.java
+++ /dev/null
@@ -1,72 +0,0 @@
-// Copyright (c) 2023, 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.verticalclassmerging.policies;
-
-import com.android.tools.r8.graph.AppView;
-import com.android.tools.r8.graph.CfCode;
-import com.android.tools.r8.graph.Code;
-import com.android.tools.r8.graph.DexProgramClass;
-import com.android.tools.r8.graph.ProgramMethod;
-import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
-import com.android.tools.r8.shaking.AppInfoWithLiveness;
-import com.android.tools.r8.shaking.MainDexInfo;
-import com.android.tools.r8.utils.TraversalContinuation;
-import com.android.tools.r8.verticalclassmerging.VerticalMergeGroup;
-import com.google.common.collect.Iterables;
-
-public class SafeConstructorInliningPolicy extends VerticalClassMergerPolicy {
-
- private final AppView<AppInfoWithLiveness> appView;
- private final MainDexInfo mainDexInfo;
-
- public SafeConstructorInliningPolicy(AppView<AppInfoWithLiveness> appView) {
- this.appView = appView;
- this.mainDexInfo = appView.appInfo().getMainDexInfo();
- }
-
- @Override
- public boolean canMerge(VerticalMergeGroup group) {
- DexProgramClass sourceClass = group.getSource();
- DexProgramClass targetClass = group.getTarget();
- // If there is a constructor in the target, make sure that all source constructors can be
- // inlined.
- if (Iterables.isEmpty(targetClass.programInstanceInitializers())) {
- return true;
- }
- TraversalContinuation<?, ?> result =
- sourceClass.traverseProgramInstanceInitializers(
- method -> TraversalContinuation.breakIf(disallowInlining(method, targetClass)));
- return result.shouldContinue();
- }
-
- private boolean disallowInlining(ProgramMethod method, DexProgramClass context) {
- if (!appView.options().inlinerOptions().enableInlining) {
- return true;
- }
- Code code = method.getDefinition().getCode();
- if (code.isCfCode()) {
- CfCode cfCode = code.asCfCode();
- ConstraintWithTarget constraint =
- cfCode.computeInliningConstraint(appView, appView.graphLens(), method);
- if (constraint.isNever()) {
- return true;
- }
- // Constructors can have references beyond the root main dex classes. This can increase the
- // size of the main dex dependent classes and we should bail out.
- if (mainDexInfo.disallowInliningIntoContext(appView, context, method)) {
- return true;
- }
- return false;
- }
- if (code.isDefaultInstanceInitializerCode()) {
- return false;
- }
- return true;
- }
-
- @Override
- public String getName() {
- return "SafeConstructorInliningPolicy";
- }
-}
diff --git a/src/test/java/com/android/tools/r8/classmerging/horizontal/SuperConstructorCallsVirtualMethodTest.java b/src/test/java/com/android/tools/r8/classmerging/horizontal/SuperConstructorCallsVirtualMethodTest.java
index f3335fa..fe4a7d8 100644
--- a/src/test/java/com/android/tools/r8/classmerging/horizontal/SuperConstructorCallsVirtualMethodTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/horizontal/SuperConstructorCallsVirtualMethodTest.java
@@ -4,9 +4,9 @@
package com.android.tools.r8.classmerging.horizontal;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.core.IsNot.not;
import com.android.tools.r8.NeverClassInline;
import com.android.tools.r8.NeverInline;
@@ -23,6 +23,9 @@
testForR8(parameters.getBackend())
.addInnerClasses(getClass())
.addKeepMainRule(Main.class)
+ .addHorizontallyMergedClassesInspector(
+ inspector ->
+ inspector.assertIsCompleteMergeGroup(A.class, B.class).assertNoOtherClassesMerged())
.enableInliningAnnotations()
.enableNeverClassInliningAnnotations()
.setMinApi(parameters)
@@ -30,9 +33,9 @@
.assertSuccessWithOutputLines("5", "foo hello", "B", "bar world", "5", "B")
.inspect(
codeInspector -> {
- assertThat(codeInspector.clazz(Parent.class), isPresent());
+ assertThat(codeInspector.clazz(Parent.class), isAbsent());
assertThat(codeInspector.clazz(A.class), isPresent());
- assertThat(codeInspector.clazz(B.class), not(isPresent()));
+ assertThat(codeInspector.clazz(B.class), isAbsent());
});
}
diff --git a/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerInitTest.java b/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerInitTest.java
index 466e9fd..f1b4b6a 100644
--- a/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerInitTest.java
+++ b/src/test/java/com/android/tools/r8/classmerging/vertical/VerticalClassMergerInitTest.java
@@ -4,6 +4,7 @@
package com.android.tools.r8.classmerging.vertical;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
import static org.hamcrest.MatcherAssert.assertThat;
@@ -13,13 +14,12 @@
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.utils.AndroidApiLevel;
-import com.android.tools.r8.utils.InternalOptions.InlinerOptions;
-import com.android.tools.r8.utils.codeinspector.VerticallyMergedClassesInspector;
import java.io.IOException;
import java.util.concurrent.ExecutionException;
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;
@RunWith(Parameterized.class)
@@ -28,38 +28,29 @@
// force-inlining, so the renamed constructor broke the init chain.
public class VerticalClassMergerInitTest extends TestBase {
- private final TestParameters parameters;
+ @Parameter(0)
+ public TestParameters parameters;
@Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withDexRuntimes().build();
- }
-
- public VerticalClassMergerInitTest(TestParameters parameters) {
- this.parameters = parameters;
+ return getTestParameters().withDexRuntimes().withApiLevel(AndroidApiLevel.K_WATCH).build();
}
@Test
public void testMergingClassWithConstructorNotInMainDex()
throws IOException, CompilationFailedException, ExecutionException {
- testForR8(parameters.getBackend())
+ testForR8Compat(parameters.getBackend())
.addInnerClasses(VerticalClassMergerInitTest.class)
.addKeepMainRule(Main.class)
.addMainDexRules("-keep class " + Main.class.getTypeName())
- .enableNeverClassInliningAnnotations()
- .setMinApi(AndroidApiLevel.K_WATCH)
- .addOptionsModification(
- options -> {
- options.forceProguardCompatibility = true;
- })
- // The initializer is small in this example so only allow FORCE inlining.
- .addOptionsModification(InlinerOptions::setOnlyForceInlining)
.addVerticallyMergedClassesInspector(
- VerticallyMergedClassesInspector::assertNoClassesMerged)
+ inspector -> inspector.assertMergedIntoSubtype(Base.class))
+ .enableNeverClassInliningAnnotations()
+ .setMinApi(parameters)
.compile()
.inspect(
inspector -> {
- assertThat(inspector.clazz(Base.class), isPresent());
+ assertThat(inspector.clazz(Base.class), isAbsent());
assertThat(inspector.clazz(Child.class), isPresent());
})
.run(parameters.getRuntime(), Main.class)
diff --git a/src/test/java/com/android/tools/r8/profile/art/completeness/RecordProfileRewritingTest.java b/src/test/java/com/android/tools/r8/profile/art/completeness/RecordProfileRewritingTest.java
index 43808b8..cd72b73 100644
--- a/src/test/java/com/android/tools/r8/profile/art/completeness/RecordProfileRewritingTest.java
+++ b/src/test/java/com/android/tools/r8/profile/art/completeness/RecordProfileRewritingTest.java
@@ -166,6 +166,7 @@
inspector,
SyntheticItemsTestUtils.syntheticRecordTagClass(),
false,
+ false,
parameters.canUseNestBasedAccessesWhenDesugaring(),
!isRecordsDesugaredForD8(parameters));
}
@@ -176,6 +177,7 @@
inspector,
RECORD_REFERENCE,
parameters.canHaveNonReboundConstructorInvoke(),
+ true,
parameters.canUseNestBasedAccesses(),
!isRecordsDesugaredForR8(parameters));
}
@@ -185,6 +187,7 @@
CodeInspector inspector,
ClassReference recordClassReference,
boolean canHaveNonReboundConstructorInvoke,
+ boolean canMergeRecordTag,
boolean canUseNestBasedAccesses,
boolean canUseRecords) {
ClassSubject mainClassSubject = inspector.clazz(MAIN_REFERENCE);
@@ -194,8 +197,7 @@
assertThat(mainMethodSubject, isPresent());
ClassSubject recordTagClassSubject = inspector.clazz(recordClassReference);
- assertThat(
- recordTagClassSubject, isAbsentIf(canHaveNonReboundConstructorInvoke || canUseRecords));
+ assertThat(recordTagClassSubject, isAbsentIf(canMergeRecordTag || canUseRecords));
if (recordTagClassSubject.isPresent()) {
assertEquals(
canHaveNonReboundConstructorInvoke ? 0 : 1, recordTagClassSubject.allMethods().size());
@@ -207,16 +209,25 @@
ClassSubject personRecordClassSubject = inspector.clazz(PERSON_REFERENCE);
assertThat(personRecordClassSubject, isPresent());
assertEquals(
- canHaveNonReboundConstructorInvoke
- ? inspector.getTypeSubject(Object.class.getTypeName())
- : canUseRecords
- ? inspector.getTypeSubject(RECORD_REFERENCE.getTypeName())
+ canUseRecords
+ ? inspector.getTypeSubject(RECORD_REFERENCE.getTypeName())
+ : canMergeRecordTag
+ ? inspector.getTypeSubject(Object.class.getTypeName())
: recordTagClassSubject.asTypeSubject(),
personRecordClassSubject.getSuperType());
- assertEquals(canUseRecords ? 6 : 10, personRecordClassSubject.allMethods().size());
+ assertEquals(
+ canUseRecords ? 6 : canHaveNonReboundConstructorInvoke || !canMergeRecordTag ? 10 : 11,
+ personRecordClassSubject.allMethods().size());
+
+ MethodSubject personDefaultInstanceInitializerSubject = personRecordClassSubject.init();
+ assertThat(
+ personDefaultInstanceInitializerSubject,
+ isPresentIf(!canHaveNonReboundConstructorInvoke && canMergeRecordTag && !canUseRecords));
MethodSubject personInstanceInitializerSubject =
- personRecordClassSubject.uniqueInstanceInitializer();
+ canMergeRecordTag
+ ? personRecordClassSubject.init(String.class.getTypeName())
+ : personRecordClassSubject.init(String.class.getTypeName(), "int");
assertThat(personInstanceInitializerSubject, isPresent());
// Name getters.
@@ -306,6 +317,9 @@
i.assertContainsMethodRules(
nameNestAccessorMethodSubject, ageNestAccessorMethodSubject))
.applyIf(
+ !canHaveNonReboundConstructorInvoke && canMergeRecordTag && !canUseRecords,
+ i -> i.assertContainsMethodRule(personDefaultInstanceInitializerSubject))
+ .applyIf(
!canUseRecords,
i ->
i.assertContainsClassRules(hashCodeHelperClassSubject, toStringHelperClassSubject)
@@ -315,7 +329,7 @@
hashCodeHelperMethodSubject,
toStringHelperMethodSubject)
.applyIf(
- !canHaveNonReboundConstructorInvoke,
+ !canMergeRecordTag,
j ->
j.assertContainsClassRules(recordTagClassSubject)
.assertContainsMethodRule(recordTagInstanceInitializerSubject)))
diff --git a/src/test/java/com/android/tools/r8/shaking/PrintUsageTest.java b/src/test/java/com/android/tools/r8/shaking/PrintUsageTest.java
index fdb513c..9f63d3a 100644
--- a/src/test/java/com/android/tools/r8/shaking/PrintUsageTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/PrintUsageTest.java
@@ -175,11 +175,11 @@
private static void inspectShaking9(PrintUsageInspector inspector) {
Optional<ClassSubject> superClass = inspector.clazz("shaking9.Superclass");
- assertFalse(superClass.isPresent());
+ assertTrue(superClass.isPresent());
Optional<ClassSubject> subClass = inspector.clazz("shaking9.Subclass");
assertTrue(subClass.isPresent());
assertTrue(subClass.get().method("void", "aMethod", ImmutableList.of()));
- assertFalse(subClass.get().method("void", "<init>", ImmutableList.of()));
+ assertTrue(subClass.get().method("void", "<init>", ImmutableList.of()));
}
private static void inspectShaking12(PrintUsageInspector inspector) {
diff --git a/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking14Test.java b/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking14Test.java
index 4733d3c..610a9c6 100644
--- a/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking14Test.java
+++ b/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking14Test.java
@@ -3,13 +3,15 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.shaking.examples;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.shaking.TreeShakingTest;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.google.common.collect.ImmutableList;
import java.util.List;
-import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -48,12 +50,9 @@
private static void shaking14EnsureRightStaticMethodsLive(CodeInspector inspector) {
ClassSubject superclass = inspector.clazz("shaking14.Superclass");
- Assert.assertFalse(superclass.method("int", "aMethod", ImmutableList.of("int")).isPresent());
- Assert.assertFalse(
- superclass.method("double", "anotherMethod", ImmutableList.of("double")).isPresent());
+ assertThat(superclass, isAbsent());
+
ClassSubject subclass = inspector.clazz("shaking14.Subclass");
- Assert.assertTrue(subclass.method("int", "aMethod", ImmutableList.of("int")).isPresent());
- Assert.assertTrue(
- subclass.method("double", "anotherMethod", ImmutableList.of("double")).isPresent());
+ assertThat(subclass, isAbsent());
}
}
diff --git a/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking9Test.java b/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking9Test.java
index 3b80981..99cbc57 100644
--- a/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking9Test.java
+++ b/src/test/java/com/android/tools/r8/shaking/examples/TreeShaking9Test.java
@@ -4,9 +4,7 @@
package com.android.tools.r8.shaking.examples;
import static com.android.tools.r8.utils.codeinspector.Matchers.isAbsent;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isAbstract;
import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
-import static com.android.tools.r8.utils.codeinspector.Matchers.isPresentIf;
import static org.hamcrest.MatcherAssert.assertThat;
import com.android.tools.r8.TestParameters;
@@ -45,10 +43,7 @@
@Test
public void testKeeprules() throws Exception {
runTest(
- this::shaking9OnlySuperMethodsKept,
- null,
- null,
- ImmutableList.of("src/test/examples/shaking9/keep-rules.txt"));
+ this::inspect, null, null, ImmutableList.of("src/test/examples/shaking9/keep-rules.txt"));
}
@Test
@@ -57,19 +52,12 @@
null, null, null, ImmutableList.of("src/test/examples/shaking9/keep-rules-printusage.txt"));
}
- private void shaking9OnlySuperMethodsKept(CodeInspector inspector) {
+ private void inspect(CodeInspector inspector) {
ClassSubject superclass = inspector.clazz("shaking9.Superclass");
- if (parameters.canHaveNonReboundConstructorInvoke()) {
- assertThat(superclass, isAbsent());
- } else {
- assertThat(superclass, isAbstract());
- assertThat(superclass.method("void", "aMethod", ImmutableList.of()), isPresent());
- }
+ assertThat(superclass, isAbsent());
ClassSubject subclass = inspector.clazz("shaking9.Subclass");
assertThat(subclass, isPresent());
- assertThat(
- subclass.method("void", "aMethod", ImmutableList.of()),
- isPresentIf(parameters.canHaveNonReboundConstructorInvoke() && !getMinify().isMinify()));
+ assertThat(subclass.method("void", "aMethod", ImmutableList.of()), isAbsent());
}
}
