Version 1.2.48.
Merge: Never lower const instruction past a cmp instruction on Lollipop.
CL: https://r8-review.googlesource.com/c/r8/+/27106
R=christofferqa@google.com
Bug: 115552239
Change-Id: Idce64af606a4befc63df03b9a459934b0183d6a1
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index ddbe2a1..c1d5255 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.47";
+ public static final String LABEL = "1.2.48";
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 748c078..d886120 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
@@ -1766,13 +1766,11 @@
InstructionListIterator insertAt = block.listIterator();
// Place the instruction as late in the block as we can. It needs to go before users
// and if we have catch handlers it needs to be placed before the throwing instruction.
- // Also, in an attempt to help Art VMs with broken bounds check elimination code,
- // we also do not lower the constants past array-length instructions in their
- // target block. See comment on options.canHaveBoundsCheckEliminationBug.
insertAt.nextUntil(i ->
i.inValues().contains(instruction.outValue())
- || i.isJumpInstruction()
- || (hasCatchHandlers && i.instructionTypeCanThrow()));
+ || i.isJumpInstruction()
+ || (hasCatchHandlers && i.instructionTypeCanThrow())
+ || (options.canHaveCmpIfFloatBug() && i.isCmp()));
Instruction next = insertAt.previous();
instruction.forceSetPosition(
next.isGoto() ? next.asGoto().getTarget().getPosition() : next.getPosition());
diff --git a/src/main/java/com/android/tools/r8/utils/InternalOptions.java b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
index 97b870c..c3a2298 100644
--- a/src/main/java/com/android/tools/r8/utils/InternalOptions.java
+++ b/src/main/java/com/android/tools/r8/utils/InternalOptions.java
@@ -591,6 +591,34 @@
return minApiLevel < AndroidApiLevel.L.getLevel();
}
+ // Some Lollipop VMs crash if there is a const instruction between a cmp and an if instruction.
+ //
+ // Crashing code:
+ //
+ // :goto_0
+ // cmpg-float v0, p0, p0
+ // const/4 v1, 0
+ // if-gez v0, :cond_0
+ // add-float/2addr p0, v1
+ // goto :goto_0
+ // :cond_0
+ // return p0
+ //
+ // Working code:
+ // :goto_0
+ // const/4 v1, 0
+ // cmpg-float v0, p0, p0
+ // if-gez v0, :cond_0
+ // add-float/2addr p0, v1
+ // goto :goto_0
+ // :cond_0
+ // return p0
+ //
+ // See b/115552239.
+ public boolean canHaveCmpIfFloatBug() {
+ return minApiLevel < AndroidApiLevel.M.getLevel();
+ }
+
// Some Lollipop VMs incorrectly optimize code with mul2addr instructions. In particular,
// the following hash code method produces wrong results after optimizations:
//
diff --git a/src/test/java/com/android/tools/r8/regress/b115552239/B115552239.java b/src/test/java/com/android/tools/r8/regress/b115552239/B115552239.java
new file mode 100644
index 0000000..80b7954
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/regress/b115552239/B115552239.java
@@ -0,0 +1,92 @@
+// 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.b115552239;
+
+import static com.android.tools.r8.utils.DexInspectorMatchers.isPresent;
+import static junit.framework.TestCase.assertFalse;
+import static junit.framework.TestCase.assertTrue;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import com.android.tools.r8.CompilationFailedException;
+import com.android.tools.r8.D8Command;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.code.CmpgFloat;
+import com.android.tools.r8.code.IfGez;
+import com.android.tools.r8.code.Instruction;
+import com.android.tools.r8.origin.Origin;
+import com.android.tools.r8.utils.AndroidApiLevel;
+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.concurrent.ExecutionException;
+import org.junit.Test;
+
+class TestClass {
+ public float f(float d) {
+ while (d < 0f) {
+ d += 360f;
+ }
+ return d % 360f;
+ }
+}
+
+public class B115552239 {
+
+ private MethodSubject compileTestClassAndGetMethod(int apiLevel)
+ throws IOException, CompilationFailedException, ExecutionException {
+ AndroidApp app =
+ ToolHelper.runD8(
+ D8Command.builder()
+ .addClassProgramData(ToolHelper.getClassAsBytes(TestClass.class), Origin.unknown())
+ .setMinApiLevel(apiLevel));
+ DexInspector inspector = new DexInspector(app);
+ ClassSubject clazz = inspector.clazz(TestClass.class);
+ assertThat(clazz, isPresent());
+ MethodSubject method = clazz.method("float", "f", ImmutableList.of("float"));
+ assertThat(method, isPresent());
+ return method;
+ }
+
+ @Test
+ public void noLowering()
+ throws IOException, CompilationFailedException, ExecutionException {
+ MethodSubject method = compileTestClassAndGetMethod(AndroidApiLevel.L.getLevel());
+ boolean previousWasCmp = false;
+ Instruction[] instructions = method.getMethod().getCode().asDexCode().instructions;
+ assertTrue(Arrays.stream(instructions).anyMatch(i -> i instanceof CmpgFloat));
+ for (Instruction instruction : instructions) {
+ if (instruction instanceof CmpgFloat) {
+ previousWasCmp = true;
+ continue;
+ } else if (previousWasCmp) {
+ assertTrue(instruction instanceof IfGez);
+ }
+ previousWasCmp = false;
+ }
+ }
+
+ @Test
+ public void lowering()
+ throws IOException, CompilationFailedException, ExecutionException {
+ MethodSubject method = compileTestClassAndGetMethod(AndroidApiLevel.M.getLevel());
+ boolean previousWasCmp = false;
+ Instruction[] instructions = method.getMethod().getCode().asDexCode().instructions;
+ assertTrue(Arrays.stream(instructions).anyMatch(i -> i instanceof CmpgFloat));
+ for (Instruction instruction : instructions) {
+ if (instruction instanceof CmpgFloat) {
+ previousWasCmp = true;
+ continue;
+ } else if (previousWasCmp) {
+ // We lowered the const instruction as close to its use as possible.
+ assertFalse(instruction instanceof IfGez);
+ }
+ previousWasCmp = false;
+ }
+ }
+}
