Version 2.1.48
Cherry-pick: Don't class inline classes that have internal phis of the instance.
CL: https://r8-review.googlesource.com/52482
Cherry-pick: Regression test for class inlining issue.
CL: https://r8-review.googlesource.com/52480
Bug: 160394262
Change-Id: Id9b96186f13bd90baf201a45fd2f44431514645a
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 8024a8a..5f5162a 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.47";
+ public static final String LABEL = "2.1.48";
private Version() {
}
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 3feac87..f80e1d7 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,8 +12,11 @@
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;
@@ -23,6 +26,7 @@
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 {
@@ -73,6 +77,28 @@
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/test/java/com/android/tools/r8/regress/Regress160394262Test.java b/src/test/java/com/android/tools/r8/regress/Regress160394262Test.java
new file mode 100644
index 0000000..0c3206c
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/regress/Regress160394262Test.java
@@ -0,0 +1,176 @@
+// 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 static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+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 com.android.tools.r8.utils.codeinspector.CodeInspector;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class Regress160394262Test extends TestBase {
+
+ static final String EXPECTED = StringUtils.lines("1;2;3");
+
+ private final TestParameters parameters;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().withAllApiLevels().build();
+ }
+
+ public Regress160394262Test(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void testReference() throws Exception {
+ testForRuntime(parameters)
+ .addInnerClasses(Regress160394262Test.class)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .allowAccessModification()
+ .addKeepMainRule(TestClass.class)
+ .addInnerClasses(Regress160394262Test.class)
+ .setMinApi(parameters.getApiLevel())
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutput(EXPECTED)
+ .inspect(this::checkJoinerIsClassInlined);
+ }
+
+ 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.
+ if (parameters.isDexRuntime()) {
+ assertThat(inspector.clazz(Joiner.class), isPresent());
+ assertThat(inspector.clazz(Joiner.class.getTypeName() + "$1"), isPresent());
+ return;
+ }
+ assertThat(inspector.clazz(Joiner.class), not(isPresent()));
+ assertThat(inspector.clazz(Joiner.class.getTypeName() + "$1"), not(isPresent()));
+ }
+
+ static class TestClass {
+
+ public void foo(List<?> items) {
+ System.out.println(Joiner.on(";").skipNulls().join(items));
+ }
+
+ public static void main(String[] args) {
+ new TestClass().foo(Arrays.asList("1", 2, 3l, null));
+ }
+ }
+
+ // Minimized copy of com.google.common.base.Preconditions
+ static class Preconditions {
+
+ public static <T> T checkNotNull(T reference) {
+ if (reference == null) {
+ throw new NullPointerException();
+ } else {
+ return reference;
+ }
+ }
+
+ public static <T> T checkNotNull(T reference, Object errorMessage) {
+ if (reference == null) {
+ throw new NullPointerException(String.valueOf(errorMessage));
+ } else {
+ return reference;
+ }
+ }
+ }
+
+ // Minimized copy of com.google.common.base.Joiner
+ static class Joiner {
+
+ private final String separator;
+
+ public static Joiner on(String separator) {
+ return new Joiner(separator);
+ }
+
+ private Joiner(String separator) {
+ this.separator = Preconditions.checkNotNull(separator);
+ }
+
+ private Joiner(Joiner prototype) {
+ this.separator = prototype.separator;
+ }
+
+ public <A extends Appendable> A appendTo(A appendable, Iterator<?> parts) throws IOException {
+ Preconditions.checkNotNull(appendable);
+ if (parts.hasNext()) {
+ appendable.append(this.toString(parts.next()));
+ while (parts.hasNext()) {
+ appendable.append(this.separator);
+ appendable.append(this.toString(parts.next()));
+ }
+ }
+ return appendable;
+ }
+
+ public final String join(Iterable<?> parts) {
+ return join(parts.iterator());
+ }
+
+ public final String join(Iterator<?> parts) {
+ StringBuilder builder = new StringBuilder();
+ try {
+ appendTo((Appendable) builder, parts);
+ return builder.toString();
+ } catch (IOException e) {
+ throw new AssertionError(e);
+ }
+ }
+
+ public Joiner skipNulls() {
+ return new Joiner(this) {
+ public <A extends Appendable> A appendTo(A appendable, Iterator<?> parts)
+ throws IOException {
+ Preconditions.checkNotNull(appendable, "appendable");
+ Preconditions.checkNotNull(parts, "parts");
+ Object part;
+ while (parts.hasNext()) {
+ part = parts.next();
+ if (part != null) {
+ appendable.append(Joiner.this.toString(part));
+ break;
+ }
+ }
+ while (parts.hasNext()) {
+ part = parts.next();
+ if (part != null) {
+ appendable.append(Joiner.this.separator);
+ appendable.append(Joiner.this.toString(part));
+ }
+ }
+ return appendable;
+ }
+ };
+ }
+
+ CharSequence toString(Object part) {
+ Preconditions.checkNotNull(part);
+ return part instanceof CharSequence ? (CharSequence) part : part.toString();
+ }
+ }
+}
