Revert "Reenable single caller inlining for virtual methods"
This reverts commit 6538572c13ef7a84f76411f5dfd6398d50fd4dd5.
Bug: b/336791970
Change-Id: I7ebe92ec2618da4e2ee15197e984a1386a523c37
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorCodeScanner.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorCodeScanner.java
index 7ee35d8..1f95169 100644
--- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorCodeScanner.java
+++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/ArgumentPropagatorCodeScanner.java
@@ -774,10 +774,6 @@
return resolvedMethod.getReference();
}
- if (isMonomorphicVirtualMethod(resolvedMethod)) {
- return resolvedMethod.getReference();
- }
-
if (invoke.isInvokeInterface()) {
assert !isMonomorphicVirtualMethod(resolvedMethod);
return getVirtualRootMethod(resolvedMethod);
@@ -785,6 +781,10 @@
assert invoke.isInvokeSuper() || invoke.isInvokeVirtual();
+ if (isMonomorphicVirtualMethod(resolvedMethod)) {
+ return resolvedMethod.getReference();
+ }
+
DexMethod rootMethod = getVirtualRootMethod(resolvedMethod);
assert rootMethod != null;
assert !isMonomorphicVirtualMethod(resolvedMethod)
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/codescanner/VirtualRootMethodsAnalysisBase.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/codescanner/VirtualRootMethodsAnalysisBase.java
index 3343089..b5f1374 100644
--- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/codescanner/VirtualRootMethodsAnalysisBase.java
+++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/codescanner/VirtualRootMethodsAnalysisBase.java
@@ -3,8 +3,9 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.optimize.argumentpropagation.codescanner;
+import static com.android.tools.r8.utils.MapUtils.ignoreKey;
+
import com.android.tools.r8.graph.AppView;
-import com.android.tools.r8.graph.DexClassAndMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.ImmediateProgramSubtypingInfo;
@@ -13,13 +14,8 @@
import com.android.tools.r8.shaking.AppInfoWithLiveness;
import com.android.tools.r8.utils.collections.DexMethodSignatureMap;
import com.android.tools.r8.utils.collections.ProgramMethodSet;
-import com.google.common.collect.Sets;
-import java.util.ArrayList;
-import java.util.Collections;
import java.util.IdentityHashMap;
-import java.util.List;
import java.util.Map;
-import java.util.Set;
import java.util.function.Consumer;
/**
@@ -31,89 +27,58 @@
protected static class VirtualRootMethod {
private final VirtualRootMethod parent;
- private final ProgramMethod method;
-
- private Set<VirtualRootMethod> overrides = Collections.emptySet();
- private List<VirtualRootMethod> siblings = Collections.emptyList();
- private boolean mayDispatchOutsideProgram = false;
+ private final ProgramMethod root;
+ private final ProgramMethodSet overrides = ProgramMethodSet.create();
VirtualRootMethod(ProgramMethod root) {
this(root, null);
}
- VirtualRootMethod(ProgramMethod method, VirtualRootMethod parent) {
- assert method != null;
+ VirtualRootMethod(ProgramMethod root, VirtualRootMethod parent) {
+ assert root != null;
this.parent = parent;
- this.method = method;
+ this.root = root;
}
- void addOverride(VirtualRootMethod override) {
- assert !override.getMethod().isStructurallyEqualTo(method);
- assert override.getMethod().getReference().match(method.getReference());
- if (overrides.isEmpty()) {
- overrides = Sets.newIdentityHashSet();
- }
+ void addOverride(ProgramMethod override) {
+ assert override.getDefinition() != root.getDefinition();
+ assert override.getMethodSignature().equals(root.getMethodSignature());
overrides.add(override);
if (hasParent()) {
getParent().addOverride(override);
}
- for (VirtualRootMethod sibling : siblings) {
- sibling.addOverride(override);
- }
- }
-
- void addSibling(VirtualRootMethod sibling) {
- if (siblings.isEmpty()) {
- siblings = new ArrayList<>(1);
- }
- siblings.add(sibling);
- }
-
- void setMayDispatchOutsideProgram() {
- mayDispatchOutsideProgram = true;
}
boolean hasParent() {
return parent != null;
}
- boolean hasSiblings() {
- return !siblings.isEmpty();
- }
-
VirtualRootMethod getParent() {
return parent;
}
- VirtualRootMethod getRoot() {
- return hasParent() ? getParent().getRoot() : this;
+ ProgramMethod getRoot() {
+ return root;
}
- ProgramMethod getMethod() {
- return method;
- }
-
- VirtualRootMethod getSingleDispatchTarget() {
- assert !hasParent();
- if (isMayDispatchOutsideProgramSet()) {
- return null;
- }
- VirtualRootMethod singleDispatchTarget = isAbstract() ? null : this;
- for (VirtualRootMethod override : overrides) {
- if (!override.isAbstract()) {
- if (singleDispatchTarget != null) {
+ ProgramMethod getSingleNonAbstractMethod() {
+ ProgramMethod singleNonAbstractMethod = root.getAccessFlags().isAbstract() ? null : root;
+ for (ProgramMethod override : overrides) {
+ if (!override.getAccessFlags().isAbstract()) {
+ if (singleNonAbstractMethod != null) {
// Not a single non-abstract method.
return null;
}
- singleDispatchTarget = override;
+ singleNonAbstractMethod = override;
}
}
- assert singleDispatchTarget == null || !singleDispatchTarget.isAbstract();
- return singleDispatchTarget;
+ assert singleNonAbstractMethod == null
+ || !singleNonAbstractMethod.getAccessFlags().isAbstract();
+ return singleNonAbstractMethod;
}
- void forEach(Consumer<VirtualRootMethod> consumer) {
- consumer.accept(this);
+ void forEach(Consumer<ProgramMethod> consumer) {
+ consumer.accept(root);
overrides.forEach(consumer);
}
@@ -121,12 +86,10 @@
return !overrides.isEmpty();
}
- boolean isAbstract() {
- return method.getAccessFlags().isAbstract();
- }
-
- boolean isMayDispatchOutsideProgramSet() {
- return mayDispatchOutsideProgram;
+ boolean isInterfaceMethodWithSiblings() {
+ // TODO(b/190154391): Conservatively returns true for all interface methods, but should only
+ // return true for those with siblings.
+ return root.getHolder().isInterface();
}
}
@@ -159,48 +122,18 @@
DexMethodSignatureMap<VirtualRootMethod> virtualRootMethodsForSuperclass =
virtualRootMethodsPerClass.get(superclass);
virtualRootMethodsForSuperclass.forEach(
- (signature, info) -> {
- virtualRootMethodsForClass.compute(
- signature,
- (ignore, existing) -> {
- if (existing == null || existing == info) {
- return info;
- } else {
- // We iterate the immediate supertypes in-order using
- // forEachImmediateProgramSuperClass. Therefore, the current method is
- // guaranteed to be an interface method when existing != null.
- assert info.getMethod().getHolder().isInterface();
- if (!existing.getMethod().getHolder().isInterface()) {
- existing.addSibling(info);
- info.addOverride(existing);
- }
- return existing;
- }
- });
- if (!clazz.isInterface() && superclass.isInterface()) {
- DexClassAndMethod resolvedMethod =
- appView.appInfo().resolveMethodOnClass(clazz, signature).getResolutionPair();
- if (resolvedMethod != null
- && !resolvedMethod.isProgramMethod()
- && !resolvedMethod.getAccessFlags().isAbstract()) {
- info.setMayDispatchOutsideProgram();
- }
- }
- });
+ (signature, info) ->
+ virtualRootMethodsForClass.computeIfAbsent(
+ signature, ignoreKey(() -> new VirtualRootMethod(info.getRoot(), info))));
});
clazz.forEachProgramVirtualMethod(
- method ->
- virtualRootMethodsForClass.compute(
- method,
- (ignore, parent) -> {
- if (parent == null) {
- return new VirtualRootMethod(method);
- } else {
- VirtualRootMethod override = new VirtualRootMethod(method, parent);
- parent.addOverride(override);
- return override;
- }
- }));
+ method -> {
+ if (virtualRootMethodsForClass.containsKey(method)) {
+ virtualRootMethodsForClass.get(method).getParent().addOverride(method);
+ } else {
+ virtualRootMethodsForClass.put(method, new VirtualRootMethod(method));
+ }
+ });
return virtualRootMethodsForClass;
}
@@ -214,41 +147,37 @@
VirtualRootMethod virtualRootMethod =
virtualRootMethodsForClass.remove(rootCandidate.getMethodSignature());
acceptVirtualMethod(rootCandidate, virtualRootMethod);
- if (virtualRootMethod.hasParent()
- || !rootCandidate.isStructurallyEqualTo(virtualRootMethod.getMethod())) {
+ if (!rootCandidate.isStructurallyEqualTo(virtualRootMethod.getRoot())) {
return;
}
- if (!virtualRootMethod.hasOverrides()
- && !virtualRootMethod.hasSiblings()
- && !virtualRootMethod.isMayDispatchOutsideProgramSet()) {
+ boolean isMonomorphicVirtualMethod =
+ !clazz.isInterface() && !virtualRootMethod.hasOverrides();
+ if (isMonomorphicVirtualMethod) {
monomorphicVirtualRootMethods.add(rootCandidate);
} else {
- VirtualRootMethod singleDispatchTarget = virtualRootMethod.getSingleDispatchTarget();
- if (singleDispatchTarget != null) {
+ ProgramMethod singleNonAbstractMethod = virtualRootMethod.getSingleNonAbstractMethod();
+ if (singleNonAbstractMethod != null
+ && !virtualRootMethod.isInterfaceMethodWithSiblings()) {
virtualRootMethod.forEach(
- method -> setRootMethod(method, virtualRootMethod, singleDispatchTarget));
- monomorphicVirtualNonRootMethods.add(singleDispatchTarget.getMethod());
+ method -> {
+ // Interface methods can have siblings and can therefore not be mapped to their
+ // unique non-abstract implementation, unless the interface method does not have
+ // any siblings.
+ virtualRootMethods.put(
+ method.getReference(), singleNonAbstractMethod.getReference());
+ });
+ if (!singleNonAbstractMethod.getHolder().isInterface()) {
+ monomorphicVirtualNonRootMethods.add(singleNonAbstractMethod);
+ }
} else {
virtualRootMethod.forEach(
- method -> setRootMethod(method, virtualRootMethod, virtualRootMethod));
+ method ->
+ virtualRootMethods.put(method.getReference(), rootCandidate.getReference()));
}
}
});
}
- private void setRootMethod(
- VirtualRootMethod method, VirtualRootMethod currentRoot, VirtualRootMethod root) {
- // Since the same method can have multiple roots due to interface methods, we only allow
- // controlling the virtual root of methods that are rooted at the current root. Otherwise, we
- // would be setting the virtual root of the same method multiple times, which could lead to
- // non-determinism in the result (i.e., the `virtualRootMethods` map).
- if (method.getRoot() == currentRoot) {
- DexMethod rootReference = root.getMethod().getReference();
- DexMethod previous = virtualRootMethods.put(method.getMethod().getReference(), rootReference);
- assert previous == null || previous.isIdenticalTo(rootReference);
- }
- }
-
protected void acceptVirtualMethod(ProgramMethod method, VirtualRootMethod virtualRootMethod) {
// Intentionally empty.
}
diff --git a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InterfaceMethodArgumentPropagator.java b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InterfaceMethodArgumentPropagator.java
index 5fcd8db..675bff8 100644
--- a/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InterfaceMethodArgumentPropagator.java
+++ b/src/main/java/com/android/tools/r8/optimize/argumentpropagation/propagation/InterfaceMethodArgumentPropagator.java
@@ -108,12 +108,10 @@
return;
}
- // If the method state is monomorphic, then this is an interface method with no overrides.
- // In this case, there is no need to add methodState to interfaceState.
- if (methodState.isMonomorphic()) {
- return;
- }
-
+ // TODO(b/190154391): We should always have an unknown or polymorphic state, but it would
+ // be better to use a monomorphic state when the interface method is a default method
+ // with no overrides (CF backend only). In this case, there is no need to add methodState
+ // to interfaceState.
assert methodState.isUnknown() || methodState.asConcrete().isPolymorphic();
interfaceState.addMethodState(appView, method, methodState);
});
diff --git a/src/main/java/com/android/tools/r8/optimize/singlecaller/SingleCallerInliner.java b/src/main/java/com/android/tools/r8/optimize/singlecaller/SingleCallerInliner.java
index 70b50c9..a6e1da7 100644
--- a/src/main/java/com/android/tools/r8/optimize/singlecaller/SingleCallerInliner.java
+++ b/src/main/java/com/android/tools/r8/optimize/singlecaller/SingleCallerInliner.java
@@ -68,8 +68,8 @@
}
public void run(ExecutorService executorService) throws ExecutionException {
- ProgramMethodSet monomorphicVirtualMethods =
- computeMonomorphicVirtualRootMethods(executorService);
+ // TODO(b/335584013): Re-enable monomorphic method analysis.
+ ProgramMethodSet monomorphicVirtualMethods = ProgramMethodSet.empty();
ProgramMethodMap<ProgramMethod> singleCallerMethods =
new SingleCallerScanner(appView, monomorphicVirtualMethods)
.getSingleCallerMethods(executorService);
diff --git a/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java b/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java
index e4872c7..377be48 100644
--- a/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java
+++ b/src/main/java/com/android/tools/r8/utils/collections/DexMethodSignatureMap.java
@@ -8,7 +8,6 @@
import com.android.tools.r8.graph.DexEncodedMethod;
import com.android.tools.r8.graph.DexMethod;
import com.android.tools.r8.graph.DexMethodSignature;
-import com.android.tools.r8.graph.ProgramMethod;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
@@ -144,12 +143,6 @@
return backing.compute(key, remappingFunction);
}
- public T compute(
- ProgramMethod method,
- BiFunction<? super DexMethodSignature, ? super T, ? extends T> remappingFunction) {
- return compute(method.getMethodSignature(), remappingFunction);
- }
-
@Override
public T merge(
DexMethodSignature key,
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/inliner/IllegalSingleCallerInliningOfNonAbstractSiblingTest.java b/src/test/java/com/android/tools/r8/ir/optimize/inliner/IllegalSingleCallerInliningOfNonAbstractSiblingTest.java
deleted file mode 100644
index 117b090..0000000
--- a/src/test/java/com/android/tools/r8/ir/optimize/inliner/IllegalSingleCallerInliningOfNonAbstractSiblingTest.java
+++ /dev/null
@@ -1,78 +0,0 @@
-// Copyright (c) 2024, 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.ir.optimize.inliner;
-
-import com.android.tools.r8.NeverClassInline;
-import com.android.tools.r8.NoHorizontalClassMerging;
-import com.android.tools.r8.NoVerticalClassMerging;
-import com.android.tools.r8.TestBase;
-import com.android.tools.r8.TestParameters;
-import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.ir.optimize.Inliner.Reason;
-import com.google.common.collect.ImmutableSet;
-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)
-public class IllegalSingleCallerInliningOfNonAbstractSiblingTest extends TestBase {
-
- @Parameter(0)
- public TestParameters parameters;
-
- @Parameters(name = "{0}")
- public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimesAndApiLevels().build();
- }
-
- @Test
- public void test() throws Exception {
- testForR8(parameters.getBackend())
- .addInnerClasses(getClass())
- .addKeepMainRule(Main.class)
- .addOptionsModification(
- options -> options.testing.validInliningReasons = ImmutableSet.of(Reason.SINGLE_CALLER))
- .enableNoHorizontalClassMergingAnnotations()
- .enableNoVerticalClassMergingAnnotations()
- .enableNeverClassInliningAnnotations()
- .setMinApi(parameters)
- .compile()
- .run(parameters.getRuntime(), Main.class)
- .assertSuccessWithOutputLines("A", "A");
- }
-
- static class Main {
-
- public static void main(String[] args) {
- I i = System.currentTimeMillis() > 0 ? new B() : new C();
- i.m();
-
- new A().m();
- }
- }
-
- interface I {
-
- default void m() {
- System.out.println("C");
- }
- }
-
- @NeverClassInline
- @NoHorizontalClassMerging
- @NoVerticalClassMerging
- static class A {
-
- public void m() {
- System.out.println("A");
- }
- }
-
- static class B extends A implements I {}
-
- @NoHorizontalClassMerging
- static class C implements I {}
-}
