Relax dead condition of instance-put.
by implementing instructionMayHaveSideEffects.
This is a variant of https://r8-review.googlesource.com/c/r8/+/37480
Bug: 129530569
Change-Id: I5ec4e83342de56ee4a03977a39d91d31d1b042bb
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstancePut.java b/src/main/java/com/android/tools/r8/ir/code/InstancePut.java
index 22fddb0..9cea0c0 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InstancePut.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InstancePut.java
@@ -16,6 +16,7 @@
import com.android.tools.r8.dex.Constants;
import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexType;
@@ -27,6 +28,7 @@
import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
import com.android.tools.r8.ir.optimize.InliningConstraints;
import com.android.tools.r8.ir.regalloc.RegisterAllocator;
+import com.android.tools.r8.shaking.AppInfoWithLiveness;
import java.util.Arrays;
import org.objectweb.asm.Opcodes;
@@ -96,6 +98,43 @@
}
@Override
+ public boolean instructionMayHaveSideEffects(AppView<?> appView, DexType context) {
+ if (appView.appInfo().hasLiveness()) {
+ AppInfoWithLiveness appInfoWithLiveness = appView.appInfo().withLiveness();
+
+ if (instructionInstanceCanThrow(appView, context).isThrowing()) {
+ return true;
+ }
+
+ DexEncodedField resolveField = appInfoWithLiveness.resolveField(getField());
+ assert resolveField != null : "NoSuchFieldError (resolution failure) should be caught.";
+ if (appInfoWithLiveness.isFieldRead(resolveField.field)) {
+ return true;
+ }
+
+ return false;
+ }
+
+ // In D8, we always have to assume that the field can be read, and thus have side effects.
+ assert instructionInstanceCanThrow(appView, context).isThrowing();
+ return true;
+ }
+
+ @Override
+ public boolean canBeDeadCode(AppView<?> appView, IRCode code) {
+ // instance-put can be dead as long as it cannot have any of the following:
+ // * NoSuchFieldError (resolution failure)
+ // * IncompatibleClassChangeError (static-* instruction for instance fields)
+ // * IllegalAccessError (not visible from the access context)
+ // * NullPointerException (null receiver)
+ // * not read at all
+ boolean haveSideEffects = instructionMayHaveSideEffects(appView, code.method.method.holder);
+ assert appView.enableWholeProgramOptimizations() || haveSideEffects
+ : "Expected instance-put instruction to have side effects in D8";
+ return !haveSideEffects;
+ }
+
+ @Override
public boolean identicalAfterRegisterAllocation(Instruction other, RegisterAllocator allocator) {
if (!super.identicalAfterRegisterAllocation(other, allocator)) {
return false;
diff --git a/src/main/java/com/android/tools/r8/ir/code/StaticPut.java b/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
index eb000ec..d8df7f1 100644
--- a/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
+++ b/src/main/java/com/android/tools/r8/ir/code/StaticPut.java
@@ -15,6 +15,7 @@
import com.android.tools.r8.dex.Constants;
import com.android.tools.r8.errors.Unreachable;
import com.android.tools.r8.graph.AppView;
+import com.android.tools.r8.graph.DexEncodedField;
import com.android.tools.r8.graph.DexField;
import com.android.tools.r8.graph.DexType;
import com.android.tools.r8.ir.analysis.ClassInitializationAnalysis;
@@ -107,7 +108,9 @@
return true;
}
- if (appInfoWithLiveness.isFieldRead(getField())) {
+ DexEncodedField resolveField = appInfoWithLiveness.resolveField(getField());
+ assert resolveField != null : "NoSuchFieldError (resolution failure) should be caught.";
+ if (appInfoWithLiveness.isFieldRead(resolveField.field)) {
return true;
}
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 b1e36a0..6f9da7a 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
@@ -26,10 +26,8 @@
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.FieldInstruction;
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.InstructionListIterator;
import com.android.tools.r8.ir.code.InvokeMethod;
import com.android.tools.r8.ir.code.Position;
@@ -104,8 +102,10 @@
replacement = createConstNumberReplacement(
code, rule.getReturnValue().getSingleValue(), typeLattice, instruction.getLocalInfo());
}
- if (replacement == null && rule != null
- && rule.hasReturnValue() && rule.getReturnValue().isField()) {
+ if (replacement == null
+ && rule != null
+ && rule.hasReturnValue()
+ && rule.getReturnValue().isField()) {
DexField field = rule.getReturnValue().getField();
assert typeLattice
== TypeLatticeElement.fromDexType(field.type, Nullability.maybeNull(), appView);
@@ -329,30 +329,6 @@
}
}
- private void rewritePutWithConstantValues(
- InstructionIterator iterator, FieldInstruction current, DexType context) {
- DexField field = current.getField();
- // TODO(b/123857022): Should be possible to use definitionFor().
- DexEncodedField target =
- current.isInstancePut()
- ? appView.appInfo().lookupInstanceTarget(field.holder, field)
- : appView.appInfo().lookupStaticTarget(field.holder, field);
- if (target == null) {
- return;
- }
-
- if (target.field.holder.classInitializationMayHaveSideEffects(
- appView, type -> appView.appInfo().isSubtype(context, type))) {
- return;
- }
-
- // TODO(b/123857022): Should be possible to use `!isFieldRead(field)`.
- if (!appView.appInfo().isFieldRead(target.field)) {
- // Remove writes to dead (i.e. never read) fields.
- iterator.removeOrReplaceByDebugLocalRead();
- }
- }
-
private void insertAssumeNotNull(
IRCode code,
Set<Value> affectedValues,
@@ -395,8 +371,6 @@
if (current.isInvokeMethod()) {
rewriteInvokeMethodWithConstantValues(
code, callingContext, affectedValues, blocks, iterator, current.asInvokeMethod());
- } else if (current.isInstancePut()) {
- rewritePutWithConstantValues(iterator, current.asFieldInstruction(), callingContext);
} else if (current.isStaticGet()) {
rewriteStaticGetWithConstantValues(
code,
diff --git a/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java b/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java
index c6402bd..bb36308 100644
--- a/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java
+++ b/src/test/java/com/android/tools/r8/ir/optimize/MemberValuePropagationTest.java
@@ -3,18 +3,15 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.ir.optimize;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
import com.android.tools.r8.TestBase;
import com.android.tools.r8.ToolHelper;
import com.android.tools.r8.utils.FileUtils;
import com.android.tools.r8.utils.codeinspector.ClassSubject;
import com.android.tools.r8.utils.codeinspector.CodeInspector;
-import com.android.tools.r8.utils.codeinspector.InstructionSubject;
import java.nio.file.Path;
import java.nio.file.Paths;
-import java.util.Iterator;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -49,11 +46,9 @@
ClassSubject clazz = inspector.clazz(QUALIFIED_CLASS_NAME);
clazz.forAllMethods(
methodSubject -> {
- Iterator<InstructionSubject> iterator = methodSubject.iterateInstructions();
- while (iterator.hasNext()) {
- InstructionSubject instruction = iterator.next();
- assertFalse(instruction.isInstancePut() || instruction.isStaticPut());
- }
+ assertTrue(
+ methodSubject.streamInstructions().noneMatch(
+ i -> i.isInstancePut() || i.isStaticPut()));
});
}
@@ -63,26 +58,11 @@
ClassSubject clazz = inspector.clazz(QUALIFIED_CLASS_NAME);
clazz.forAllMethods(
methodSubject -> {
- Iterator<InstructionSubject> iterator = methodSubject.iterateInstructions();
- if (methodSubject.isClassInitializer()) {
- int numPuts = 0;
- while (iterator.hasNext()) {
- if (iterator.next().isStaticPut()) {
- ++numPuts;
- }
- }
- // dead code removal is not part of -dontoptimize.
- assertEquals(0, numPuts);
- }
- if (methodSubject.isInstanceInitializer()) {
- int numPuts = 0;
- while (iterator.hasNext()) {
- if (iterator.next().isInstancePut()) {
- ++numPuts;
- }
- }
- assertEquals(1, numPuts);
- }
+ // Dead code removal is not part of -dontoptimize. That is, even with -dontoptimize,
+ // field put instructions are gone with better dead code removal.
+ assertTrue(
+ methodSubject.streamInstructions().noneMatch(
+ i -> i.isInstancePut() || i.isStaticPut()));
});
}
diff --git a/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java b/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
index f02109d..8d5db8c 100644
--- a/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
+++ b/src/test/java/com/android/tools/r8/naming/IdentifierNameStringMarkerTest.java
@@ -51,6 +51,7 @@
List<String> pgConfigs = ImmutableList.of(
"-identifiernamestring class " + CLASS_NAME + " { java.lang.String aClassName; }",
"-keep class " + CLASS_NAME,
+ "-keepclassmembers,allowobfuscation class " + CLASS_NAME + " { !static <fields>; }",
"-dontoptimize");
CodeInspector inspector = getInspectorAfterRunR8(builder, pgConfigs);
@@ -83,6 +84,7 @@
List<String> pgConfigs = ImmutableList.of(
"-identifiernamestring class " + CLASS_NAME + " { java.lang.String aClassName; }",
"-keep class " + CLASS_NAME,
+ "-keepclassmembers,allowobfuscation class " + CLASS_NAME + " { !static <fields>; }",
"-dontoptimize");
CodeInspector inspector = getInspectorAfterRunR8(builder, pgConfigs);
@@ -124,6 +126,7 @@
List<String> pgConfigs = ImmutableList.of(
"-identifiernamestring class " + CLASS_NAME + " { java.lang.String aClassName; }",
"-keep class " + CLASS_NAME,
+ "-keepclassmembers,allowobfuscation class " + CLASS_NAME + " { !static <fields>; }",
"-keep,allowobfuscation class " + BOO,
"-dontoptimize");
CodeInspector inspector = getInspectorAfterRunR8(builder, pgConfigs);
