Relax dead condition of array-length.
by implementing instructionMayHaveSideEffects:
as long as the given array is known to be non-null, array-length is
free from side effects---NPE.
It is not removable yet, though, because non-null IR, which informs
the nullability of array, is wiped out before dead code removal.
Bug: 129530569, 120920488
Change-Id: I75226415200271149f01f0dfa97b038aa3f32739
diff --git a/src/main/java/com/android/tools/r8/ir/code/ArrayLength.java b/src/main/java/com/android/tools/r8/ir/code/ArrayLength.java
index 6e6a9f8..9de9b6e 100644
--- a/src/main/java/com/android/tools/r8/ir/code/ArrayLength.java
+++ b/src/main/java/com/android/tools/r8/ir/code/ArrayLength.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.graph.AppView;
import com.android.tools.r8.graph.DexItemFactory;
import com.android.tools.r8.graph.DexType;
+import com.android.tools.r8.ir.analysis.AbstractError;
import com.android.tools.r8.ir.analysis.type.TypeLatticeElement;
import com.android.tools.r8.ir.conversion.CfBuilder;
import com.android.tools.r8.ir.conversion.DexBuilder;
@@ -70,6 +71,27 @@
}
@Override
+ public AbstractError instructionInstanceCanThrow(
+ AppView<? extends AppInfo> appView, DexType context) {
+ if (array().typeLattice.isNullable()) {
+ return AbstractError.specific(appView.dexItemFactory().npeType);
+ }
+
+ return AbstractError.bottom();
+ }
+
+ @Override
+ public boolean instructionMayHaveSideEffects(
+ AppView<? extends AppInfo> appView, DexType context) {
+ return instructionInstanceCanThrow(appView, context).isThrowing();
+ }
+
+ @Override
+ public boolean canBeDeadCode(AppView<? extends AppInfo> appView, IRCode code) {
+ return !instructionMayHaveSideEffects(appView, code.method.method.holder);
+ }
+
+ @Override
public boolean identicalAfterRegisterAllocation(Instruction other, RegisterAllocator allocator) {
if (super.identicalAfterRegisterAllocation(other, allocator)) {
// The array length instruction doesn't carry the element type. The art verifier doesn't
diff --git a/src/test/java/com/android/tools/r8/naming/arraytypes/ArrayTypesTest.java b/src/test/java/com/android/tools/r8/naming/arraytypes/ArrayTypesTest.java
index 5315868..adfbcec 100644
--- a/src/test/java/com/android/tools/r8/naming/arraytypes/ArrayTypesTest.java
+++ b/src/test/java/com/android/tools/r8/naming/arraytypes/ArrayTypesTest.java
@@ -4,8 +4,11 @@
package com.android.tools.r8.naming.arraytypes;
+import static org.junit.Assume.assumeTrue;
+
import com.android.tools.r8.TestBase;
-import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
import com.android.tools.r8.utils.DescriptorUtils;
import com.android.tools.r8.utils.FileUtils;
import com.android.tools.r8.utils.StringUtils;
@@ -24,7 +27,7 @@
@RunWith(Parameterized.class)
public class ArrayTypesTest extends TestBase {
- private final Backend backend;
+ private final TestParameters parameters;
private static String packageName;
private static String arrayBaseTypeDescriptor;
@@ -32,13 +35,13 @@
private static String generatedTestClassName;
private static String expectedOutput;
- public ArrayTypesTest(Backend backend) {
- this.backend = backend;
+ public ArrayTypesTest(TestParameters parameters) {
+ this.parameters = parameters;
}
- @Parameterized.Parameters(name = "Backend: {0}")
- public static Object[] data() {
- return ToolHelper.getBackends();
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().build();
}
@BeforeClass
@@ -66,13 +69,14 @@
}
private void runR8Test(boolean enableMinification) throws Exception {
- testForR8(backend)
+ testForR8(parameters.getBackend())
.minification(enableMinification)
.addProgramClasses(Main.class, A.class)
.addProgramClassFileData(generateTestClass())
.addKeepMainRule(Main.class)
.addKeepRules("-keep class " + generatedTestClassName + " { test(...); }")
- .run(Main.class)
+ .setMinApi(parameters.getRuntime())
+ .run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutput(expectedOutput);
}
@@ -95,27 +99,28 @@
StringUtils.lines(
A.class.getTypeName() + " -> " + packageName + ".a:"));
- testForR8(backend)
+ testForR8(parameters.getBackend())
.addProgramClasses(Main.class, A.class)
.addProgramClassFileData(generateTestClass())
.addKeepMainRule(Main.class)
.addKeepRules("-applymapping " + mappingFile.toAbsolutePath())
.noMinification()
.noTreeShaking()
- .run(Main.class)
+ .setMinApi(parameters.getRuntime())
+ .run(parameters.getRuntime(), Main.class)
.assertSuccessWithOutput(expectedOutput);
}
@Test
public void testD8() throws Exception {
- if (backend == Backend.DEX) {
- testForD8()
- .addProgramClasses(Main.class, A.class)
- .addProgramClassFileData(generateTestClass())
- .run(Main.class)
- .writeProcessResult(System.out)
- .assertSuccessWithOutput(expectedOutput);
- }
+ assumeTrue(parameters.isDexRuntime());
+ testForD8()
+ .addProgramClasses(Main.class, A.class)
+ .addProgramClassFileData(generateTestClass())
+ .setMinApi(parameters.getRuntime())
+ .run(parameters.getRuntime(), Main.class)
+ .writeProcessResult(System.out)
+ .assertSuccessWithOutput(expectedOutput);
}
public static byte[] generateTestClass() {
diff --git a/src/test/java/com/android/tools/r8/shaking/array/DeadArrayLengthTest.java b/src/test/java/com/android/tools/r8/shaking/array/DeadArrayLengthTest.java
new file mode 100644
index 0000000..4b24afc
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/shaking/array/DeadArrayLengthTest.java
@@ -0,0 +1,131 @@
+// 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.array;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.NeverPropagateValue;
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestParameters;
+import com.android.tools.r8.TestParametersCollection;
+import com.android.tools.r8.utils.StringUtils;
+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 com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.Streams;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class DeadArrayLengthTest extends TestBase {
+ private static final Class<?> MAIN = TestClass.class;
+ private static final String EXPECTED_OUTPUT = StringUtils.lines("1", "Expected NPE", "3");
+
+ private final TestParameters parameters;
+
+ @Parameterized.Parameters(name = "{0}")
+ public static TestParametersCollection data() {
+ return getTestParameters().withAllRuntimes().build();
+ }
+
+ public DeadArrayLengthTest(TestParameters parameters) {
+ this.parameters = parameters;
+ }
+
+ private void inspect(CodeInspector inspector) {
+ ClassSubject main = inspector.clazz(MAIN);
+ assertThat(main, isPresent());
+
+ MethodSubject nonNull = main.uniqueMethodWithName("clearlyNonNull");
+ assertThat(nonNull, isPresent());
+ assertEquals(
+ 0,
+ Streams.stream(nonNull.iterateInstructions(InstructionSubject::isArrayLength)).count());
+
+ MethodSubject nullable = main.uniqueMethodWithName("isNullable");
+ assertThat(nullable, isPresent());
+ assertEquals(
+ 1,
+ Streams.stream(nullable.iterateInstructions(InstructionSubject::isArrayLength)).count());
+
+ MethodSubject nullCheck = main.uniqueMethodWithName("afterNullCheck");
+ assertThat(nullCheck, isPresent());
+ // TODO(b/120920488): could be zero if we extend non-null to first dead code removal.
+ assertEquals(
+ 1,
+ Streams.stream(nullCheck.iterateInstructions(InstructionSubject::isArrayLength)).count());
+ }
+
+ @Test
+ public void testD8() throws Exception {
+ assumeTrue(parameters.isDexRuntime());
+ testForD8()
+ .release()
+ .addProgramClasses(MAIN)
+ .setMinApi(parameters.getRuntime())
+ .run(parameters.getRuntime(), MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT)
+ .inspect(this::inspect);
+ }
+
+ @Test
+ public void testR8() throws Exception {
+ testForR8(parameters.getBackend())
+ .addProgramClasses(MAIN)
+ .addKeepClassAndMembersRules(MAIN)
+ .enableMemberValuePropagationAnnotations()
+ .noMinification()
+ .setMinApi(parameters.getRuntime())
+ .run(parameters.getRuntime(), MAIN)
+ .assertSuccessWithOutput(EXPECTED_OUTPUT)
+ .inspect(this::inspect);
+ }
+
+ static class TestClass {
+ @NeverPropagateValue
+ static int clearlyNonNull(int size) {
+ int[] array = new int[size];
+ int l = array.length;
+ return System.currentTimeMillis() > 0 ? 1 : -1;
+ }
+
+ @NeverPropagateValue
+ static int isNullable(int[] args) {
+ // Not used, but can't be dead due to the potential NPE.
+ int l = args.length;
+ return System.currentTimeMillis() > 0 ? 2 : -2;
+ }
+
+ @NeverPropagateValue
+ static int afterNullCheck(int[] args) {
+ if (args != null) {
+ // Can be removed.
+ int l = args.length;
+ return System.currentTimeMillis() > 0 ? 8 : -8;
+ }
+ return System.currentTimeMillis() > 0 ? 3 : -3;
+ }
+
+ public static void main(String[] args) {
+ System.out.println(clearlyNonNull(args.length));
+ try {
+ System.out.println(isNullable(null));
+ fail("Expect to see NPE");
+ } catch (NullPointerException npe) {
+ System.out.println("Expected NPE");
+ }
+ try {
+ System.out.println(afterNullCheck(null));
+ } catch (NullPointerException npe) {
+ fail("Not expect to see NPE");
+ }
+ }
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java
index ded4f14..1223b61 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/CfInstructionSubject.java
@@ -6,6 +6,7 @@
import com.android.tools.r8.cf.code.CfArithmeticBinop;
+import com.android.tools.r8.cf.code.CfArrayLength;
import com.android.tools.r8.cf.code.CfArrayStore;
import com.android.tools.r8.cf.code.CfCheckCast;
import com.android.tools.r8.cf.code.CfConstClass;
@@ -306,6 +307,11 @@
}
@Override
+ public boolean isArrayLength() {
+ return instruction instanceof CfArrayLength;
+ }
+
+ @Override
public boolean isArrayPut() {
return instruction instanceof CfArrayStore;
}
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java
index 3b04382..c195dc0 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/DexInstructionSubject.java
@@ -11,6 +11,7 @@
import com.android.tools.r8.code.AputObject;
import com.android.tools.r8.code.AputShort;
import com.android.tools.r8.code.AputWide;
+import com.android.tools.r8.code.ArrayLength;
import com.android.tools.r8.code.CheckCast;
import com.android.tools.r8.code.Const;
import com.android.tools.r8.code.Const16;
@@ -395,6 +396,11 @@
}
@Override
+ public boolean isArrayLength() {
+ return instruction instanceof ArrayLength;
+ }
+
+ @Override
public boolean isArrayPut() {
return instruction instanceof Aput
|| instruction instanceof AputBoolean
diff --git a/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java b/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java
index d297a3b..43ef5d4 100644
--- a/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java
+++ b/src/test/java/com/android/tools/r8/utils/codeinspector/InstructionSubject.java
@@ -90,6 +90,8 @@
boolean isNewArray();
+ boolean isArrayLength();
+
boolean isArrayPut();
boolean isMonitorEnter();
