Member value propagation in presence of class initialization side effects
Bug: 137908034
Change-Id: I55560a88861dd1f5fe4292bfded9c6ce1f4d9218
diff --git a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
index 383f816..05feecc 100644
--- a/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
+++ b/src/main/java/com/android/tools/r8/graph/DexEncodedField.java
@@ -7,7 +7,6 @@
import com.android.tools.r8.dex.MixedSectionCollection;
import com.android.tools.r8.ir.code.ConstInstruction;
import com.android.tools.r8.ir.code.IRCode;
-import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.optimize.info.DefaultFieldOptimizationInfo;
import com.android.tools.r8.ir.optimize.info.FieldOptimizationInfo;
import com.android.tools.r8.ir.optimize.info.MutableFieldOptimizationInfo;
@@ -150,36 +149,12 @@
* <p>NOTE: It is the responsibility of the caller to check if this field is pinned or not.
*/
public ConstInstruction valueAsConstInstruction(
- IRCode code, Value dest, AppView<AppInfoWithLiveness> appView) {
- // If it is a static field, we can only propagate the value if class initialization does not
- // have side effects.
- if (isStatic()) {
- DexClass clazz = appView.definitionFor(field.holder);
- if (clazz == null) {
- return null;
- }
- DexType context = code.method.method.holder;
- if (clazz.classInitializationMayHaveSideEffects(
- appView,
- // Types that are a super type of the current context are guaranteed to be initialized
- // already.
- type -> appView.isSubtype(context, type).isTrue())) {
- // Ignore class initialization side-effects for dead proto extension fields to ensure that
- // we force replace these field reads by null.
- boolean ignore =
- appView.withGeneratedExtensionRegistryShrinker(
- shrinker -> shrinker.isDeadProtoExtensionField(field), false);
- if (!ignore) {
- return null;
- }
- }
- }
-
+ IRCode code, DebugLocalInfo local, AppView<AppInfoWithLiveness> appView) {
boolean isWritten = appView.appInfo().isFieldWrittenByFieldPutInstruction(this);
if (!isWritten) {
// Since the field is not written, we can simply return the default value for the type.
DexValue value = isStatic() ? getStaticValue() : DexValue.defaultForType(field.type);
- return value.asConstInstruction(code, dest, appView.options());
+ return value.asConstInstruction(appView, code, local);
}
// The only way to figure out whether the DexValue contains the final value is ensure the value
@@ -191,13 +166,38 @@
}
DexValue staticValue = getStaticValue();
if (!staticValue.isDefault(field.type)) {
- return staticValue.asConstInstruction(code, dest, appView.options());
+ return staticValue.asConstInstruction(appView, code, local);
}
}
return null;
}
+ public boolean mayTriggerClassInitializationSideEffects(
+ AppView<AppInfoWithLiveness> appView, DexType context) {
+ // Only static field matters when it comes to class initialization side effects.
+ if (!isStatic()) {
+ return false;
+ }
+ DexClass clazz = appView.definitionFor(field.holder);
+ if (clazz == null) {
+ return true;
+ }
+ if (clazz.classInitializationMayHaveSideEffects(
+ appView,
+ // Types that are a super type of the current context are guaranteed to be initialized
+ // already.
+ type -> appView.isSubtype(context, type).isTrue())) {
+ // Ignore class initialization side-effects for dead proto extension fields to ensure that
+ // we force replace these field reads by null.
+ boolean ignore =
+ appView.withGeneratedExtensionRegistryShrinker(
+ shrinker -> shrinker.isDeadProtoExtensionField(field), false);
+ return !ignore;
+ }
+ return false;
+ }
+
public DexEncodedField toTypeSubstitutedField(DexField field) {
if (this.field == field) {
return this;
diff --git a/src/main/java/com/android/tools/r8/graph/DexValue.java b/src/main/java/com/android/tools/r8/graph/DexValue.java
index f9b12d3..f48a6a6 100644
--- a/src/main/java/com/android/tools/r8/graph/DexValue.java
+++ b/src/main/java/com/android/tools/r8/graph/DexValue.java
@@ -3,21 +3,23 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.graph;
+import static com.android.tools.r8.ir.analysis.type.Nullability.definitelyNotNull;
+
import com.android.tools.r8.dex.DexOutputBuffer;
import com.android.tools.r8.dex.FileWriter;
import com.android.tools.r8.dex.IndexedItemCollection;
import com.android.tools.r8.dex.MixedSectionCollection;
import com.android.tools.r8.errors.Unreachable;
+import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
import com.android.tools.r8.ir.code.BasicBlock.ThrowingInfo;
import com.android.tools.r8.ir.code.ConstInstruction;
-import com.android.tools.r8.ir.code.ConstNumber;
import com.android.tools.r8.ir.code.ConstString;
import com.android.tools.r8.ir.code.DexItemBasedConstString;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.naming.dexitembasedstring.NameComputationInfo;
+import com.android.tools.r8.utils.BooleanUtils;
import com.android.tools.r8.utils.EncodedValueUtils;
-import com.android.tools.r8.utils.InternalOptions;
import java.util.Arrays;
import org.objectweb.asm.Handle;
import org.objectweb.asm.Type;
@@ -139,7 +141,8 @@
public abstract Object getBoxedValue();
/** Returns an instruction that can be used to materialize this {@link DexValue} (or null). */
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
return null;
}
@@ -214,7 +217,8 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
return null;
}
}
@@ -301,8 +305,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- return new ConstNumber(dest, value);
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createIntConstant(value, local);
}
}
@@ -357,8 +362,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- return new ConstNumber(dest, value);
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createIntConstant(value, local);
}
}
@@ -417,8 +423,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- return new ConstNumber(dest, value);
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createIntConstant(value, local);
}
}
@@ -473,8 +480,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- return new ConstNumber(dest, value);
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createIntConstant(value, local);
}
}
@@ -529,8 +537,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- return new ConstNumber(dest, value);
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createLongConstant(value, local);
}
}
@@ -733,9 +742,12 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ TypeLatticeElement type = TypeLatticeElement.stringClassType(appView, definitelyNotNull());
+ Value outValue = code.createValue(type, local);
ConstString instruction =
- new ConstString(dest, value, ThrowingInfo.defaultForConstString(options));
+ new ConstString(outValue, value, ThrowingInfo.defaultForConstString(appView.options()));
if (!instruction.instructionInstanceCanThrow()) {
return instruction;
}
@@ -773,10 +785,16 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ TypeLatticeElement type = TypeLatticeElement.stringClassType(appView, definitelyNotNull());
+ Value outValue = code.createValue(type, local);
DexItemBasedConstString instruction =
new DexItemBasedConstString(
- dest, value, nameComputationInfo, ThrowingInfo.defaultForConstString(options));
+ outValue,
+ value,
+ nameComputationInfo,
+ ThrowingInfo.defaultForConstString(appView.options()));
// DexItemBasedConstString cannot throw.
assert !instruction.instructionInstanceCanThrow();
return instruction;
@@ -1055,11 +1073,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- if (dest.getTypeLattice().isNullType()) {
- return new ConstNumber(dest, 0);
- }
- return code.createConstNull(dest.getLocalInfo());
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createConstNull(local);
}
}
@@ -1118,8 +1134,9 @@
}
@Override
- public ConstInstruction asConstInstruction(IRCode code, Value dest, InternalOptions options) {
- return new ConstNumber(dest, value ? 1 : 0);
+ public ConstInstruction asConstInstruction(
+ AppView<? extends AppInfoWithSubtyping> appView, IRCode code, DebugLocalInfo local) {
+ return code.createIntConstant(BooleanUtils.intValue(value), local);
}
}
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 ce02bf6..f0c062b 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,15 +1008,29 @@
return createValue(typeLattice, null);
}
- public Phi createPhi(BasicBlock block, TypeLatticeElement type) {
- return new Phi(valueNumberGenerator.next(), block, type, null, RegisterReadType.NORMAL);
+ public ConstNumber createDoubleConstant(long value, DebugLocalInfo local) {
+ Value out = createValue(TypeLatticeElement.DOUBLE, local);
+ return new ConstNumber(out, value);
}
public ConstNumber createIntConstant(int value) {
- Value out = createValue(TypeLatticeElement.INT);
+ return createIntConstant(value, null);
+ }
+
+ public ConstNumber createIntConstant(int value, DebugLocalInfo local) {
+ Value out = createValue(TypeLatticeElement.INT, local);
return new ConstNumber(out, value);
}
+ public ConstNumber createLongConstant(long value, DebugLocalInfo local) {
+ Value out = createValue(TypeLatticeElement.LONG, local);
+ return new ConstNumber(out, value);
+ }
+
+ public Phi createPhi(BasicBlock block, TypeLatticeElement type) {
+ return new Phi(valueNumberGenerator.next(), block, type, null, RegisterReadType.NORMAL);
+ }
+
public final int getHighestBlockNumber() {
return blocks.stream().max(Comparator.comparingInt(BasicBlock::getNumber)).get().getNumber();
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java b/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
index 787cf33..62d5bd8 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/MemberValuePropagation.java
@@ -151,8 +151,8 @@
return null;
}
- Value value = code.createValue(typeLattice, instruction.getLocalInfo());
- ConstInstruction replacement = staticField.valueAsConstInstruction(code, value, appView);
+ ConstInstruction replacement =
+ staticField.valueAsConstInstruction(code, instruction.getLocalInfo(), appView);
if (replacement == null) {
reporter.warning(
new StringDiagnostic(
@@ -369,10 +369,23 @@
return;
}
- ConstInstruction replacement = target.valueAsConstInstruction(code, current.dest(), appView);
+ ConstInstruction replacement =
+ target.valueAsConstInstruction(code, current.outValue().getLocalInfo(), appView);
if (replacement != null) {
affectedValues.addAll(current.outValue().affectedValues());
- iterator.replaceCurrentInstruction(replacement);
+ if (target.mayTriggerClassInitializationSideEffects(appView, code.method.method.holder)) {
+ // To preserve class initialization side effects, original static-get remains as-is, but its
+ // value is replaced with constant.
+ replacement.setPosition(current.getPosition());
+ current.outValue().replaceUsers(replacement.outValue());
+ if (current.getBlock().hasCatchHandlers()) {
+ iterator.split(code, blocks).listIterator(code).add(replacement);
+ } else {
+ iterator.add(replacement);
+ }
+ } else {
+ iterator.replaceCurrentInstruction(replacement);
+ }
if (replacement.isDexItemBasedConstString()) {
code.method.getMutableOptimizationInfo().markUseIdentifierNameString();
}
@@ -398,7 +411,8 @@
}
// Check if a this value is known const.
- ConstInstruction replacement = target.valueAsConstInstruction(code, current.dest(), appView);
+ ConstInstruction replacement =
+ target.valueAsConstInstruction(code, current.outValue().getLocalInfo(), appView);
if (replacement != null) {
affectedValues.add(replacement.outValue());
iterator.replaceCurrentInstruction(replacement);
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/MemberValuePropagationWithClassInitializationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/MemberValuePropagationWithClassInitializationTest.java
new file mode 100644
index 0000000..9c32e92
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/MemberValuePropagationWithClassInitializationTest.java
@@ -0,0 +1,133 @@
+// 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.optimize.membervaluepropagation;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.NeverInline;
+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 com.android.tools.r8.utils.codeinspector.FieldSubject;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+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 MemberValuePropagationWithClassInitializationTest extends TestBase {
+
+ private final TestParameters parameters;
+
+ @Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
+ }
+
+ public MemberValuePropagationWithClassInitializationTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ @Test
+ public void test() throws Exception {
+ testForR8(parameters.getBackend())
+ .addInnerClasses(MemberValuePropagationWithClassInitializationTest.class)
+ .addKeepMainRule(TestClass.class)
+ .enableInliningAnnotations()
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(this::inspect)
+ .run(parameters.getRuntime(), TestClass.class)
+ .assertSuccessWithOutputLines("A!", "B!");
+ }
+
+ private void inspect(CodeInspector inspector) {
+ // A.field is present.
+ ClassSubject aClassSubject = inspector.clazz(A.class);
+ assertThat(aClassSubject, isPresent());
+
+ FieldSubject fieldSubject = aClassSubject.uniqueFieldWithName("field");
+ assertThat(fieldSubject, isPresent());
+
+ // B.method() is present.
+ ClassSubject bClassSubject = inspector.clazz(B.class);
+ assertThat(bClassSubject, isPresent());
+
+ MethodSubject methodSubject = bClassSubject.uniqueMethodWithName("method");
+ assertThat(methodSubject, isPresent());
+
+ // TestClass.missingFieldValuePropagation() and TestClass.missingMethodValuePropagation() are
+ // absent.
+ ClassSubject testClassSubject = inspector.clazz(TestClass.class);
+ assertThat(testClassSubject, isPresent());
+ assertThat(
+ testClassSubject.uniqueMethodWithName("missingFieldValuePropagation"), not(isPresent()));
+ assertThat(
+ testClassSubject.uniqueMethodWithName("missingMethodValuePropagation"), not(isPresent()));
+
+ // TestClass.main() still accesses A.field and invokes B.method().
+ MethodSubject mainMethodSubject = testClassSubject.mainMethod();
+ assertThat(mainMethodSubject, isPresent());
+ assertTrue(
+ mainMethodSubject
+ .streamInstructions()
+ .filter(InstructionSubject::isStaticGet)
+ .anyMatch(x -> x.getField() == fieldSubject.getField().field));
+ assertTrue(
+ mainMethodSubject
+ .streamInstructions()
+ .filter(InstructionSubject::isInvokeStatic)
+ .anyMatch(x -> x.getMethod() == methodSubject.getMethod().method));
+ }
+
+ static class TestClass {
+
+ public static void main(String[] args) {
+ if (A.field) {
+ missingFieldValuePropagation();
+ }
+ if (B.method()) {
+ missingMethodValuePropagation();
+ }
+ }
+
+ @NeverInline
+ static void missingFieldValuePropagation() {
+ System.out.println("Missing field value propagation!");
+ }
+
+ @NeverInline
+ static void missingMethodValuePropagation() {
+ System.out.println("Missing method value propagation!");
+ }
+ }
+
+ static class A {
+
+ static boolean field = false;
+
+ static {
+ System.out.println("A!");
+ }
+ }
+
+ static class B {
+
+ static {
+ System.out.println("B!");
+ }
+
+ static boolean method() {
+ return false;
+ }
+ }
+}
