Re^2land "Extend the range of assume instructions, part3."
PS2 is (almost) same as go/r8g/43460, along with two changes:
* verify types after class inlining, and
* move alias removal one step forwards (before lambda merger)
in preparation for addressing b/141654799 separately.
PS3 resolves the regression added by that CL.
Test: tools/test.py
Test: tools/run_on_app.py --app youtube --compiler r8 --version 12.17
Test: tools/run_on_app.py --app youtube --compiler r8 --version 14.19
Test: tools/run_on_as_app.py --app tivi
Bug: 141667708, 120920488
Change-Id: Ia7e7e9ea18ef6cab1ffbaaf7109520cc06ebb69a
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..95dac01 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) {
+ public 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 4105077..8e57306 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..1b31d62 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) {
@@ -1293,6 +1273,7 @@
Integer.MAX_VALUE / 2,
Integer.MAX_VALUE / 2)));
assert code.isConsistentSSA();
+ assert code.verifyTypes(appView);
}
previous = printMethod(code, "IR after class inlining (SSA)", previous);
@@ -1323,6 +1304,26 @@
twrCloseResourceRewriter.rewriteMethodCode(code);
}
+ 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 twr close resource rewriter (SSA)", previous);
if (lambdaMerger != null) {
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 387571b..f550309 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;
@@ -225,7 +229,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;
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 e207e50..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
@@ -41,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) {
@@ -124,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;
}
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 0a2ca69..6fdba3d 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,12 +41,14 @@
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.Iterables;
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;
@@ -261,8 +264,15 @@
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)) {
@@ -290,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);
@@ -345,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
@@ -354,6 +365,8 @@
//
// 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);
@@ -376,11 +389,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();
@@ -421,6 +437,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;
@@ -510,7 +542,11 @@
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.
+ // In addition to values affected by `newValue`, it's necessary to revisit `newValue` itself.
+ new TypeAnalysis(appView).narrowing(
+ Iterables.concat(ImmutableSet.of(newValue), newValue.affectedValues()));
}
removeInstruction(fieldRead);
}
@@ -541,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;
}
@@ -647,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(
@@ -716,7 +754,8 @@
return false;
}
if (invoke.isInvokeMethodWithReceiver()
- && invoke.asInvokeMethodWithReceiver().getReceiver() == eligibleInstance) {
+ && invoke.asInvokeMethodWithReceiver().getReceiver().getAliasedValue()
+ == eligibleInstance) {
return false;
}
if (invoke.isInvokeSuper()) {
@@ -757,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;
}
@@ -798,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.
}
