Reland "Keep single caller inlinees after inlining if reprocessing"
This reverts commit ac7ca7ea02277793be74e5384512245a535775e2.
Bug: b/285021603
Change-Id: I5fe1d76407ae6331b142591fe1a9bc086a0f9d80
diff --git a/src/main/java/com/android/tools/r8/ir/conversion/callgraph/CallSiteInformation.java b/src/main/java/com/android/tools/r8/ir/conversion/callgraph/CallSiteInformation.java
index c2e13af..e412ed7 100644
--- a/src/main/java/com/android/tools/r8/ir/conversion/callgraph/CallSiteInformation.java
+++ b/src/main/java/com/android/tools/r8/ir/conversion/callgraph/CallSiteInformation.java
@@ -13,6 +13,8 @@
import com.android.tools.r8.utils.classhierarchy.MethodOverridesCollector;
import com.android.tools.r8.utils.collections.ProgramMethodSet;
import com.google.common.collect.Sets;
+import java.util.IdentityHashMap;
+import java.util.Map;
import java.util.Set;
public abstract class CallSiteInformation {
@@ -23,6 +25,14 @@
* <p>For pinned methods (methods kept through Proguard keep rules) this will always answer <code>
* false</code>.
*/
+ public abstract boolean hasSingleCallSite(ProgramMethod method, ProgramMethod context);
+
+ /**
+ * Checks if the given method only has a single call without considering context.
+ *
+ * <p>For pinned methods (methods kept through Proguard keep rules) and methods that override a
+ * library method this always returns false.
+ */
public abstract boolean hasSingleCallSite(ProgramMethod method);
public abstract boolean isMultiCallerInlineCandidate(ProgramMethod method);
@@ -38,6 +48,11 @@
private static final EmptyCallSiteInformation EMPTY_INFO = new EmptyCallSiteInformation();
@Override
+ public boolean hasSingleCallSite(ProgramMethod method, ProgramMethod context) {
+ return false;
+ }
+
+ @Override
public boolean hasSingleCallSite(ProgramMethod method) {
return false;
}
@@ -55,7 +70,9 @@
static class CallGraphBasedCallSiteInformation extends CallSiteInformation {
- private final Set<DexMethod> singleCallerMethods = Sets.newIdentityHashSet();
+ // Single callers track their calling context to ensure that the predicate is stable after
+ // inlining of the caller.
+ private final Map<DexMethod, DexMethod> singleCallerMethods = new IdentityHashMap<>();
private final Set<DexMethod> multiCallerInlineCandidates = Sets.newIdentityHashSet();
CallGraphBasedCallSiteInformation(AppView<AppInfoWithLiveness> appView, CallGraph graph) {
@@ -94,7 +111,16 @@
int numberOfCallSites = node.getNumberOfCallSites();
if (numberOfCallSites == 1) {
- singleCallerMethods.add(reference);
+ Set<Node> callersWithDeterministicOrder = node.getCallersWithDeterministicOrder();
+ DexMethod caller = reference;
+ // We can have recursive methods where the recursive call is the only call site. We do
+ // not track callers for these.
+ if (!callersWithDeterministicOrder.isEmpty()) {
+ assert callersWithDeterministicOrder.size() == 1;
+ caller = callersWithDeterministicOrder.iterator().next().getMethod().getReference();
+ }
+ DexMethod existing = singleCallerMethods.put(reference, caller);
+ assert existing == null;
} else if (numberOfCallSites > 1) {
multiCallerInlineCandidates.add(reference);
}
@@ -102,14 +128,25 @@
}
/**
- * Checks if the given method only has a single call site.
+ * Checks if the given method only has a single call site with the given context.
+ *
+ * <p>For pinned methods (methods kept through Proguard keep rules) and methods that override a
+ * library method this always returns false.
+ */
+ @Override
+ public boolean hasSingleCallSite(ProgramMethod method, ProgramMethod context) {
+ return singleCallerMethods.get(method.getReference()) == context.getReference();
+ }
+
+ /**
+ * Checks if the given method only has a single call without considering context.
*
* <p>For pinned methods (methods kept through Proguard keep rules) and methods that override a
* library method this always returns false.
*/
@Override
public boolean hasSingleCallSite(ProgramMethod method) {
- return singleCallerMethods.contains(method.getReference());
+ return singleCallerMethods.containsKey(method.getReference());
}
/**
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/MultiCallerInliner.java b/src/main/java/com/android/tools/r8/ir/optimize/MultiCallerInliner.java
index 54c8f61..4b5f0c9 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/MultiCallerInliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/MultiCallerInliner.java
@@ -147,12 +147,11 @@
// We track up to n call sites, where n is the size of multiCallerInliningInstructionLimits.
if (callers.size() > multiCallerInliningInstructionLimits.length) {
stopTrackingCallSitesForMethodIfDefinitelyIneligibleForMultiCallerInlining(
- method, singleTarget, methodProcessor, callers);
+ singleTarget, methodProcessor, callers);
}
}
private void stopTrackingCallSitesForMethodIfDefinitelyIneligibleForMultiCallerInlining(
- ProgramMethod method,
ProgramMethod singleTarget,
MethodProcessor methodProcessor,
ProgramMethodMultiset callers) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/inliner/DefaultInliningReasonStrategy.java b/src/main/java/com/android/tools/r8/ir/optimize/inliner/DefaultInliningReasonStrategy.java
index 537150e..02d13dd 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/inliner/DefaultInliningReasonStrategy.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/inliner/DefaultInliningReasonStrategy.java
@@ -53,7 +53,7 @@
// program.
return Reason.SIMPLE;
}
- if (isSingleCallerInliningTarget(target)) {
+ if (isSingleCallerInliningTarget(target, context)) {
return Reason.SINGLE_CALLER;
}
if (isMultiCallerInlineCandidate(invoke, target, oracle, methodProcessor)) {
@@ -64,8 +64,8 @@
return Reason.SIMPLE;
}
- private boolean isSingleCallerInliningTarget(ProgramMethod method) {
- if (!callSiteInformation.hasSingleCallSite(method)) {
+ private boolean isSingleCallerInliningTarget(ProgramMethod method, ProgramMethod context) {
+ if (!callSiteInformation.hasSingleCallSite(method, context)) {
return false;
}
if (appView.appInfo().isNeverInlineDueToSingleCallerMethod(method)) {
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/SingleTargetAfterInliningTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/SingleTargetAfterInliningTest.java
index f5b5a47..706f8f0 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/SingleTargetAfterInliningTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/inliner/SingleTargetAfterInliningTest.java
@@ -75,13 +75,15 @@
ClassSubject aClassSubject = inspector.clazz(A.class);
assertThat(aClassSubject, isPresent());
- // B.foo() should be absent if the max inlining depth is 1, because indirection() has been
- // inlined into main(), which makes B.foo() eligible for inlining into main().
+ // B.foo() could potentially be absent if the max inlining depth is 1, because indirection() has
+ // been inlined into main(), which makes B.foo() eligible for inlining into main() since we can
+ // compute a single target. However, because indirection() can be inlined into multiple callers
+ // we cannot trust the single caller predicate. If indirection() also has a singler caller we
+ // could propagate the single call site property to B.foo().
ClassSubject bClassSubject = inspector.clazz(B.class);
assertThat(bClassSubject, isPresent());
- assertThat(
- bClassSubject.uniqueMethodWithOriginalName("foo"),
- notIf(isPresent(), maxInliningDepth == 1));
+ // TODO(b/286058449): We could inline this.
+ assertThat(bClassSubject.uniqueMethodWithOriginalName("foo"), isPresent());
// B.bar() should always be inlined because it is marked as @AlwaysInline.
assertThat(
diff --git a/src/test/java/com/android/tools/r8/startup/SingleCallerBridgeStartupTest.java b/src/test/java/com/android/tools/r8/startup/SingleCallerBridgeStartupTest.java
index 4a27e0f..1cb68de 100644
--- a/src/test/java/com/android/tools/r8/startup/SingleCallerBridgeStartupTest.java
+++ b/src/test/java/com/android/tools/r8/startup/SingleCallerBridgeStartupTest.java
@@ -4,6 +4,9 @@
package com.android.tools.r8.startup;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
@@ -12,6 +15,7 @@
import com.android.tools.r8.startup.profile.ExternalStartupItem;
import com.android.tools.r8.startup.profile.ExternalStartupMethod;
import com.android.tools.r8.startup.utils.StartupTestingUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.google.common.collect.ImmutableList;
import java.util.Collection;
import org.junit.Test;
@@ -55,9 +59,16 @@
(appView, inlinee, inliningDepth) ->
inlinee.getMethodReference().equals(barMethod))
.setMinApi(parameters)
+ .compile()
+ .inspect(
+ inspector -> {
+ // Assert that foo is not inlined.
+ ClassSubject A = inspector.clazz(A.class);
+ assertThat(A, isPresent());
+ assertThat(A.uniqueMethodWithOriginalName("foo"), isPresent());
+ })
.run(parameters.getRuntime(), Main.class)
- // TODO(b/285021603): We should not fail here.
- .assertFailureWithErrorThatThrows(NoSuchMethodError.class);
+ .assertSuccessWithOutputLines("A::foo", "A::foo");
}
static class Main {
