Reland "Relax the dead condition of field reads."
PS1 is same as go/r8g/33900.
PS2/4 re-enable the test in question on 6.0.1 / for R8 only.
Bug: 70295684, 124039115
Change-Id: I2d21a10829bb1f11f8080040a3869a63b3ba8f18
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 40756ae..5f3c869 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
@@ -4,6 +4,8 @@
package com.android.tools.r8.ir.code;
+import static com.android.tools.r8.optimize.MemberRebindingAnalysis.isVisibleFromOriginalContext;
+
import com.android.tools.r8.cf.LoadStoreHelper;
import com.android.tools.r8.cf.TypeVerificationHelper;
import com.android.tools.r8.cf.code.CfFieldInstruction;
@@ -19,6 +21,7 @@
import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.AppInfoWithSubtyping;
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;
@@ -31,6 +34,7 @@
import com.android.tools.r8.ir.conversion.DexBuilder;
import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
import com.android.tools.r8.ir.optimize.InliningConstraints;
+import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import org.objectweb.asm.Opcodes;
public class InstanceGet extends FieldInstruction {
@@ -98,6 +102,29 @@
}
@Override
+ public boolean canBeDeadCode(
+ AppView<? extends AppInfoWithLiveness> appView, AppInfo appInfo, IRCode code) {
+ // Not applicable for D8.
+ if (appView == null || !appView.enableWholeProgramOptimizations()) {
+ return false;
+ }
+ // instance-get can be dead code as long as it cannot have any of the following:
+ // * NoSuchFieldError (resolution failure)
+ // * IllegalAccessError (not visible from the access context)
+ // * NullPointerException (null receiver).
+ // TODO(b/123857022): Should be possible to use definitionFor().
+ DexEncodedField resolvedField = appInfo.resolveFieldOn(getField().getHolder(), getField());
+ if (resolvedField == null) {
+ return false;
+ }
+ if (code == null
+ || !isVisibleFromOriginalContext(appInfo, code.method.method.getHolder(), resolvedField)) {
+ return false;
+ }
+ return object().getTypeLattice().nullability().isDefinitelyNotNull();
+ }
+
+ @Override
public int maxInValueRegister() {
return Constants.U4BIT_MAX;
}
diff --git a/src/main/java/com/android/tools/r8/ir/code/StaticGet.java b/src/main/java/com/android/tools/r8/ir/code/StaticGet.java
index b79d076..f27ebd6 100644
--- a/src/main/java/com/android/tools/r8/ir/code/StaticGet.java
+++ b/src/main/java/com/android/tools/r8/ir/code/StaticGet.java
@@ -3,6 +3,8 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.ir.code;
+import static com.android.tools.r8.optimize.MemberRebindingAnalysis.isVisibleFromOriginalContext;
+
import com.android.tools.r8.cf.LoadStoreHelper;
import com.android.tools.r8.cf.TypeVerificationHelper;
import com.android.tools.r8.cf.code.CfFieldInstruction;
@@ -18,6 +20,7 @@
import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.AppInfoWithSubtyping;
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;
@@ -29,6 +32,7 @@
import com.android.tools.r8.ir.conversion.DexBuilder;
import com.android.tools.r8.ir.optimize.Inliner.ConstraintWithTarget;
import com.android.tools.r8.ir.optimize.InliningConstraints;
+import com.android.tools.r8.shaking.Enqueuer.AppInfoWithLiveness;
import org.objectweb.asm.Opcodes;
public class StaticGet extends FieldInstruction {
@@ -91,6 +95,33 @@
}
@Override
+ public boolean canBeDeadCode(
+ AppView<? extends AppInfoWithLiveness> appView, AppInfo appInfo, IRCode code) {
+ // Not applicable for D8.
+ if (appView == null || !appView.enableWholeProgramOptimizations()) {
+ return false;
+ }
+ // static-get can be dead as long as it cannot have any of the following:
+ // * NoSuchFieldError (resolution failure)
+ // * IllegalAccessError (not visible from the access context)
+ // * side-effects in <clinit>
+ // TODO(b/123857022): Should be possible to use definitionFor().
+ DexEncodedField resolvedField = appInfo.resolveFieldOn(getField().getHolder(), getField());
+ if (resolvedField == null) {
+ return false;
+ }
+ if (code == null
+ || !isVisibleFromOriginalContext(appInfo, code.method.method.getHolder(), resolvedField)) {
+ return false;
+ }
+ DexType context = code.method.method.holder;
+ return !getField().clazz.classInitializationMayHaveSideEffects(
+ appInfo,
+ // Types that are a super type of `context` are guaranteed to be initialized already.
+ type -> context.isSubtypeOf(type, appInfo));
+ }
+
+ @Override
public int maxInValueRegister() {
return Constants.U8BIT_MAX;
}
diff --git a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
index 3ff88b9..a2bed1d 100644
--- a/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
+++ b/src/main/java/com/android/tools/r8/optimize/MemberRebindingAnalysis.java
@@ -3,6 +3,7 @@
// BSD-style license that can be found in the LICENSE file.
package com.android.tools.r8.optimize;
+import com.android.tools.r8.graph.AppInfo;
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexClass;
import com.android.tools.r8.graph.DexEncodedField;
@@ -290,14 +291,15 @@
Set<DexEncodedMethod> contexts = fieldsWithContexts.get(field);
if (target != null && target.field != field
&& contexts.stream().allMatch(context ->
- isVisibleFromOriginalContext(context.method.getHolder(), target))) {
+ isVisibleFromOriginalContext(appInfo, context.method.getHolder(), target))) {
builder.map(field,
lense.lookupField(validTargetFor(target.field, field, lookupTargetOnClass)));
}
}
}
- private boolean isVisibleFromOriginalContext(DexType context, DexEncodedField field) {
+ public static boolean isVisibleFromOriginalContext(
+ AppInfo appInfo, DexType context, DexEncodedField field) {
DexType holderType = field.field.getHolder();
DexClass holder = appInfo.definitionFor(holderType);
if (holder == null) {
diff --git a/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java b/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
index 5efc26d..6d34f44 100644
--- a/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
+++ b/src/test/java/com/android/tools/r8/R8RunArtTestsTest.java
@@ -508,7 +508,6 @@
"156-register-dex-file-multi-loader",
"412-new-array",
"530-checker-lse2",
- "550-new-instance-clinit",
"580-checker-round",
"594-invoke-super",
"625-checker-licm-regressions",
@@ -670,6 +669,12 @@
// lib64 libarttest.so: wrong ELF class ELFCLASS64.
.put("543-env-long-ref",
TestCondition.match(TestCondition.runtimesUpTo(DexVm.Version.V4_4_4)))
+ // Leaving two static-get triggers LSE bug on 6.0.1 (b/25735083).
+ // R8, with subtyping, knows the first sget is dead, and removing it avoids the bug.
+ // Due to the lack of subtype hierarchy, D8 can't guarantee <clinit> side effects.
+ .put("550-new-instance-clinit",
+ TestCondition.match(
+ TestCondition.D8_COMPILER, TestCondition.runtimes(DexVm.Version.V6_0_1)))
// Regression test for an issue that is not fixed on version 5.1.1. Throws an Exception
// instance instead of the expected NullPointerException. This bug is only tickled when
// running the R8 generated code when starting from jar or from dex code generated with
diff --git a/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java b/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java
index 33f28fc..ea8ed0b 100644
--- a/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java
+++ b/src/test/java/com/android/tools/r8/jasmin/MemberResolutionTest.java
@@ -462,7 +462,7 @@
main.addMainMethod(
".limit stack 2",
".limit locals 1",
- " getstatic Empty/aMethod I",
+ " getstatic Empty/aField I",
" return");
ensureRuntimeException(builder, NoSuchFieldError.class);
diff --git a/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java b/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java
new file mode 100644
index 0000000..f306e04
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/FieldReadsJasminTest.java
@@ -0,0 +1,286 @@
+// 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.shaking;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isRenamed;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import com.android.tools.r8.jasmin.JasminBuilder;
+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.codeinspector.ClassSubject;
+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 com.google.common.collect.ImmutableList;
+import java.util.Iterator;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class FieldReadsJasminTest extends JasminTestBase {
+ private static final String CLS = "Empty";
+ private static final String MAIN = "Main";
+ private final Backend backend;
+
+ @Parameterized.Parameters(name = "Backend: {0}")
+ public static Object[] data() {
+ return Backend.values();
+ }
+
+ public FieldReadsJasminTest(Backend backend) {
+ this.backend = backend;
+ }
+
+ @Test
+ public void testInstanceGet_nonNullReceiver() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder empty = builder.addClass(CLS);
+ empty.addField("protected", "aField", "I", null);
+
+ ClassBuilder main = builder.addClass(MAIN);
+ main.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " new Empty",
+ " dup",
+ " invokespecial Empty/<init>()V",
+ " getfield Empty/aField I",
+ " return");
+
+ ensureNoFields(builder, empty);
+ }
+
+ @Test
+ public void testStaticGet_noSideEffect() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder empty = builder.addClass(CLS);
+ empty.addStaticField("sField", "I");
+
+ ClassBuilder main = builder.addClass(MAIN);
+ main.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " getstatic Empty/sField I",
+ " return");
+
+ ensureNoFields(builder, empty);
+ }
+
+ private void ensureNoFields(JasminBuilder app, ClassBuilder clazz) throws Exception {
+ testForR8(backend)
+ .addProgramClassFileData(app.buildClasses())
+ .addKeepRules("-keep class * { <methods>; }")
+ .compile()
+ .inspect(inspector -> {
+ ClassSubject classSubject = inspector.clazz(clazz.name);
+ assertThat(classSubject, isPresent());
+ classSubject.forAllFields(foundFieldSubject -> {
+ fail("Expect not to see any fields.");
+ });
+ });
+ }
+
+ @Test
+ public void testStaticGet_nonTrivialClinit_yetSameHolder() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder main = builder.addClass(MAIN);
+ // static int sField = System.currentTimeMillis() >=0 ? 42 : 0;
+ main.addStaticField("sField", "I", null);
+ main.addClassInitializer(
+ ".limit stack 4",
+ ".limit locals 0",
+ " invokestatic java/lang/System/currentTimeMillis()J",
+ " lconst_0",
+ " lcmp",
+ " iflt l",
+ " bipush 42",
+ " goto p",
+ "l:",
+ " iconst_0",
+ "p:",
+ " putstatic Main/sField I",
+ " return");
+ MethodSignature mainMethod = main.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " getstatic Main/sField I",
+ " return");
+
+ ensureFieldExistsButNoRead(builder, main, mainMethod, main, "sField");
+ }
+
+ private void ensureFieldExistsButNoRead(
+ JasminBuilder app,
+ ClassBuilder clazz,
+ MethodSignature method,
+ ClassBuilder fieldHolder,
+ String fieldName)
+ throws Exception {
+ testForR8(backend)
+ .addProgramClassFileData(app.buildClasses())
+ .addKeepRules("-keep class * { <methods>; }")
+ .compile()
+ .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());
+ });
+ }
+
+ @Test
+ public void testInstanceGet_nullableReceiver() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder empty = builder.addClass(CLS);
+ empty.addDefaultConstructor();
+ empty.addField("protected", "aField", "I", null);
+ MethodSignature foo = empty.addStaticMethod("foo", ImmutableList.of("L" + CLS + ";"), "V",
+ ".limit stack 2",
+ ".limit locals 1",
+ " aload 0",
+ " getfield Empty/aField I",
+ " return");
+
+ ClassBuilder main = builder.addClass(MAIN);
+ main.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " aconst_null",
+ " invokestatic Empty/foo(L" + CLS + ";)V",
+ " return");
+
+ ensureFieldExistsAndReadOnlyOnce(builder, empty, foo, empty, "aField");
+ }
+
+ @Test
+ public void testStaticGet_nonTrivialClinit() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder empty = builder.addClass(CLS);
+ empty.addDefaultConstructor();
+ empty.addStaticField("sField", "I");
+ empty.addClassInitializer(
+ ".limit stack 3",
+ ".limit locals 1",
+ " getstatic java/lang/System/out Ljava/io/PrintStream;",
+ " ldc \"hello\"",
+ " invokevirtual java/io/PrintStream/print(Ljava/lang/String;)V",
+ " return");
+
+ ClassBuilder main = builder.addClass(MAIN);
+ MethodSignature mainMethod = main.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " getstatic Empty/sField I",
+ " return");
+
+ ensureFieldExistsAndReadOnlyOnce(builder, main, mainMethod, empty, "sField");
+ }
+
+ @Test
+ public void testStaticGet_allocation() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder empty = builder.addClass(CLS);
+ empty.addDefaultConstructor();
+ empty.addStaticField("sField", "Ljava/lang/String;", "\"8\"");
+
+ ClassBuilder main = builder.addClass(MAIN);
+ MethodSignature mainMethod = main.addMainMethod(
+ ".limit stack 2",
+ ".limit locals 1",
+ " getstatic Empty/sField Ljava/lang/String;",
+ " return");
+
+ ensureFieldExistsAndReadOnlyOnce(builder, main, mainMethod, empty, "sField");
+ }
+
+ @Test
+ public void b124039115() throws Exception {
+ JasminBuilder builder = new JasminBuilder();
+
+ ClassBuilder empty = builder.addClass(CLS);
+ empty.addDefaultConstructor();
+ empty.addClassInitializer(
+ ".limit stack 2",
+ ".limit locals 0",
+ " getstatic Main/sField I",
+ " iconst_1",
+ " iadd",
+ " putstatic Main/sField I",
+ " return");
+
+ ClassBuilder main = builder.addClass(MAIN);
+ main.addDefaultConstructor();
+ main.addStaticField("sField", "I", null);
+ main.addClassInitializer(
+ ".limit stack 2",
+ ".limit locals 0",
+ " bipush 1",
+ " putstatic Main/sField I",
+ " return");
+ MethodSignature mainMethod = main.addMainMethod(
+ ".limit stack 3",
+ ".limit locals 2",
+ " getstatic Main/sField I",
+ " new Empty",
+ " dup",
+ " invokespecial Empty/<init>()V",
+ " getstatic Main/sField I",
+ " bipush 2",
+ " if_icmpeq r",
+ " aconst_null",
+ " athrow",
+ "r:",
+ " return");
+
+ ensureFieldExistsAndReadOnlyOnce(builder, main, mainMethod, main, "sField");
+ }
+
+ private void ensureFieldExistsAndReadOnlyOnce(
+ JasminBuilder app,
+ ClassBuilder clazz,
+ MethodSignature method,
+ ClassBuilder fieldHolder,
+ String fieldName)
+ throws Exception {
+ testForR8(backend)
+ .addProgramClassFileData(app.buildClasses())
+ .addKeepRules("-keep class * { <methods>; }")
+ .compile()
+ .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);
+ assertTrue(it.hasNext());
+ assertEquals(fld.getFinalName(), it.next().getField().name.toString());
+ assertFalse(it.hasNext());
+ });
+ }
+
+}
