Replace instance-get instructions by requireNonNull() during value propagation
Change-Id: Ia2307f6fcb480c9b61eac944c3bd90380c66a455
Bug: 149650435
diff --git a/src/main/java/com/android/tools/r8/ir/code/FieldInstruction.java b/src/main/java/com/android/tools/r8/ir/code/FieldInstruction.java
index c8d6abc..69c6281 100644
--- a/src/main/java/com/android/tools/r8/ir/code/FieldInstruction.java
+++ b/src/main/java/com/android/tools/r8/ir/code/FieldInstruction.java
@@ -27,6 +27,15 @@
public abstract class FieldInstruction extends Instruction {
+ public enum Assumption {
+ NONE,
+ RECEIVER_NOT_NULL;
+
+ boolean canAssumeReceiverIsNotNull() {
+ return this == RECEIVER_NOT_NULL;
+ }
+ }
+
private final DexField field;
protected FieldInstruction(DexField field, Value dest, Value value) {
@@ -61,6 +70,11 @@
@Override
public AbstractError instructionInstanceCanThrow(AppView<?> appView, DexType context) {
+ return instructionInstanceCanThrow(appView, context, Assumption.NONE);
+ }
+
+ public AbstractError instructionInstanceCanThrow(
+ AppView<?> appView, DexType context, Assumption assumption) {
DexEncodedField resolvedField;
if (appView.enableWholeProgramOptimizations()) {
// TODO(b/123857022): Should be possible to use definitionFor().
@@ -106,9 +120,11 @@
// TODO(b/137168535): Without non-null tracking, only locally created receiver is allowed in D8.
// * NullPointerException (null receiver).
if (isInstanceGet() || isInstancePut()) {
- Value receiver = inValues.get(0);
- if (receiver.isAlwaysNull(appView) || receiver.typeLattice.isNullable()) {
- return AbstractError.specific(appView.dexItemFactory().npeType);
+ if (!assumption.canAssumeReceiverIsNotNull()) {
+ Value receiver = inValues.get(0);
+ if (receiver.isAlwaysNull(appView) || receiver.typeLattice.isNullable()) {
+ return AbstractError.specific(appView.dexItemFactory().npeType);
+ }
}
}
// For D8, reaching here means the field is in the same context, hence the class is guaranteed
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java b/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
index 3155fa1..2ca1b2a 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InstanceGet.java
@@ -116,7 +116,12 @@
@Override
public boolean instructionMayHaveSideEffects(AppView<?> appView, DexType context) {
- return instructionInstanceCanThrow(appView, context).isThrowing();
+ return instructionMayHaveSideEffects(appView, context, Assumption.NONE);
+ }
+
+ public boolean instructionMayHaveSideEffects(
+ AppView<?> appView, DexType context, Assumption assumption) {
+ return instructionInstanceCanThrow(appView, context, assumption).isThrowing();
}
@Override
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 722625f..a28181f 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
@@ -20,9 +20,12 @@
import com.android.tools.r8.ir.code.FieldInstruction;
import com.android.tools.r8.ir.code.IRCode;
import com.android.tools.r8.ir.code.IRMetadata;
+import com.android.tools.r8.ir.code.InstanceGet;
import com.android.tools.r8.ir.code.Instruction;
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.InvokeMethod;
+import com.android.tools.r8.ir.code.InvokeStatic;
+import com.android.tools.r8.ir.code.InvokeVirtual;
import com.android.tools.r8.ir.code.Value;
import com.android.tools.r8.ir.optimize.info.OptimizationFeedback;
import com.android.tools.r8.ir.optimize.info.OptimizationFeedbackSimple;
@@ -31,6 +34,7 @@
import com.android.tools.r8.shaking.ProguardMemberRuleReturnValue;
import com.android.tools.r8.utils.Reporter;
import com.android.tools.r8.utils.StringDiagnostic;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Sets;
import java.util.ListIterator;
import java.util.Set;
@@ -336,12 +340,24 @@
target.valueAsConstInstruction(code, current.outValue().getLocalInfo(), appView);
if (replacement != null) {
affectedValues.addAll(current.outValue().affectedValues());
- if (current.instructionMayHaveSideEffects(appView, code.method.method.holder)) {
- // To preserve class initialization/NPE side effects, original field-get remains as-is,
- // but its value is replaced with constant.
- replacement.setPosition(current.getPosition());
+ DexType context = code.method.method.holder;
+ if (current.instructionMayHaveSideEffects(appView, context)) {
+ // All usages are replaced by the replacement value.
current.outValue().replaceUsers(replacement.outValue());
- if (current.getBlock().hasCatchHandlers()) {
+
+ // To preserve side effects, original field-get is replaced by an explicit null-check, if
+ // the field-get instruction may only fail with an NPE, or the field-get remains as-is.
+ Instruction currentOrNullCheck;
+ if (current.isInstanceGet()) {
+ currentOrNullCheck =
+ replaceInstanceGetByNullCheckIfPossible(current.asInstanceGet(), iterator, context);
+ } else {
+ currentOrNullCheck = current;
+ }
+
+ // Insert the definition of the replacement.
+ replacement.setPosition(currentOrNullCheck.getPosition());
+ if (currentOrNullCheck.getBlock().hasCatchHandlers()) {
iterator.split(code, blocks).listIterator(code).add(replacement);
} else {
iterator.add(replacement);
@@ -353,6 +369,26 @@
}
}
+ private Instruction replaceInstanceGetByNullCheckIfPossible(
+ InstanceGet instruction, InstructionListIterator iterator, DexType context) {
+ assert !instruction.outValue().hasAnyUsers();
+ if (instruction.instructionMayHaveSideEffects(
+ appView, context, FieldInstruction.Assumption.RECEIVER_NOT_NULL)) {
+ return instruction;
+ }
+ Value receiver = instruction.object();
+ InvokeMethod replacement;
+ if (appView.options().canUseRequireNonNull()) {
+ DexMethod requireNonNullMethod = appView.dexItemFactory().objectsMethods.requireNonNull;
+ replacement = new InvokeStatic(requireNonNullMethod, null, ImmutableList.of(receiver));
+ } else {
+ DexMethod getClassMethod = appView.dexItemFactory().objectMethods.getClass;
+ replacement = new InvokeVirtual(getClassMethod, null, ImmutableList.of(receiver));
+ }
+ iterator.replaceCurrentInstruction(replacement);
+ return replacement;
+ }
+
/**
* Replace invoke targets and field accesses with constant values where possible.
*
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldValuePropagationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldValuePropagationTest.java
index a74ac5f..2c8f4b3 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldValuePropagationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/membervaluepropagation/InstanceFieldValuePropagationTest.java
@@ -75,12 +75,10 @@
MethodSubject testMaybeNullMethodSubject =
testClassSubject.uniqueMethodWithName("testMaybeNull");
assertThat(testMaybeNullMethodSubject, isPresent());
- // TODO(b/125282093): Should synthesize a null-check and still propagate the field values even
- // when the receiver is nullable.
assertTrue(
testMaybeNullMethodSubject
.streamInstructions()
- .anyMatch(InstructionSubject::isInstanceGet));
+ .noneMatch(InstructionSubject::isInstanceGet));
// TODO(b/125282093): Should be able to remove the new-instance instruction since the instance
// ends up being unused.
assertTrue(
diff --git a/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java b/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java
index dfd14da..397c054 100644
--- a/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java
+++ b/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java
@@ -17,6 +17,7 @@
import com.android.tools.r8.jasmin.JasminBuilder.ClassBuilder;
import com.android.tools.r8.jasmin.JasminTestBase;
import com.android.tools.r8.naming.MemberNaming.MethodSignature;
+import com.android.tools.r8.utils.ThrowingConsumer;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
import com.android.tools.r8.utils.codeinspector.FieldSubject;
@@ -37,7 +38,7 @@
@Parameterized.Parameters(name = "{0}")
public static TestParametersCollection data() {
- return getTestParameters().withAllRuntimes().build();
+ return getTestParameters().withAllRuntimesAndApiLevels().build();
}
public FieldReadsJasminTest(TestParameters parameters) {
@@ -99,7 +100,7 @@
if (parameters.isDexRuntime()) {
testForD8()
.addProgramClassFileData(classes)
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
.inspect(inspector -> ensureNoFieldsRead(inspector, clazz.name, false));
}
@@ -107,7 +108,7 @@
testForR8(parameters.getBackend())
.addProgramClassFileData(classes)
.addKeepRules("-keep class * { <methods>; }")
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
.inspect(inspector -> ensureNoFieldsRead(inspector, clazz.name, true));
}
@@ -167,20 +168,21 @@
testForR8(parameters.getBackend())
.addProgramClassFileData(app.buildClasses())
.addKeepRules("-keep class * { <methods>; }")
- .setMinApi(parameters.getRuntime())
+ .setMinApi(parameters.getApiLevel())
.compile()
- .inspect(inspector -> {
- FieldSubject fld = inspector.clazz(fieldHolder.name).uniqueFieldWithName(fieldName);
- assertThat(fld, isRenamed());
+ .inspect(
+ inspector -> {
+ FieldSubject fld = inspector.clazz(fieldHolder.name).uniqueFieldWithName(fieldName);
+ assertThat(fld, isRenamed());
- ClassSubject classSubject = inspector.clazz(clazz.name);
- assertThat(classSubject, isPresent());
- MethodSubject methodSubject = classSubject.uniqueMethodWithName(method.name);
- assertThat(methodSubject, isPresent());
- Iterator<InstructionSubject> it =
- methodSubject.iterateInstructions(InstructionSubject::isFieldAccess);
- assertFalse(it.hasNext());
- });
+ ClassSubject classSubject = inspector.clazz(clazz.name);
+ assertThat(classSubject, isPresent());
+ MethodSubject methodSubject = classSubject.uniqueMethodWithName(method.name);
+ assertThat(methodSubject, isPresent());
+ Iterator<InstructionSubject> it =
+ methodSubject.iterateInstructions(InstructionSubject::isFieldAccess);
+ assertFalse(it.hasNext());
+ });
}
@Test
@@ -205,7 +207,27 @@
" invokestatic Empty/foo(L" + CLS + ";)V",
" return");
- ensureFieldExistsAndReadOnlyOnce(builder, empty, foo, empty, "aField");
+ inspect(
+ builder,
+ inspector ->
+ ensureFieldExistsAndReadOnlyOnce(
+ inspector, empty.name, foo.name, empty, "aField", false),
+ inspector -> {
+ ClassSubject emptyClassSubject = inspector.clazz(CLS);
+ assertThat(emptyClassSubject, isPresent());
+ assertTrue(emptyClassSubject.allFields().isEmpty());
+
+ MethodSubject fooMethodSubject = emptyClassSubject.uniqueMethodWithName("foo");
+ assertThat(fooMethodSubject, isPresent());
+ assertTrue(
+ fooMethodSubject
+ .streamInstructions()
+ .filter(InstructionSubject::isInvoke)
+ .anyMatch(
+ invoke ->
+ invoke.getMethod().toSourceString().contains("requireNonNull")
+ || invoke.getMethod().toSourceString().contains("getClass")));
+ });
}
@Test
@@ -275,6 +297,29 @@
ensureFieldExistsAndReadOnlyOnce(builder, main, mainMethod, main, "sField");
}
+ private void inspect(
+ JasminBuilder app,
+ ThrowingConsumer<CodeInspector, RuntimeException> d8Inspector,
+ ThrowingConsumer<CodeInspector, RuntimeException> r8Inspector)
+ throws Exception {
+ List<byte[]> classes = app.buildClasses();
+
+ if (parameters.isDexRuntime()) {
+ testForD8()
+ .addProgramClassFileData(classes)
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(d8Inspector);
+ }
+
+ testForR8(parameters.getBackend())
+ .addProgramClassFileData(classes)
+ .addKeepRules("-keep class * { <methods>; }")
+ .setMinApi(parameters.getApiLevel())
+ .compile()
+ .inspect(r8Inspector);
+ }
+
private void ensureFieldExistsAndReadOnlyOnce(
JasminBuilder app,
ClassBuilder clazz,
@@ -282,24 +327,14 @@
ClassBuilder fieldHolder,
String fieldName)
throws Exception {
- List<byte[]> classes = app.buildClasses();
-
- if (parameters.isDexRuntime()) {
- testForD8()
- .addProgramClassFileData(classes)
- .setMinApi(parameters.getRuntime())
- .compile()
- .inspect(inspector -> ensureFieldExistsAndReadOnlyOnce(
- inspector, clazz.name, method.name, fieldHolder, fieldName, false));
- }
-
- testForR8(parameters.getBackend())
- .addProgramClassFileData(classes)
- .addKeepRules("-keep class * { <methods>; }")
- .setMinApi(parameters.getRuntime())
- .compile()
- .inspect(inspector -> ensureFieldExistsAndReadOnlyOnce(
- inspector, clazz.name, method.name, fieldHolder, fieldName, true));
+ inspect(
+ app,
+ inspector ->
+ ensureFieldExistsAndReadOnlyOnce(
+ inspector, clazz.name, method.name, fieldHolder, fieldName, false),
+ inspector ->
+ ensureFieldExistsAndReadOnlyOnce(
+ inspector, clazz.name, method.name, fieldHolder, fieldName, true));
}
private void ensureFieldExistsAndReadOnlyOnce(
