Remove trivial phis in class inliner.
Bug: 160394262
Bug: 160640028
Bug: 160634549
Change-Id: I610c6a3768a7f8ad2b3ab7c2c0b72a9e7d336ddc
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerCostAnalysis.java b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerCostAnalysis.java
index f80e1d7..3feac87 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerCostAnalysis.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/classinliner/ClassInlinerCostAnalysis.java
@@ -12,11 +12,8 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexProgramClass;
import com.android.tools.r8.graph.ProgramMethod;
-import com.android.tools.r8.ir.analysis.type.TypeElement;
-import com.android.tools.r8.ir.code.AssumeAndCheckCastAliasedValueConfiguration;
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.InvokeMethod;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.optimize.inliner.InliningIRProvider;
@@ -26,7 +23,6 @@
import java.util.Map;
import java.util.Set;
-// TODO(b/160634549): Rename or refactor this to reflect its non-cost related analysis.
/** Analysis that estimates the cost of class inlining an object allocation. */
class ClassInlinerCostAnalysis {
@@ -77,28 +73,6 @@
continue;
}
IRCode inliningIR = inliningIRProvider.getAndCacheInliningIR(invoke, inlinee);
-
- // If the instance is part of a phi, then inlining will invalidate the inliner assumptions.
- // TODO(b/160634549): This is not a budget miss but a hard requirement.
- InstructionIterator iterator = inliningIR.entryBlock().iterator();
- while (iterator.hasNext()) {
- Instruction next = iterator.next();
- if (!next.isArgument()) {
- break;
- }
- Value argumentValue = next.outValue();
- TypeElement argumentType = argumentValue.getType();
- if (argumentType.isClassType()
- && argumentType.asClassType().getClassType() == eligibleClass.type) {
- assert argumentValue.uniqueUsers().stream()
- .noneMatch(
- AssumeAndCheckCastAliasedValueConfiguration.getInstance()::isIntroducingAnAlias);
- if (argumentValue.hasPhiUsers()) {
- return true;
- }
- }
- }
-
int increment =
inlinee.getDefinition().getCode().estimatedSizeForInlining()
- estimateNumberOfNonMaterializingInstructions(invoke, inliningIR);
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 4e52743..72be050 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
@@ -8,6 +8,7 @@
import static com.android.tools.r8.graph.DexProgramClass.asProgramClassOrNull;
import static com.google.common.base.Predicates.alwaysFalse;
+import com.android.tools.r8.errors.InternalCompilerError;
import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.AccessControl;
import com.android.tools.r8.graph.AppView;
@@ -41,6 +42,7 @@
import com.android.tools.r8.ir.code.InvokeDirect;
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
+import com.android.tools.r8.ir.code.Phi;
import com.android.tools.r8.ir.code.StaticGet;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.conversion.MethodProcessor;
@@ -58,11 +60,11 @@
import com.android.tools.r8.ir.optimize.inliner.NopWhyAreYouNotInliningReporter;
import com.android.tools.r8.kotlin.KotlinClassLevelInfo;
import com.android.tools.r8.shaking.AppInfoWithLiveness;
-import com.android.tools.r8.utils.ListUtils;
import com.android.tools.r8.utils.OptionalBool;
import com.android.tools.r8.utils.Pair;
import com.android.tools.r8.utils.StringUtils;
import com.android.tools.r8.utils.Timing;
+import com.android.tools.r8.utils.WorkList;
import com.android.tools.r8.utils.collections.ProgramMethodSet;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
@@ -581,11 +583,35 @@
while (!currentUsers.isEmpty()) {
Set<Instruction> indirectOutValueUsers = Sets.newIdentityHashSet();
for (Instruction instruction : currentUsers) {
- if (instruction.isAssume() || instruction.isCheckCast()) {
- Value src = ListUtils.first(instruction.inValues());
+ if (aliasesThroughAssumeAndCheckCasts.isIntroducingAnAlias(instruction)) {
+ Value src = aliasesThroughAssumeAndCheckCasts.getAliasForOutValue(instruction);
Value dest = instruction.outValue();
- indirectOutValueUsers.addAll(dest.uniqueUsers());
+ if (dest.hasPhiUsers()) {
+ // It is possible that a trivial phi is constructed upon IR building for the eligible
+ // value. It must actually be trivial so verify that it is indeed trivial and replace
+ // all of the phis involved with the value.
+ WorkList<Phi> worklist = WorkList.newIdentityWorkList(dest.uniquePhiUsers());
+ while (worklist.hasNext()) {
+ Phi phi = worklist.next();
+ for (Value operand : phi.getOperands()) {
+ operand = operand.getAliasedValue(aliasesThroughAssumeAndCheckCasts);
+ if (operand.isPhi()) {
+ worklist.addIfNotSeen(operand.asPhi());
+ } else if (src != operand) {
+ throw new InternalCompilerError(
+ "Unexpected non-trivial phi in method eligible for class inlining");
+ }
+ }
+ }
+ // The only value flowing into any of the phis is src, so replace all phis by src.
+ for (Phi phi : worklist.getSeenSet()) {
+ indirectOutValueUsers.addAll(phi.uniqueUsers());
+ phi.replaceUsers(src);
+ phi.removeDeadPhi();
+ }
+ }
assert !dest.hasPhiUsers();
+ indirectOutValueUsers.addAll(dest.uniqueUsers());
dest.replaceUsers(src);
removeInstruction(instruction);
}
diff --git a/src/main/java/com/android/tools/r8/utils/WorkList.java b/src/main/java/com/android/tools/r8/utils/WorkList.java
index d8bd420..092a4fc 100644
--- a/src/main/java/com/android/tools/r8/utils/WorkList.java
+++ b/src/main/java/com/android/tools/r8/utils/WorkList.java
@@ -6,6 +6,7 @@
import com.google.common.collect.Sets;
import java.util.ArrayDeque;
+import java.util.Collections;
import java.util.Deque;
import java.util.HashSet;
import java.util.Set;
@@ -78,6 +79,10 @@
return workingList.removeFirst();
}
+ public Set<T> getSeenSet() {
+ return Collections.unmodifiableSet(seen);
+ }
+
public enum EqualityTest {
HASH,
IDENTITY
diff --git a/src/test/java/com/android/tools/r8/regress/Regress160394262Test.java b/src/test/java/com/android/tools/r8/regress/Regress160394262Test.java
index 0c3206c..383a7ef 100644
--- a/src/test/java/com/android/tools/r8/regress/Regress160394262Test.java
+++ b/src/test/java/com/android/tools/r8/regress/Regress160394262Test.java
@@ -57,15 +57,13 @@
}
private void checkJoinerIsClassInlined(CodeInspector inspector) {
- // TODO(b/160640028): When compiling to DEX a trivial phi remains in the inline code preventing
- // class inlining of Joiner and the anonymous Joiner subclass.
+ assertThat(inspector.clazz(Joiner.class.getTypeName() + "$1"), not(isPresent()));
+ // TODO(b/160640028): When compiling to DEX the outer Joiner class is not inlined.
if (parameters.isDexRuntime()) {
assertThat(inspector.clazz(Joiner.class), isPresent());
- assertThat(inspector.clazz(Joiner.class.getTypeName() + "$1"), isPresent());
- return;
+ } else {
+ assertThat(inspector.clazz(Joiner.class), not(isPresent()));
}
- assertThat(inspector.clazz(Joiner.class), not(isPresent()));
- assertThat(inspector.clazz(Joiner.class.getTypeName() + "$1"), not(isPresent()));
}
static class TestClass {
