Reland "Extend the range of assume instructions, part 3." PS1 (or rebased PS3) is same as go/r8g/42840 PS4 adds more type updates for remnant phis in ClassInliner. Bug: 141517313, 120920488 Change-Id: Idb2e960a67786f9253d8c1bcbeeecc21abdfdf01
diff --git a/src/main/java/com/android/tools/r8/ir/code/Assume.java b/src/main/java/com/android/tools/r8/ir/code/Assume.java index c4dfbc4..18ba962 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Assume.java +++ b/src/main/java/com/android/tools/r8/ir/code/Assume.java
@@ -153,6 +153,10 @@ return self; } + public boolean mayAffectStaticType() { + return isAssumeNonNull(); + } + @Override public boolean couldIntroduceAnAlias(AppView<?> appView, Value root) { assert root != null && root.getTypeLattice().isReference();
diff --git a/src/main/java/com/android/tools/r8/ir/code/IRCode.java b/src/main/java/com/android/tools/r8/ir/code/IRCode.java index fa3074e..cb32015 100644 --- a/src/main/java/com/android/tools/r8/ir/code/IRCode.java +++ b/src/main/java/com/android/tools/r8/ir/code/IRCode.java
@@ -1008,14 +1008,22 @@ } public void removeAllTrivialPhis() { - removeAllTrivialPhis(null); + removeAllTrivialPhis(null, null); } public void removeAllTrivialPhis(IRBuilder builder) { + removeAllTrivialPhis(builder, null); + } + + public void removeAllTrivialPhis(Set<Value> affectedValues) { + removeAllTrivialPhis(null, affectedValues); + } + + public void removeAllTrivialPhis(IRBuilder builder, Set<Value> affectedValues) { for (BasicBlock block : blocks) { List<Phi> phis = new ArrayList<>(block.getPhis()); for (Phi phi : phis) { - phi.removeTrivialPhi(builder); + phi.removeTrivialPhi(builder, affectedValues); } } }
diff --git a/src/main/java/com/android/tools/r8/ir/code/Phi.java b/src/main/java/com/android/tools/r8/ir/code/Phi.java index e2f9cf3..7cd8ca2 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Phi.java +++ b/src/main/java/com/android/tools/r8/ir/code/Phi.java
@@ -123,7 +123,7 @@ builder.constrainType(operand, readConstraint); appendOperand(operand); } - removeTrivialPhi(builder); + removeTrivialPhi(builder, null); } public void addOperands(List<Value> operands) { @@ -224,10 +224,10 @@ } public void removeTrivialPhi() { - removeTrivialPhi(null); + removeTrivialPhi(null, null); } - public void removeTrivialPhi(IRBuilder builder) { + void removeTrivialPhi(IRBuilder builder, Set<Value> affectedValues) { Value same = null; for (Value op : operands) { if (op == same || op == this) { @@ -252,6 +252,9 @@ if (builder != null && typeLattice.isPreciseType() && !typeLattice.isBottom()) { builder.constrainType(same, ValueTypeConstraint.fromTypeLattice(typeLattice)); } + if (affectedValues != null) { + affectedValues.addAll(this.affectedValues()); + } // Removing this phi, so get rid of it as a phi user from all of the operands to avoid // recursively getting back here with the same phi. If the phi has itself as an operand // that also removes the self-reference. @@ -277,7 +280,7 @@ replaceUsers(same); // Try to simplify phi users that might now have become trivial. for (Phi user : phiUsersToSimplify) { - user.removeTrivialPhi(builder); + user.removeTrivialPhi(builder, affectedValues); } } // Get rid of the phi itself.
diff --git a/src/main/java/com/android/tools/r8/ir/code/Value.java b/src/main/java/com/android/tools/r8/ir/code/Value.java index 2881de7..6648777 100644 --- a/src/main/java/com/android/tools/r8/ir/code/Value.java +++ b/src/main/java/com/android/tools/r8/ir/code/Value.java
@@ -19,6 +19,7 @@ import com.android.tools.r8.position.MethodPosition; import com.android.tools.r8.utils.LongInterval; import com.android.tools.r8.utils.Reporter; +import com.android.tools.r8.utils.SetUtils; import com.android.tools.r8.utils.StringDiagnostic; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableSet; @@ -424,6 +425,22 @@ return users.getFirst(); } + public Set<Instruction> aliasedUsers() { + Set<Instruction> users = SetUtils.newIdentityHashSet(uniqueUsers()); + collectAliasedUsersViaAssume(uniqueUsers(), users); + return users; + } + + private static void collectAliasedUsersViaAssume( + Set<Instruction> usersToTest, Set<Instruction> collectedUsers) { + for (Instruction user : usersToTest) { + if (user.isAssume()) { + collectedUsers.addAll(user.outValue().uniqueUsers()); + collectAliasedUsersViaAssume(user.outValue().uniqueUsers(), collectedUsers); + } + } + } + public Phi firstPhiUser() { assert !phiUsers.isEmpty(); return phiUsers.getFirst();
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 545fcff..b46a834 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
@@ -1249,26 +1249,6 @@ assert code.verifyTypes(appView); - if (nonNullTracker != null) { - // TODO(b/139246447): Once we extend this optimization to, e.g., constants of primitive args, - // this may not be the right place to collect call site optimization info. - // Collecting call-site optimization info depends on the existence of non-null IRs. - // Arguments can be changed during the debug mode. - if (!isDebugMode && appView.callSiteOptimizationInfoPropagator() != null) { - appView.callSiteOptimizationInfoPropagator().collectCallSiteOptimizationInfo(code); - } - // Computation of non-null parameters on normal exits rely on the existence of non-null IRs. - nonNullTracker.computeNonNullParamOnNormalExits(feedback, code); - } - if (aliasIntroducer != null || nonNullTracker != null || dynamicTypeOptimization != null) { - codeRewriter.removeAssumeInstructions(code); - assert code.isConsistentSSA(); - } - // Assert that we do not have unremoved non-sense code in the output, e.g., v <- non-null NULL. - assert code.verifyNoNullabilityBottomTypes(); - - assert code.verifyTypes(appView); - previous = printMethod(code, "IR before class inlining (SSA)", previous); if (classInliner != null) { @@ -1330,6 +1310,26 @@ assert code.isConsistentSSA(); } + if (nonNullTracker != null) { + // TODO(b/139246447): Once we extend this optimization to, e.g., constants of primitive args, + // this may not be the right place to collect call site optimization info. + // Collecting call-site optimization info depends on the existence of non-null IRs. + // Arguments can be changed during the debug mode. + if (!isDebugMode && appView.callSiteOptimizationInfoPropagator() != null) { + appView.callSiteOptimizationInfoPropagator().collectCallSiteOptimizationInfo(code); + } + // Computation of non-null parameters on normal exits rely on the existence of non-null IRs. + nonNullTracker.computeNonNullParamOnNormalExits(feedback, code); + } + if (aliasIntroducer != null || nonNullTracker != null || dynamicTypeOptimization != null) { + codeRewriter.removeAssumeInstructions(code); + assert code.isConsistentSSA(); + } + // Assert that we do not have unremoved non-sense code in the output, e.g., v <- non-null NULL. + assert code.verifyNoNullabilityBottomTypes(); + + assert code.verifyTypes(appView); + previous = printMethod(code, "IR after lambda merger (SSA)", previous); if (options.outline.enabled) {
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java index fba5ae7..1f36d25 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -218,7 +218,7 @@ // Therefore, Assume elimination may result in a trivial phi: // z <- phi(x, x) if (needToCheckTrivialPhis) { - code.removeAllTrivialPhis(); + code.removeAllTrivialPhis(valuesThatRequireWidening); } if (!valuesThatRequireWidening.isEmpty()) {
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 b656678..7df3542 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
@@ -8,9 +8,11 @@ import com.android.tools.r8.graph.DexClass; import com.android.tools.r8.graph.DexEncodedMethod; import com.android.tools.r8.graph.DexItemFactory; +import com.android.tools.r8.ir.analysis.type.TypeAnalysis; import com.android.tools.r8.ir.code.IRCode; 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; @@ -18,9 +20,11 @@ import com.android.tools.r8.ir.optimize.string.StringOptimizer; import com.android.tools.r8.logging.Log; import com.android.tools.r8.shaking.AppInfoWithLiveness; +import com.google.common.collect.Sets; import com.google.common.collect.Streams; import java.util.Iterator; import java.util.List; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; import java.util.function.Supplier; @@ -34,8 +38,14 @@ NON_CLASS_TYPE, UNKNOWN_TYPE, + // Used by isClassEligible + NON_PROGRAM_CLASS, + ABSTRACT_OR_INTERFACE, + NEVER_CLASS_INLINE, + HAS_FINALIZER, + TRIGGER_CLINIT, + // Used by InlineCandidateProcessor#isClassAndUsageEligible - INELIGIBLE_CLASS, HAS_CLINIT, HAS_INSTANCE_FIELDS, NON_FINAL_TYPE, @@ -46,7 +56,8 @@ } private final LambdaRewriter lambdaRewriter; - private final ConcurrentHashMap<DexClass, Boolean> knownClasses = new ConcurrentHashMap<>(); + private final ConcurrentHashMap<DexClass, EligibilityStatus> knownClasses = + new ConcurrentHashMap<>(); public ClassInliner(LambdaRewriter lambdaRewriter) { this.lambdaRewriter = lambdaRewriter; @@ -56,7 +67,15 @@ DexEncodedMethod context, Instruction root, EligibilityStatus status) { if (Log.ENABLED && Log.isLoggingEnabledFor(ClassInliner.class)) { Log.info(getClass(), "At %s,", context.toSourceString()); - Log.info(getClass(), "ClassInlining eligibility of %s: %s,", root, status); + Log.info(getClass(), "ClassInlining eligibility of `%s`: %s.", root, status); + } + } + + private void logIneligibleUser( + DexEncodedMethod context, Instruction root, InstructionOrPhi ineligibleUser) { + if (Log.ENABLED && Log.isLoggingEnabledFor(ClassInliner.class)) { + Log.info(getClass(), "At %s,", context.toSourceString()); + Log.info(getClass(), "Ineligible user of `%s`: `%s`.", root, ineligibleUser); } } @@ -133,7 +152,7 @@ // return 1; // } // static int method3() { - // return "F::getX"; + // return 123; // } // } // @@ -195,6 +214,7 @@ InstructionOrPhi ineligibleUser = processor.areInstanceUsersEligible(defaultOracle); if (ineligibleUser != null) { // This root may succeed if users change in future. + logIneligibleUser(code.method, root, ineligibleUser); continue; } @@ -208,7 +228,11 @@ anyInlinedMethods |= processor.processInlining(code, defaultOracle); // Restore normality. - code.removeAllTrivialPhis(); + Set<Value> affectedValues = Sets.newIdentityHashSet(); + code.removeAllTrivialPhis(affectedValues); + if (!affectedValues.isEmpty()) { + new TypeAnalysis(appView).narrowing(affectedValues); + } assert code.isConsistentSSA(); rootsIterator.remove(); repeat = true; @@ -233,11 +257,11 @@ } } - private boolean isClassEligible(AppView<AppInfoWithLiveness> appView, DexClass clazz) { - Boolean eligible = knownClasses.get(clazz); + private EligibilityStatus isClassEligible(AppView<AppInfoWithLiveness> appView, DexClass clazz) { + EligibilityStatus eligible = knownClasses.get(clazz); if (eligible == null) { - boolean computed = computeClassEligible(appView, clazz); - Boolean existing = knownClasses.putIfAbsent(clazz, computed); + EligibilityStatus computed = computeClassEligible(appView, clazz); + EligibilityStatus existing = knownClasses.putIfAbsent(clazz, computed); assert existing == null || existing == computed; eligible = existing == null ? computed : existing; } @@ -248,13 +272,19 @@ // - is not an abstract class or interface // - does not declare finalizer // - does not trigger any static initializers except for its own - private boolean computeClassEligible(AppView<AppInfoWithLiveness> appView, DexClass clazz) { - if (clazz == null - || clazz.isNotProgramClass() - || clazz.accessFlags.isAbstract() - || clazz.accessFlags.isInterface() - || appView.appInfo().neverClassInline.contains(clazz.type)) { - return false; + private EligibilityStatus computeClassEligible( + AppView<AppInfoWithLiveness> appView, DexClass clazz) { + if (clazz == null) { + return EligibilityStatus.UNKNOWN_TYPE; + } + if (clazz.isNotProgramClass()) { + return EligibilityStatus.NON_PROGRAM_CLASS; + } + if (clazz.isAbstract() || clazz.isInterface()) { + return EligibilityStatus.ABSTRACT_OR_INTERFACE; + } + if (appView.appInfo().neverClassInline.contains(clazz.type)) { + return EligibilityStatus.NEVER_CLASS_INLINE; } // Class must not define finalizer. @@ -262,11 +292,14 @@ for (DexEncodedMethod method : clazz.virtualMethods()) { if (method.method.name == dexItemFactory.finalizeMethodName && method.method.proto == dexItemFactory.objectMethods.finalize.proto) { - return false; + return EligibilityStatus.HAS_FINALIZER; } } // Check for static initializers in this class or any of interfaces it implements. - return !clazz.initializationOfParentTypesMayHaveSideEffects(appView); + if (clazz.initializationOfParentTypesMayHaveSideEffects(appView)) { + return EligibilityStatus.TRIGGER_CLINIT; + } + return EligibilityStatus.ELIGIBLE; } }
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/FieldValueHelper.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/FieldValueHelper.java index a16e9a3..d06b3f9 100644 --- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/FieldValueHelper.java +++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/FieldValueHelper.java
@@ -4,6 +4,7 @@ package com.android.tools.r8.ir.optimize.classinliner; +import static com.android.tools.r8.ir.analysis.type.Nullability.definitelyNotNull; import static com.android.tools.r8.ir.analysis.type.Nullability.maybeNull; import com.android.tools.r8.graph.AppView; @@ -14,6 +15,7 @@ import com.android.tools.r8.ir.code.IRCode; import com.android.tools.r8.ir.code.Instruction; import com.android.tools.r8.ir.code.InstructionIterator; +import com.android.tools.r8.ir.code.InstructionListIterator; import com.android.tools.r8.ir.code.Phi; import com.android.tools.r8.ir.code.Phi.RegisterReadType; import com.android.tools.r8.ir.code.Value; @@ -39,6 +41,9 @@ this.code = code; this.root = root; this.appView = appView; + // Verify that `root` is not aliased. + assert root.hasOutValue(); + assert root.outValue() == root.outValue().getAliasedValue(); } void replaceValue(Value oldValue, Value newValue) { @@ -122,10 +127,10 @@ Instruction instruction = iterator.previous(); assert instruction != null; - if (instruction == root || - (instruction.isInstancePut() && - instruction.asInstancePut().getField() == field && - instruction.asInstancePut().object() == root.outValue())) { + if (instruction == root + || (instruction.isInstancePut() + && instruction.asInstancePut().getField() == field + && instruction.asInstancePut().object().getAliasedValue() == root.outValue())) { valueProducingInsn = instruction; break; } @@ -140,12 +145,17 @@ assert root == valueProducingInsn; if (defaultValue == null) { + InstructionListIterator it = block.listIterator(code, root); // If we met newInstance it means that default value is supposed to be used. - defaultValue = - code.createValue(TypeLatticeElement.fromDexType(field.type, maybeNull(), appView)); - ConstNumber defaultValueInsn = new ConstNumber(defaultValue, 0); - defaultValueInsn.setPosition(root.getPosition()); - block.listIterator(code, root).add(defaultValueInsn); + if (field.type.isPrimitiveType()) { + defaultValue = code.createValue( + TypeLatticeElement.fromDexType(field.type, definitelyNotNull(), appView)); + ConstNumber defaultValueInsn = new ConstNumber(defaultValue, 0); + defaultValueInsn.setPosition(root.getPosition()); + it.add(defaultValueInsn); + } else { + defaultValue = it.insertConstNullInstruction(code, appView.options()); + } } return defaultValue; }
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 a2e9358..1318135 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
@@ -17,6 +17,7 @@ import com.android.tools.r8.graph.ResolutionResult; import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis; import com.android.tools.r8.ir.analysis.type.TypeAnalysis; +import com.android.tools.r8.ir.code.Assume; import com.android.tools.r8.ir.code.BasicBlock; import com.android.tools.r8.ir.code.ConstNumber; import com.android.tools.r8.ir.code.IRCode; @@ -40,16 +41,18 @@ import com.android.tools.r8.ir.optimize.info.ParameterUsagesInfo.ParameterUsage; import com.android.tools.r8.kotlin.KotlinInfo; import com.android.tools.r8.shaking.AppInfoWithLiveness; +import com.android.tools.r8.utils.ListUtils; import com.android.tools.r8.utils.Pair; +import com.android.tools.r8.utils.StringUtils; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Sets; import java.util.ArrayList; -import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Function; import java.util.function.Predicate; import java.util.function.Supplier; @@ -60,7 +63,7 @@ private final AppView<AppInfoWithLiveness> appView; private final LambdaRewriter lambdaRewriter; private final Inliner inliner; - private final Predicate<DexClass> isClassEligible; + private final Function<DexClass, EligibilityStatus> isClassEligible; private final Predicate<DexEncodedMethod> isProcessedConcurrently; private final DexEncodedMethod method; private final Instruction root; @@ -83,7 +86,7 @@ AppView<AppInfoWithLiveness> appView, LambdaRewriter lambdaRewriter, Inliner inliner, - Predicate<DexClass> isClassEligible, + Function<DexClass, EligibilityStatus> isClassEligible, Predicate<DexEncodedMethod> isProcessedConcurrently, DexEncodedMethod method, Instruction root) { @@ -140,8 +143,9 @@ // * class has class initializer marked as TrivialClassInitializer, and // class initializer initializes the field we are reading here. EligibilityStatus isClassAndUsageEligible() { - if (!isClassEligible.test(eligibleClassDefinition)) { - return EligibilityStatus.INELIGIBLE_CLASS; + EligibilityStatus status = isClassEligible.apply(eligibleClassDefinition); + if (status != EligibilityStatus.ELIGIBLE) { + return status; } if (root.isNewInstance()) { @@ -251,7 +255,7 @@ * * @return null if all users are eligible, or the first ineligible user. */ - protected InstructionOrPhi areInstanceUsersEligible(Supplier<InliningOracle> defaultOracle) { + InstructionOrPhi areInstanceUsersEligible(Supplier<InliningOracle> defaultOracle) { // No Phi users. if (eligibleInstance.numberOfPhiUsers() > 0) { return eligibleInstance.firstPhiUser(); // Not eligible. @@ -259,11 +263,19 @@ Set<Instruction> currentUsers = eligibleInstance.uniqueUsers(); while (!currentUsers.isEmpty()) { - Set<Instruction> indirectUsers = new HashSet<>(); + Set<Instruction> indirectUsers = Sets.newIdentityHashSet(); for (Instruction user : currentUsers) { + if (user.isAssume()) { + if (user.outValue().numberOfPhiUsers() > 0) { + return user.outValue().firstPhiUser(); // Not eligible. + } + indirectUsers.addAll(user.outValue().uniqueUsers()); + continue; + } // Field read/write. if (user.isInstanceGet() - || (user.isInstancePut() && user.asInstancePut().value() != eligibleInstance)) { + || (user.isInstancePut() + && user.asInstancePut().value().getAliasedValue() != eligibleInstance)) { DexField field = user.asFieldInstruction().getField(); if (field.holder == eligibleClass && eligibleClassDefinition.lookupInstanceField(field) != null) { @@ -288,7 +300,7 @@ boolean isCorrespondingConstructorCall = root.isNewInstance() && !invoke.inValues().isEmpty() - && root.outValue() == invoke.inValues().get(0); + && root.outValue() == invoke.getReceiver(); if (isCorrespondingConstructorCall) { InliningInfo inliningInfo = isEligibleConstructorCall(user.asInvokeDirect(), singleTarget, defaultOracle); @@ -343,6 +355,7 @@ // Process inlining, includes the following steps: // + // * remove linked assume instructions if any so that users of the eligible field are up-to-date. // * replace unused instance usages as arguments which are never used // * inline extra methods if any, collect new direct method calls // * inline direct methods if any @@ -352,6 +365,9 @@ // // Returns `true` if at least one method was inlined. boolean processInlining(IRCode code, Supplier<InliningOracle> defaultOracle) { + // Verify that `eligibleInstance` is not aliased. + assert eligibleInstance == eligibleInstance.getAliasedValue(); + replaceUsagesAsUnusedArgument(code); boolean anyInlinedMethods = forceInlineExtraMethodInvocations(code); @@ -374,11 +390,14 @@ // methods that need to be inlined anyway. return true; } - assert extraMethodCalls.isEmpty(); - assert unusedArguments.isEmpty(); + assert extraMethodCalls.isEmpty() + : "Remaining extra method calls: " + StringUtils.join(extraMethodCalls.entrySet(), ", "); + assert unusedArguments.isEmpty() + : "Remaining unused arg: " + StringUtils.join(unusedArguments, ", "); } anyInlinedMethods |= forceInlineDirectMethodInvocations(code); + removeAssumeInstructionsLinkedToEligibleInstance(); removeMiscUsages(code); removeFieldReads(code); removeFieldWrites(); @@ -419,6 +438,22 @@ return true; } + private void removeAssumeInstructionsLinkedToEligibleInstance() { + for (Instruction user : eligibleInstance.aliasedUsers()) { + if (!user.isAssume()) { + continue; + } + Assume<?> assumeInstruction = user.asAssume(); + Value src = assumeInstruction.src(); + Value dest = assumeInstruction.outValue(); + assert dest.numberOfPhiUsers() == 0; + dest.replaceUsers(src); + removeInstruction(user); + } + // Verify that no more assume instructions are left as users. + assert eligibleInstance.aliasedUsers().stream().noneMatch(Instruction::isAssume); + } + // Remove miscellaneous users before handling field reads. private void removeMiscUsages(IRCode code) { boolean needToRemoveUnreachableBlocks = false; @@ -495,8 +530,8 @@ } } - private void replaceFieldRead(IRCode code, - InstanceGet fieldRead, Map<DexField, FieldValueHelper> fieldHelpers) { + private void replaceFieldRead( + IRCode code, InstanceGet fieldRead, Map<DexField, FieldValueHelper> fieldHelpers) { Value value = fieldRead.outValue(); if (value != null) { FieldValueHelper helper = @@ -508,7 +543,10 @@ fieldValueHelper.replaceValue(value, newValue); } assert value.numberOfAllUsers() == 0; - new TypeAnalysis(appView).narrowing(newValue.affectedValues()); + // `newValue` could be a phi introduced by FieldValueHelper. Its initial type is set as the + // type of read field, but it could be more precise than that due to (multiple) inlining. + // Instead of values affected by `newValue`, it's necessary to begin with `newValue` itself. + new TypeAnalysis(appView).narrowing(ImmutableSet.of(newValue)); } removeInstruction(fieldRead); } @@ -539,7 +577,8 @@ assert isEligibleSingleTarget(singleTarget); // Must be a constructor called on the receiver. - if (invoke.inValues().lastIndexOf(eligibleInstance) != 0) { + if (ListUtils.lastIndexMatching( + invoke.inValues(), v -> v.getAliasedValue() == eligibleInstance) != 0) { return null; } @@ -594,7 +633,7 @@ : null; } - // An invoke is eligible for inlinining in the following cases: + // An invoke is eligible for inlining in the following cases: // // - if it does not return the receiver // - if there are no uses of the out value @@ -645,7 +684,8 @@ DexEncodedMethod singleTarget, Set<Instruction> indirectUsers) { assert isEligibleSingleTarget(singleTarget); - if (invoke.inValues().lastIndexOf(eligibleInstance) > 0) { + if (ListUtils.lastIndexMatching( + invoke.inValues(), v -> v.getAliasedValue() == eligibleInstance) > 0) { return null; // Instance passed as an argument. } return isEligibleVirtualMethodCall( @@ -714,7 +754,8 @@ return false; } if (invoke.isInvokeMethodWithReceiver() - && invoke.asInvokeMethodWithReceiver().getReceiver() == eligibleInstance) { + && invoke.asInvokeMethodWithReceiver().getReceiver().getAliasedValue() + == eligibleInstance) { return false; } if (invoke.isInvokeSuper()) { @@ -755,7 +796,7 @@ // If we got here with invocation on receiver the user is ineligible. if (invoke.isInvokeMethodWithReceiver()) { - if (arguments.get(0) == eligibleInstance) { + if (arguments.get(0).getAliasedValue() == eligibleInstance) { return false; } @@ -775,7 +816,7 @@ } for (int argIndex = 0; argIndex < arguments.size(); argIndex++) { - Value argument = arguments.get(argIndex); + Value argument = arguments.get(argIndex).getAliasedValue(); if (argument == eligibleInstance && optimizationInfo.getParameterUsages(argIndex).notUsed()) { // Reference can be removed since it's not used. unusedArguments.add(new Pair<>(invoke, argIndex)); @@ -796,7 +837,7 @@ Supplier<InliningOracle> defaultOracle) { // Go through all arguments, see if all usages of eligibleInstance are good. for (int argIndex = 0; argIndex < arguments.size(); argIndex++) { - Value argument = arguments.get(argIndex); + Value argument = arguments.get(argIndex).getAliasedValue(); if (argument != eligibleInstance) { continue; // Nothing to worry about. }
diff --git a/src/main/java/com/android/tools/r8/utils/ListUtils.java b/src/main/java/com/android/tools/r8/utils/ListUtils.java index a9c3f98..370d5e2 100644 --- a/src/main/java/com/android/tools/r8/utils/ListUtils.java +++ b/src/main/java/com/android/tools/r8/utils/ListUtils.java
@@ -8,9 +8,19 @@ import java.util.Collection; import java.util.List; import java.util.function.Function; +import java.util.function.Predicate; public class ListUtils { + public static <T> int lastIndexMatching(List<T> list, Predicate<T> tester) { + for (int i = list.size() - 1; i >= 0; i--) { + if (tester.test(list.get(i))) { + return i; + } + } + return -1; + } + public static <S, T> List<T> map(Collection<S> list, Function<S, T> fn) { List<T> result = new ArrayList<>(list.size()); for (S element : list) {
diff --git a/src/test/java/com/android/tools/r8/utils/ListUtilsTest.java b/src/test/java/com/android/tools/r8/utils/ListUtilsTest.java new file mode 100644 index 0000000..3a63555 --- /dev/null +++ b/src/test/java/com/android/tools/r8/utils/ListUtilsTest.java
@@ -0,0 +1,50 @@ +// Copyright (c) 2019, 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.utils; + +import static org.junit.Assert.assertEquals; + +import java.util.ArrayList; +import java.util.List; +import java.util.function.Predicate; +import org.junit.Test; + +public class ListUtilsTest { + + private List<Integer> createInputData(int size) { + List<Integer> input = new ArrayList<>(size); + for (int i = 0; i < size; i++) { + input.add(i); + } + return input; + } + + @Test + public void lastIndexOf_outOfRange() { + List<Integer> input = createInputData(3); + Predicate<Integer> tester = x -> x * x == -1; + assertEquals(-1, ListUtils.lastIndexMatching(input, tester)); + } + + @Test + public void lastIndexOf_first() { + List<Integer> input = createInputData(3); + Predicate<Integer> tester = x -> x * x == 0; + assertEquals(0, ListUtils.lastIndexMatching(input, tester)); + } + + @Test + public void lastIndexOf_middle() { + List<Integer> input = createInputData(4); + Predicate<Integer> tester = x -> x * x == 4; + assertEquals(2, ListUtils.lastIndexMatching(input, tester)); + } + + @Test + public void lastIndexOf_last() { + List<Integer> input = createInputData(2); + Predicate<Integer> tester = x -> x * x == 1; + assertEquals(1, ListUtils.lastIndexMatching(input, tester)); + } +}