Version 2.1.51 Cherry-pick: Fix the fix for enum single field values. CL: https://r8-review.googlesource.com/c/r8/+/52528 Cherry-pick: Determine enum single field values using holder. CL: https://r8-review.googlesource.com/c/r8/+/52525 Bug: 160831625 Cherry-pick: Remove trivial phis in class inliner. CL: https://r8-review.googlesource.com/c/r8/+/52507 Bug: 160394262 Change-Id: If3d67b8c3ebcfbc958e54bd705bdb3ec33842cef
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java index e9f227a..5f079ea 100644 --- a/src/main/java/com/android/tools/r8/Version.java +++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@ // This field is accessed from release scripts using simple pattern matching. // Therefore, changing this field could break our release scripts. - public static final String LABEL = "2.1.50"; + public static final String LABEL = "2.1.51"; private Version() { }
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/value/SingleFieldValue.java b/src/main/java/com/android/tools/r8/ir/analysis/value/SingleFieldValue.java index 14dee6f..7bdcc84 100644 --- a/src/main/java/com/android/tools/r8/ir/analysis/value/SingleFieldValue.java +++ b/src/main/java/com/android/tools/r8/ir/analysis/value/SingleFieldValue.java
@@ -109,12 +109,14 @@ @Override public SingleValue rewrittenWithLens(AppView<AppInfoWithLiveness> appView, GraphLense lens) { AbstractValueFactory factory = appView.abstractValueFactory(); - EnumValueInfoMap unboxedEnumInfo = appView.unboxedEnums().getEnumValueInfoMap(field.type); - if (unboxedEnumInfo != null) { - // Return the ordinal of the unboxed enum. - assert unboxedEnumInfo.hasEnumValueInfo(field); - return factory.createSingleNumberValue( - unboxedEnumInfo.getEnumValueInfo(field).convertToInt()); + if (field.holder == field.type) { + EnumValueInfoMap unboxedEnumInfo = appView.unboxedEnums().getEnumValueInfoMap(field.type); + if (unboxedEnumInfo != null) { + // Return the ordinal of the unboxed enum. + assert unboxedEnumInfo.hasEnumValueInfo(field); + return factory.createSingleNumberValue( + unboxedEnumInfo.getEnumValueInfo(field).convertToInt()); + } } return factory.createSingleFieldValue( lens.lookupField(field), getState().rewrittenWithLens(appView, lens));
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 da8c4cc..74d07d2 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.AppView; import com.android.tools.r8.graph.DexClass; @@ -40,6 +41,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; @@ -57,10 +59,10 @@ 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.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; @@ -568,11 +570,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 3e35802..c6e554a 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; @@ -70,6 +71,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 {
diff --git a/src/test/java/com/android/tools/r8/regress/Regress160831625Test.java b/src/test/java/com/android/tools/r8/regress/Regress160831625Test.java new file mode 100644 index 0000000..15c0c75 --- /dev/null +++ b/src/test/java/com/android/tools/r8/regress/Regress160831625Test.java
@@ -0,0 +1,72 @@ +// Copyright (c) 2020, 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.regress; + +import com.android.tools.r8.NeverPropagateValue; +import com.android.tools.r8.TestBase; +import com.android.tools.r8.TestParameters; +import com.android.tools.r8.TestParametersCollection; +import com.android.tools.r8.utils.StringUtils; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +@RunWith(Parameterized.class) +public class Regress160831625Test extends TestBase { + + static final String EXPECTED = StringUtils.lines("A", "0"); + + private final TestParameters parameters; + + @Parameterized.Parameters(name = "{0}") + public static TestParametersCollection data() { + return getTestParameters().withAllRuntimes().withAllApiLevels().build(); + } + + public Regress160831625Test(TestParameters parameters) { + this.parameters = parameters; + } + + @Test + public void testReference() throws Exception { + testForRuntime(parameters) + .addInnerClasses(getClass()) + .run(parameters.getRuntime(), TestClass.class) + .assertSuccessWithOutput(EXPECTED); + } + + @Test + public void testR8() throws Exception { + testForR8(parameters.getBackend()) + .setMinApi(parameters.getApiLevel()) + .addInnerClasses(getClass()) + .enableMemberValuePropagationAnnotations() + .addKeepMainRule(TestClass.class) + .run(parameters.getRuntime(), TestClass.class) + .assertSuccessWithOutput(EXPECTED); + } + + enum MyEnum { + // Ensure that the enum field value is not inlined in the alias in MyClass.B + @NeverPropagateValue + A + } + + static class MyClass { + + private static final MyEnum B = MyEnum.A; + + public static MyEnum getB() { + return B; + } + } + + static class TestClass { + + public static void main(String[] args) { + System.out.println(MyClass.getB().name()); + System.out.println(MyClass.getB().ordinal()); + } + } +}