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);