Remove special handling for desugared lambdas in class inliner
Change-Id: I614f37598a38c8bf9456016fd4ce70e39851a180
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 b8ce45a..35f7041 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
@@ -274,9 +274,7 @@
assert appView.rootSet() != null;
AppView<AppInfoWithLiveness> appViewWithLiveness = appView.withLiveness();
this.classInliner =
- options.enableClassInlining && options.enableInlining
- ? new ClassInliner(lambdaRewriter)
- : null;
+ options.enableClassInlining && options.enableInlining ? new ClassInliner() : null;
this.classStaticizer =
options.enableClassStaticizer ? new ClassStaticizer(appViewWithLiveness, this) : null;
this.dynamicTypeOptimization =
diff --git a/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java b/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
index 36fd691..216a9d7 100644
--- a/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/desugar/LambdaRewriter.java
@@ -262,15 +262,6 @@
return false;
}
- /**
- * Returns a synthetic class for desugared lambda or `null` if the `type` does not represent one.
- * Method can be called concurrently.
- */
- public DexProgramClass getLambdaClass(DexType type) {
- LambdaClass lambdaClass = getKnown(knownLambdaClasses, type);
- return lambdaClass == null ? null : lambdaClass.getOrCreateLambdaClass();
- }
-
/** Generates lambda classes and adds them to the builder. */
public void finalizeLambdaDesugaringForD8(Builder<?> builder, ExecutorService executorService)
throws ExecutionException {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
index 36fd914..538cba6 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInliner.java
@@ -13,7 +13,6 @@
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionOrPhi;
import com.android.tools.r8.ir.code.Value;
-import com.android.tools.r8.ir.desugar.LambdaRewriter;
import com.android.tools.r8.ir.optimize.CodeRewriter;
import com.android.tools.r8.ir.optimize.Inliner;
import com.android.tools.r8.ir.optimize.InliningOracle;
@@ -60,14 +59,9 @@
ELIGIBLE
}
- private final LambdaRewriter lambdaRewriter;
private final ConcurrentHashMap<DexClass, EligibilityStatus> knownClasses =
new ConcurrentHashMap<>();
- public ClassInliner(LambdaRewriter lambdaRewriter) {
- this.lambdaRewriter = lambdaRewriter;
- }
-
private void logEligibilityStatus(
DexEncodedMethod context, Instruction root, EligibilityStatus status) {
if (Log.ENABLED && Log.isLoggingEnabledFor(ClassInliner.class)) {
@@ -192,7 +186,6 @@
InlineCandidateProcessor processor =
new InlineCandidateProcessor(
appView,
- lambdaRewriter,
inliner,
clazz -> isClassEligible(appView, clazz),
isProcessedConcurrently,
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
index c522ff3..35e84ef 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/InlineCandidateProcessor.java
@@ -35,7 +35,6 @@
import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
import com.android.tools.r8.ir.code.StaticGet;
import com.android.tools.r8.ir.code.Value;
-import com.android.tools.r8.ir.desugar.LambdaRewriter;
import com.android.tools.r8.ir.optimize.Inliner;
import com.android.tools.r8.ir.optimize.Inliner.InlineAction;
import com.android.tools.r8.ir.optimize.Inliner.InliningInfo;
@@ -78,7 +77,6 @@
ImmutableSet.of(If.Type.EQ, If.Type.NE);
private final AppView<AppInfoWithLiveness> appView;
- private final LambdaRewriter lambdaRewriter;
private final Inliner inliner;
private final Function<DexClass, EligibilityStatus> isClassEligible;
private final Predicate<DexEncodedMethod> isProcessedConcurrently;
@@ -88,7 +86,6 @@
private Value eligibleInstance;
private DexType eligibleClass;
private DexProgramClass eligibleClassDefinition;
- private boolean isDesugaredLambda;
private final Map<InvokeMethodWithReceiver, InliningInfo> methodCallsOnInstance =
new IdentityHashMap<>();
@@ -106,14 +103,12 @@
InlineCandidateProcessor(
AppView<AppInfoWithLiveness> appView,
- LambdaRewriter lambdaRewriter,
Inliner inliner,
Function<DexClass, EligibilityStatus> isClassEligible,
Predicate<DexEncodedMethod> isProcessedConcurrently,
DexEncodedMethod method,
Instruction root) {
this.appView = appView;
- this.lambdaRewriter = lambdaRewriter;
this.inliner = inliner;
this.isClassEligible = isClassEligible;
this.method = method;
@@ -165,11 +160,6 @@
if (!eligibleClass.isClassType()) {
return EligibilityStatus.NON_CLASS_TYPE;
}
- if (lambdaRewriter != null) {
- // Check if the class is synthesized for a desugared lambda
- eligibleClassDefinition = lambdaRewriter.getLambdaClass(eligibleClass);
- isDesugaredLambda = eligibleClassDefinition != null;
- }
if (eligibleClassDefinition == null) {
eligibleClassDefinition = asProgramClassOrNull(appView.definitionFor(eligibleClass));
}
@@ -213,65 +203,6 @@
}
assert root.isStaticGet();
-
- // We know that desugared lambda classes satisfy eligibility requirements.
- if (isDesugaredLambda) {
- return EligibilityStatus.ELIGIBLE;
- }
-
- // Checking if we can safely inline class implemented following singleton-like
- // pattern, by which we assume a static final field holding on to the reference
- // initialized in class constructor.
- //
- // In general we are targeting cases when the class is defined as:
- //
- // class X {
- // static final X F;
- // static {
- // F = new X();
- // }
- // }
- //
- // and being used as follows:
- //
- // void foo() {
- // f = X.F;
- // f.bar();
- // }
- //
- // The main difference from the similar case of class inliner with 'new-instance'
- // instruction is that in this case the instance we inline is not just leaked, but
- // is actually published via X.F field. There are several risks we need to address
- // in this case:
- //
- // Risk: instance stored in field X.F has changed after it was initialized in
- // class initializer
- // Solution: we assume that final field X.F is not modified outside the class
- // initializer. In rare cases when it is (e.g. via reflections) it should
- // be marked with keep rules
- //
- // Risk: instance stored in field X.F is not initialized yet
- // Solution: not initialized instance can only be visible if X.<clinit>
- // triggers other class initialization which references X.F. This
- // situation should never happen if we:
- // -- don't allow any superclasses to have static initializer,
- // -- don't allow any subclasses,
- // -- guarantee the class has trivial class initializer
- // (see CodeRewriter::computeClassInitializerInfo), and
- // -- guarantee the instance is initialized with trivial instance
- // initializer (see CodeRewriter::computeInstanceInitializerInfo)
- //
- // Risk: instance stored in field X.F was mutated
- // Solution: we require that class X does not have any instance fields, and
- // if any of its superclasses has instance fields, accessing them will make
- // this instance not eligible for inlining. I.e. even though the instance is
- // publicized and its state has been mutated, it will not effect the logic
- // of class inlining
- //
-
- if (!eligibleClassDefinition.instanceFields().isEmpty()) {
- return EligibilityStatus.HAS_INSTANCE_FIELDS;
- }
return EligibilityStatus.ELIGIBLE;
}
@@ -305,10 +236,24 @@
indirectUsers.addAll(alias.uniqueUsers());
continue;
}
- // Field read/write.
- if (user.isInstanceGet()
- || (user.isInstancePut()
- && receivers.addIllegalReceiverAlias(user.asInstancePut().value()))) {
+
+ if (user.isInstanceGet()) {
+ if (root.isStaticGet()) {
+ // We don't have a replacement for this field read.
+ return user; // Not eligible.
+ }
+ DexEncodedField field =
+ appView.appInfo().resolveField(user.asFieldInstruction().getField());
+ if (field == null || field.isStatic()) {
+ return user; // Not eligible.
+ }
+ continue;
+ }
+
+ if (user.isInstancePut()) {
+ if (!receivers.addIllegalReceiverAlias(user.asInstancePut().value())) {
+ return user; // Not eligible.
+ }
DexEncodedField field =
appView.appInfo().resolveField(user.asFieldInstruction().getField());
if (field == null || field.isStatic()) {
@@ -707,12 +652,6 @@
return null;
}
- if (isDesugaredLambda) {
- // Lambda desugaring synthesizes eligible constructors.
- markSizeForInlining(invoke, singleTarget);
- return new InliningInfo(singleTarget, eligibleClass);
- }
-
// Check that the entire constructor chain can be inlined into the current context.
DexItemFactory dexItemFactory = appView.dexItemFactory();
DexMethod parent = instanceInitializerInfo.getParent();
@@ -878,6 +817,15 @@
return null;
}
+ if (root.isStaticGet()) {
+ // If we are class inlining a singleton instance from a static-get, then we don't the value of
+ // the fields.
+ ParameterUsage receiverUsage = optimizationInfo.getParameterUsages(0);
+ if (receiverUsage == null || receiverUsage.hasFieldRead) {
+ return null;
+ }
+ }
+
// If the method returns receiver and the return value is actually
// used in the code we need to make some additional checks.
if (!eligibilityAcceptanceCheck.test(eligibility)) {
@@ -1013,6 +961,14 @@
return false;
}
+ if (root.isStaticGet()) {
+ // If we are class inlining a singleton instance from a static-get, then we don't the value of
+ // the fields.
+ if (parameterUsage.hasFieldRead) {
+ return false;
+ }
+ }
+
if (parameterUsage.isReturned) {
if (invoke.outValue() != null && invoke.outValue().hasAnyUsers()) {
// Used as return value which is not ignored.
@@ -1080,12 +1036,6 @@
private boolean exemptFromInstructionLimit(DexEncodedMethod inlinee) {
DexType inlineeHolder = inlinee.method.holder;
- if (isDesugaredLambda && inlineeHolder == eligibleClass) {
- return true;
- }
- if (appView.appInfo().isPinned(inlineeHolder)) {
- return false;
- }
DexClass inlineeClass = appView.definitionFor(inlineeHolder);
assert inlineeClass != null;
@@ -1116,15 +1066,6 @@
if (isProcessedConcurrently.test(singleTarget)) {
return false;
}
- if (isDesugaredLambda && !singleTarget.accessFlags.isBridge()) {
- // OK if this is the call to the main method of a desugared lambda (for both direct and
- // indirect calls).
- //
- // Note: This is needed because lambda methods are generally processed in the same batch as
- // they are class inlined, which means that the call to isInliningCandidate() below will
- // return false.
- return true;
- }
if (!singleTarget.isInliningCandidate(
method, Reason.SIMPLE, appView.appInfo(), NopWhyAreYouNotInliningReporter.getInstance())) {
// If `singleTarget` is not an inlining candidate, we won't be able to inline it here.
diff --git a/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java b/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
index 8154dfc..92d02ee 100644
--- a/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunExamplesAndroidOTest.java
@@ -174,7 +174,7 @@
.withBuilderTransformation(ToolHelper::allowTestProguardOptions)
.withBuilderTransformation(
b -> b.addProguardConfiguration(PROGUARD_OPTIONS_N_PLUS, Origin.unknown()))
- .withDexCheck(inspector -> checkLambdaCount(inspector, 5, "lambdadesugaringnplus"))
+ .withDexCheck(inspector -> checkLambdaCount(inspector, 7, "lambdadesugaringnplus"))
.run();
}
