Allow clinits that create arrays of instances to be postponed
Bug: 142762129
Change-Id: Ifbfa2654f7e4242325ff6486f013501e25ba8432
diff --git a/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java b/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java
index d654c87..96dada1 100644
--- a/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java
+++ b/src/main/java/com/android/tools/r8/ir/analysis/ValueMayDependOnEnvironmentAnalysis.java
@@ -29,6 +29,8 @@
private final IRCode code;
private final DexType context;
+ private final Set<Value> knownNotToDependOnEnvironment = Sets.newIdentityHashSet();
+
public ValueMayDependOnEnvironmentAnalysis(AppView<?> appView, IRCode code) {
this.appView = appView;
this.code = code;
@@ -36,20 +38,57 @@
}
public boolean valueMayDependOnEnvironment(Value value) {
+ return valueMayDependOnEnvironment(value, Sets.newIdentityHashSet());
+ }
+
+ private boolean valueMayDependOnEnvironment(
+ Value value, Set<Value> assumedNotToDependOnEnvironment) {
Value root = value.getAliasedValue();
+ if (assumedNotToDependOnEnvironment.contains(root)) {
+ return false;
+ }
+ if (knownNotToDependOnEnvironment.contains(root)) {
+ return false;
+ }
if (root.isConstant()) {
return false;
}
- if (isConstantArrayThroughoutMethod(root)) {
+ if (isConstantArrayThroughoutMethod(root, assumedNotToDependOnEnvironment)) {
return false;
}
- if (isNewInstanceWithoutEnvironmentDependentFields(root)) {
+ if (isNewInstanceWithoutEnvironmentDependentFields(root, assumedNotToDependOnEnvironment)) {
return false;
}
return true;
}
+ private boolean valueMayNotDependOnEnvironmentAssumingArrayDoesNotDependOnEnvironment(
+ Value value, Value array, Set<Value> assumedNotToDependOnEnvironment) {
+ assert !value.hasAliasedValue();
+ assert !array.hasAliasedValue();
+
+ if (assumedNotToDependOnEnvironment.add(array)) {
+ boolean valueMayDependOnEnvironment =
+ valueMayDependOnEnvironment(value, assumedNotToDependOnEnvironment);
+ boolean changed = assumedNotToDependOnEnvironment.remove(array);
+ assert changed;
+ return !valueMayDependOnEnvironment;
+ }
+ return !valueMayDependOnEnvironment(value, assumedNotToDependOnEnvironment);
+ }
+
+ /**
+ * Used to identify if an array is "constant" in the sense that none of the values written into
+ * the array may depend on the environment.
+ *
+ * <p>Examples include {@code new int[] {1,2,3}} and {@code new Object[]{new Object()}}.
+ */
public boolean isConstantArrayThroughoutMethod(Value value) {
+ return isConstantArrayThroughoutMethod(value, Sets.newIdentityHashSet());
+ }
+
+ private boolean isConstantArrayThroughoutMethod(
+ Value value, Set<Value> assumedNotToDependOnEnvironment) {
Value root = value.getAliasedValue();
if (root.isPhi()) {
// Would need to track the aliases, just give up.
@@ -96,6 +135,7 @@
// Allow array-put and new-array-filled-data instructions that immediately follow the array
// creation.
+ Set<Value> arrayValues = Sets.newIdentityHashSet();
Set<Instruction> consumedInstructions = Sets.newIdentityHashSet();
for (Instruction instruction : definition.getBlock().instructionsAfter(definition)) {
@@ -118,10 +158,19 @@
return false;
}
- if (!arrayPut.value().isConstant()) {
+ // Check if the value being written into the array may depend on the environment.
+ //
+ // When analyzing if the value may depend on the environment, we assume that the current
+ // array does not depend on the environment. Otherwise, we would classify the value as
+ // possibly depending on the environment since it could escape via the array and then
+ // be mutated indirectly.
+ Value rhs = arrayPut.value().getAliasedValue();
+ if (!valueMayNotDependOnEnvironmentAssumingArrayDoesNotDependOnEnvironment(
+ rhs, root, assumedNotToDependOnEnvironment)) {
return false;
}
+ arrayValues.add(rhs);
consumedInstructions.add(arrayPut);
continue;
}
@@ -149,10 +198,21 @@
// Currently, we only allow the array to flow into static-put instructions that are not
// followed by an instruction that may have side effects. Instructions that do not have any
// side effects are ignored because they cannot mutate the array.
- return !valueMayBeMutatedBeforeMethodExit(root, consumedInstructions);
+ if (valueMayBeMutatedBeforeMethodExit(
+ root, assumedNotToDependOnEnvironment, consumedInstructions)) {
+ return false;
+ }
+
+ if (assumedNotToDependOnEnvironment.isEmpty()) {
+ knownNotToDependOnEnvironment.add(root);
+ knownNotToDependOnEnvironment.addAll(arrayValues);
+ }
+
+ return true;
}
- private boolean isNewInstanceWithoutEnvironmentDependentFields(Value value) {
+ private boolean isNewInstanceWithoutEnvironmentDependentFields(
+ Value value, Set<Value> assumedNotToDependOnEnvironment) {
assert !value.hasAliasedValue();
if (value.isPhi() || !value.definition.isNewInstance()) {
@@ -203,16 +263,26 @@
// Check that none of the arguments to the constructor depend on the environment.
for (int i = 1; i < constructorInvoke.arguments().size(); i++) {
Value argument = constructorInvoke.arguments().get(i);
- if (valueMayDependOnEnvironment(argument)) {
+ if (valueMayDependOnEnvironment(argument, assumedNotToDependOnEnvironment)) {
return false;
}
}
// Finally, check that the object does not escape.
- return !valueMayBeMutatedBeforeMethodExit(value, ImmutableSet.of(constructorInvoke));
+ if (valueMayBeMutatedBeforeMethodExit(
+ value, assumedNotToDependOnEnvironment, ImmutableSet.of(constructorInvoke))) {
+ return false;
+ }
+
+ if (assumedNotToDependOnEnvironment.isEmpty()) {
+ knownNotToDependOnEnvironment.add(value);
+ }
+
+ return true;
}
- private boolean valueMayBeMutatedBeforeMethodExit(Value value, Set<Instruction> whitelist) {
+ private boolean valueMayBeMutatedBeforeMethodExit(
+ Value value, Set<Value> assumedNotToDependOnEnvironment, Set<Instruction> whitelist) {
assert !value.hasAliasedValue();
if (value.numberOfPhiUsers() > 0) {
@@ -226,6 +296,14 @@
continue;
}
+ if (user.isArrayPut()) {
+ ArrayPut arrayPut = user.asArrayPut();
+ if (value == arrayPut.value()
+ && !valueMayDependOnEnvironment(arrayPut.array(), assumedNotToDependOnEnvironment)) {
+ continue;
+ }
+ }
+
if (user.isStaticPut()) {
StaticPut staticPut = user.asStaticPut();
if (visited.contains(staticPut)) {
diff --git a/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/ArrayOfObjectsCreationCanBePostponedTest.java b/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/ArrayOfObjectsCreationCanBePostponedTest.java
new file mode 100644
index 0000000..065e069
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/analysis/sideeffect/ArrayOfObjectsCreationCanBePostponedTest.java
@@ -0,0 +1,81 @@
+// 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.ir.analysis.sideeffect;
+
+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.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameters;
+
+@RunWith(Parameterized.class)
+public class ArrayOfObjectsCreationCanBePostponedTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public ArrayOfObjectsCreationCanBePostponedTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(ArrayOfObjectsCreationCanBePostponedTest.class)
+ .addKeepMainRule(TestClass.class)
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("Hello world!");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ ClassSubject classSubject = inspector.clazz(A.class);
+ assertThat(classSubject, isPresent());
+
+ // We should have been able to inline A.inlineable() since A.<clinit>() can safely be postponed.
+ assertThat(classSubject.uniqueMethodWithName("inlineable"), not(isPresent()));
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ A.inlineable();
+ System.out.println(A.values[0].getMessage());
+ }
+ }
+
+ static class A {
+
+ static A[] values = new A[] {new A(" world!")};
+
+ String message;
+
+ A(String message) {
+ this.message = message;
+ }
+
+ static void inlineable() {
+ System.out.print("Hello");
+ }
+
+ String getMessage() {
+ return message;
+ }
+ }
+}
