Version 1.2.45.
Merge: Fix incorrect <clinit> optimization.
CL: https://r8-review.googlesource.com/c/r8/+/26000
R=sgjesse@google.com
Bug: 113326860
Change-Id: I3c7eea3bef13c6a0bdafb1f8e2bee651ebd5569a
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index e310190..0b00160 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
// This field is accessed from release scripts using simple pattern matching.
// Therefore, changing this field could break our release scripts.
- public static final String LABEL = "1.2.44";
+ public static final String LABEL = "1.2.45";
private Version() {
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
index 985b240..67e779a 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/CodeRewriter.java
@@ -1196,18 +1196,24 @@
// Check if the static put is a constant derived from the class holding the method.
// This checks for java.lang.Class.getName and java.lang.Class.getSimpleName.
- private boolean isClassNameConstant(DexEncodedMethod method, StaticPut put) {
+ private boolean isClassNameConstantOf(DexClass clazz, StaticPut put) {
if (put.getField().type != dexItemFactory.stringType) {
return false;
}
- if (put.inValue().definition != null && put.inValue().definition.isInvokeVirtual()) {
- InvokeVirtual invoke = put.inValue().definition.asInvokeVirtual();
+ if (put.inValue().definition != null) {
+ return isClassNameConstantOf(clazz, put.inValue().definition);
+ }
+ return false;
+ }
+
+ private boolean isClassNameConstantOf(DexClass clazz, Instruction instruction) {
+ if (instruction.isInvokeVirtual()) {
+ InvokeVirtual invoke = instruction.asInvokeVirtual();
if ((invoke.getInvokedMethod() == dexItemFactory.classMethods.getSimpleName
|| invoke.getInvokedMethod() == dexItemFactory.classMethods.getName)
&& !invoke.inValues().get(0).isPhi()
&& invoke.inValues().get(0).definition.isConstClass()
- && invoke.inValues().get(0).definition.asConstClass().getValue()
- == method.method.getHolder()) {
+ && invoke.inValues().get(0).definition.asConstClass().getValue() == clazz.type) {
return true;
}
}
@@ -1219,62 +1225,21 @@
return;
}
- // Collect relevant static put which are dominated by the exit block, and not dominated by a
- // static get.
- // This does not check for instructions that can throw. However, as classes which throw in the
- // class initializer are never visible to the program (see Java Virtual Machine Specification
- // section 5.5, https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-5.html#jvms-5.5), this
- // does not matter (except maybe for removal of const-string instructions, but that is
- // acceptable).
- if (code.computeNormalExitBlocks().isEmpty()) {
+ DexClass clazz = definitionFor(method.method.getHolder());
+ if (clazz == null) {
return;
}
- DexClass clazz = definitionFor(method.method.getHolder());
- assert clazz != null;
- DominatorTree dominatorTree = new DominatorTree(code);
+
+ // Collect straight-line static puts up to the first side-effect that is not
+ // a static put on a field on this class with a value that can be hoisted to
+ // the field initial value.
Set<StaticPut> puts = Sets.newIdentityHashSet();
- Map<DexField, StaticPut> dominatingPuts = Maps.newIdentityHashMap();
- for (BasicBlock block : dominatorTree.normalExitDominatorBlocks()) {
- InstructionListIterator iterator = block.listIterator(block.getInstructions().size());
- while (iterator.hasPrevious()) {
- Instruction current = iterator.previous();
- if (current.isStaticPut()) {
- StaticPut put = current.asStaticPut();
- DexField field = put.getField();
- if (clazz.definesStaticField(field)) {
- if (put.inValue().isConstant()) {
- if ((field.type.isClassType() || field.type.isArrayType())
- && put.inValue().isZero()) {
- // Collect put of zero as a potential default value.
- dominatingPuts.putIfAbsent(put.getField(), put);
- puts.add(put);
- } else if (field.type.isPrimitiveType() || field.type == dexItemFactory.stringType) {
- // Collect put as a potential default value.
- dominatingPuts.putIfAbsent(put.getField(), put);
- puts.add(put);
- }
- } else if (isClassNameConstant(method, put)) {
- // Collect put of class name constant as a potential default value.
- dominatingPuts.putIfAbsent(put.getField(), put);
- puts.add(put);
- }
- }
- }
- if (current.isStaticGet()) {
- // If a static field is read, any collected potential default value cannot be a
- // default value.
- DexField field = current.asStaticGet().getField();
- if (dominatingPuts.containsKey(field)) {
- dominatingPuts.remove(field);
- Iterables.removeIf(puts, put -> put.getField() == field);
- }
- }
- }
- }
+ Map<DexField, StaticPut> finalFieldPut = Maps.newIdentityHashMap();
+ computeUnnecessaryStaticPuts(code, method, clazz, puts, finalFieldPut);
if (!puts.isEmpty()) {
// Set initial values for static fields from the definitive static put instructions collected.
- for (StaticPut put : dominatingPuts.values()) {
+ for (StaticPut put : finalFieldPut.values()) {
DexField field = put.getField();
DexEncodedField encodedField = appInfo.definitionFor(field);
if (field.type == dexItemFactory.stringType) {
@@ -1328,24 +1293,22 @@
}
}
- // Remove the static put instructions now replaced by static filed initial values.
+ // Remove the static put instructions now replaced by static field initial values.
List<Instruction> toRemove = new ArrayList<>();
- for (BasicBlock block : dominatorTree.normalExitDominatorBlocks()) {
- InstructionListIterator iterator = block.listIterator();
- while (iterator.hasNext()) {
- Instruction current = iterator.next();
- if (current.isStaticPut() && puts.contains(current.asStaticPut())) {
- iterator.remove();
- // Collect, for removal, the instruction that created the value for the static put,
- // if all users are gone. This is done even if these instructions can throw as for
- // the current patterns matched these exceptions are not detectable.
- StaticPut put = current.asStaticPut();
- if (put.inValue().uniqueUsers().size() == 0) {
- if (put.inValue().isConstString()) {
- toRemove.add(put.inValue().definition);
- } else if (put.inValue().definition.isInvokeVirtual()) {
- toRemove.add(put.inValue().definition);
- }
+ InstructionIterator iterator = code.instructionIterator();
+ while (iterator.hasNext()) {
+ Instruction current = iterator.next();
+ if (current.isStaticPut() && puts.contains(current.asStaticPut())) {
+ iterator.remove();
+ // Collect, for removal, the instruction that created the value for the static put,
+ // if all users are gone. This is done even if these instructions can throw as for
+ // the current patterns matched these exceptions are not detectable.
+ StaticPut put = current.asStaticPut();
+ if (put.inValue().uniqueUsers().size() == 0) {
+ if (put.inValue().isConstString()) {
+ toRemove.add(put.inValue().definition);
+ } else if (put.inValue().definition.isInvokeVirtual()) {
+ toRemove.add(put.inValue().definition);
}
}
}
@@ -1353,18 +1316,70 @@
// Remove the instructions collected for removal.
if (toRemove.size() > 0) {
- for (BasicBlock block : dominatorTree.normalExitDominatorBlocks()) {
- InstructionListIterator iterator = block.listIterator();
- while (iterator.hasNext()) {
- if (toRemove.contains(iterator.next())) {
- iterator.remove();
- }
+ iterator = code.instructionIterator();
+ while (iterator.hasNext()) {
+ if (toRemove.contains(iterator.next())) {
+ iterator.remove();
}
}
}
}
}
+ private void computeUnnecessaryStaticPuts(IRCode code, DexEncodedMethod clinit, DexClass clazz,
+ Set<StaticPut> puts, Map<DexField, StaticPut> finalFieldPut) {
+ final int color = code.reserveMarkingColor();
+ try {
+ BasicBlock block = code.blocks.getFirst();
+ while (!block.isMarked(color) && block.getPredecessors().size() <= 1) {
+ block.mark(color);
+ InstructionListIterator it = block.listIterator();
+ while (it.hasNext()) {
+ Instruction instruction = it.next();
+ if (instructionHasSideEffects(instruction)) {
+ if (isClassNameConstantOf(clazz, instruction)) {
+ continue;
+ } else if (instruction.isStaticPut()) {
+ StaticPut put = instruction.asStaticPut();
+ if (put.getField().clazz != clazz.type) {
+ // Can cause clinit on another class which can read uninitialized static fields
+ // of this class.
+ return;
+ }
+ DexField field = put.getField();
+ if (clazz.definesStaticField(field)) {
+ if (put.inValue().isConstant()) {
+ if ((field.type.isClassType() || field.type.isArrayType())
+ && put.inValue().isZero()) {
+ finalFieldPut.put(put.getField(), put);
+ puts.add(put);
+ } else if (field.type.isPrimitiveType()
+ || field.type == dexItemFactory.stringType) {
+ finalFieldPut.put(put.getField(), put);
+ puts.add(put);
+ }
+ } else if (isClassNameConstantOf(clazz, put)) {
+ // Collect put of class name constant as a potential default value.
+ finalFieldPut.put(put.getField(), put);
+ puts.add(put);
+ }
+ }
+ } else if (!(instruction.isConstString() || instruction.isConstClass())) {
+ // Allow const string and const class which can only throw exceptions as their
+ // side-effect. Bail out for anything else.
+ return;
+ }
+ }
+ }
+ if (block.exit().isGoto()) {
+ block = block.exit().asGoto().getTarget();
+ }
+ }
+ } finally {
+ code.returnMarkingColor(color);
+ }
+ }
+
private DexClass definitionFor(DexType type) {
if (cachedClass != null && cachedClass.type == type) {
return cachedClass;
diff --git a/src/test/java/com/android/tools/r8/regress/b113326860/B113326860.java b/src/test/java/com/android/tools/r8/regress/b113326860/B113326860.java
new file mode 100644
index 0000000..ecac410
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/regress/b113326860/B113326860.java
@@ -0,0 +1,159 @@
+// Copyright (c) 2018 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.regress.b113326860;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.CompilationMode;
+import com.android.tools.r8.D8Command;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.code.Sput;
+import com.android.tools.r8.code.SputBoolean;
+import com.android.tools.r8.code.SputObject;
+import com.android.tools.r8.ir.code.SingleConstant;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.AndroidApp;
+import com.android.tools.r8.utils.DexInspector;
+import com.android.tools.r8.utils.DexInspector.ClassSubject;
+import com.android.tools.r8.utils.DexInspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.ExecutionException;
+import org.junit.Test;
+
+class TestClassDoOptimize {
+ private static boolean b = false;
+ private static int i = 3;
+ private static Class clazz;
+ static {
+ clazz = TestClassDoOptimize.class;
+ b = true;
+ i = 42;
+ }
+}
+
+class TestClassDoNotOptimize {
+
+ private static boolean initialized = false;
+ private static final TestClassDoNotOptimize INSTANCE;
+
+ TestClassDoNotOptimize() {
+ if (!initialized) {
+ System.out.println("Not initialized as expected");
+ }
+ }
+
+ static {
+ INSTANCE = new TestClassDoNotOptimize();
+ initialized = true;
+ }
+}
+
+class TestClassDoNotOptimize2 {
+ static boolean b = false;
+ static {
+ boolean forcingOtherClassInit = TestClassDoNotOptimize3.b;
+ b = true;
+ }
+
+ static void m() {
+ System.out.println(b);
+ }
+}
+
+class TestClassDoNotOptimize3 {
+ static boolean b = false;
+ static {
+ TestClassDoNotOptimize2.m();
+ }
+}
+
+class TestDoWhileLoop {
+ static int i = 0;
+ static int j = 0;
+ static {
+ do {
+ i = 42;
+ System.out.println(i);
+ j = j + 1;
+ i = 10;
+ } while (j < 10);
+ }
+
+ public static void main(String[] args) {
+ System.out.println(TestDoWhileLoop.i);
+ }
+}
+
+public class B113326860 {
+
+ private DexInspector compileTestClasses(List<Class> classes)
+ throws IOException, CompilationFailedException, ExecutionException {
+ D8Command.Builder builder = D8Command.builder().setMode(CompilationMode.RELEASE);
+ for (Class c : classes) {
+ builder.addClassProgramData(ToolHelper.getClassAsBytes(c), Origin.unknown());
+ }
+ AndroidApp app = ToolHelper.runD8(builder);
+ return new DexInspector(app);
+ }
+
+ @Test
+ public void optimizedClassInitializer()
+ throws IOException, CompilationFailedException, ExecutionException {
+ DexInspector inspector = compileTestClasses(ImmutableList.of(TestClassDoOptimize.class));
+ ClassSubject clazz = inspector.clazz(TestClassDoOptimize.class);
+ assertThat(clazz, isPresent());
+ MethodSubject method = clazz.method("void", "<clinit>", ImmutableList.of());
+ assertThat(method, isPresent());
+ assertFalse(Arrays.stream(method.getMethod().getCode().asDexCode().instructions)
+ .anyMatch(i -> i instanceof SputBoolean || i instanceof Sput));
+ assertTrue(Arrays.stream(method.getMethod().getCode().asDexCode().instructions)
+ .anyMatch(i -> i instanceof SputObject));
+ }
+
+ @Test
+ public void nonOptimizedClassInitializer()
+ throws ExecutionException, CompilationFailedException, IOException {
+ DexInspector inspector =
+ compileTestClasses(ImmutableList.of(TestClassDoNotOptimize.class));
+ ClassSubject clazz = inspector.clazz(TestClassDoNotOptimize.class);
+ assertThat(clazz, isPresent());
+ MethodSubject method = clazz.method("void", "<clinit>", ImmutableList.of());
+ assertThat(method, isPresent());
+ assertTrue(Arrays.stream(method.getMethod().getCode().asDexCode().instructions)
+ .anyMatch(i -> i instanceof SputBoolean));
+ }
+
+ @Test
+ public void nonOptimizedClassInitializer2()
+ throws ExecutionException, CompilationFailedException, IOException {
+ DexInspector inspector = compileTestClasses(
+ ImmutableList.of(TestClassDoNotOptimize2.class, TestClassDoNotOptimize3.class));
+ ClassSubject clazz = inspector.clazz(TestClassDoNotOptimize2.class);
+ assertThat(clazz, isPresent());
+ MethodSubject method = clazz.method("void", "<clinit>", ImmutableList.of());
+ assertThat(method, isPresent());
+ assertTrue(Arrays.stream(method.getMethod().getCode().asDexCode().instructions)
+ .anyMatch(i -> i instanceof SputBoolean));
+ }
+
+ @Test
+ public void doWhileLoop() throws ExecutionException, CompilationFailedException, IOException {
+ DexInspector inspector = compileTestClasses(ImmutableList.of(TestDoWhileLoop.class));
+ ClassSubject clazz = inspector.clazz(TestDoWhileLoop.class);
+ assertThat(clazz, isPresent());
+ MethodSubject method = clazz.method("void", "<clinit>", ImmutableList.of());
+ assertThat(method, isPresent());
+ // Leave the const 42 and the assignment in there!
+ assertTrue(Arrays.stream(method.getMethod().getCode().asDexCode().instructions)
+ .anyMatch(i -> i instanceof SingleConstant && (((SingleConstant) i).decodedValue() == 42)));
+ }
+}
diff --git a/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java b/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java
index 087b5f8..bedfa8f 100644
--- a/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java
+++ b/src/test/java/com/android/tools/r8/rewrite/staticvalues/StaticValuesTest.java
@@ -8,7 +8,9 @@
import static org.junit.Assert.assertTrue;
import com.android.tools.r8.ToolHelper.ProcessResult;
+import com.android.tools.r8.code.IfEqz;
import com.android.tools.r8.code.Instruction;
+import com.android.tools.r8.code.SgetBoolean;
import com.android.tools.r8.code.Sput;
import com.android.tools.r8.code.SputObject;
import com.android.tools.r8.graph.DexCode;
@@ -387,28 +389,20 @@
DexInspector inspector = new DexInspector(processedApplication);
assertTrue(inspector.clazz("Test").clinit().isPresent());
- DexValue value;
assertTrue(inspector.clazz("Test").field("int", "intField").hasExplicitStaticValue());
- value = inspector.clazz("Test").field("int", "intField").getStaticValue();
+ DexValue value = inspector.clazz("Test").field("int", "intField").getStaticValue();
assertTrue(value instanceof DexValueInt);
- assertEquals(3, ((DexValueInt) value).getValue());
+ assertEquals(1, ((DexValueInt) value).getValue());
assertTrue(inspector.clazz("Test").field("java.lang.String", "stringField").hasExplicitStaticValue());
value = inspector.clazz("Test").field("java.lang.String", "stringField").getStaticValue();
assertTrue(value instanceof DexValueString);
- assertEquals(("7"), ((DexValueString) value).getValue().toString());
+ // We stop at control-flow and therefore the initial value for the string field will be 5.
+ assertEquals(("5"), ((DexValueString) value).getValue().toString());
DexCode code = inspector.clazz("Test").clinit().getMethod().getCode().asDexCode();
- for (Instruction instruction : code.instructions) {
- if (instruction instanceof Sput) {
- Sput put = (Sput) instruction;
- // Only int put ot intField2.
- assertEquals(put.getField().name.toString(), "intField2");
- } else {
- // No Object (String) puts.
- assertFalse(instruction instanceof SputObject);
- }
- }
+ assertTrue(code.instructions[0] instanceof SgetBoolean);
+ assertTrue(code.instructions[1] instanceof IfEqz);
String result = runArt(processedApplication);