Move dup to block with store in case of linear flow
When the StoreLoadToDupStorePeephole matched we could move dups and
store down into the block that had the load. If that block had
exception handlers we would get a verify error.
Bug: 122445224
Change-Id: I39ebeaa82b9fdfec94b281b2688a7fecbd4b0dc4
diff --git a/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java b/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java
index cde197c..48b6c9b 100644
--- a/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java
+++ b/src/main/java/com/android/tools/r8/ir/code/InstructionListIterator.java
@@ -11,8 +11,10 @@
import java.util.ListIterator;
import java.util.function.Predicate;
-public interface InstructionListIterator extends ListIterator<Instruction>,
- NextUntilIterator<Instruction> {
+public interface InstructionListIterator
+ extends ListIterator<Instruction>,
+ NextUntilIterator<Instruction>,
+ PreviousUntilIterator<Instruction> {
/**
* Peek the previous instruction.
diff --git a/src/main/java/com/android/tools/r8/ir/code/PreviousUntilIterator.java b/src/main/java/com/android/tools/r8/ir/code/PreviousUntilIterator.java
new file mode 100644
index 0000000..4d0f790
--- /dev/null
+++ b/src/main/java/com/android/tools/r8/ir/code/PreviousUntilIterator.java
@@ -0,0 +1,26 @@
+// 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.ir.code;
+
+import java.util.ListIterator;
+import java.util.function.Predicate;
+
+public interface PreviousUntilIterator<T> extends ListIterator<T> {
+
+ /**
+ * Continue to call {@link #previous} while {@code predicate} tests {@code false}.
+ *
+ * @returns the item that matched the predicate or {@code null} if all items fail the predicate
+ * test
+ */
+ default T previousUntil(Predicate<T> predicate) {
+ while (hasPrevious()) {
+ T item = previous();
+ if (predicate.test(item)) {
+ return item;
+ }
+ }
+ return null;
+ }
+}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/peepholes/PeepholeHelper.java b/src/main/java/com/android/tools/r8/ir/optimize/peepholes/PeepholeHelper.java
index c1e911f..d656455 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/peepholes/PeepholeHelper.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/peepholes/PeepholeHelper.java
@@ -8,6 +8,7 @@
import com.android.tools.r8.ir.code.InstructionListIterator;
import com.android.tools.r8.ir.code.StackValues;
import com.android.tools.r8.ir.code.Value;
+import java.util.List;
import java.util.function.Predicate;
public class PeepholeHelper {
@@ -19,18 +20,6 @@
&& (t.outValue() == null || !t.outValue().hasLocalInfo());
}
- public static void swapNextTwoInstructions(InstructionListIterator it) {
- assert it.hasNext();
- Instruction moveForward = it.next();
- Instruction moveBack = it.next();
- it.set(moveForward);
- // Two calls to previous is needed because the iterator moves between elements.
- it.previous();
- it.previous();
- it.set(moveBack);
- it.next();
- }
-
public static void resetNext(InstructionListIterator it, int count) {
for (int i = 0; i < count; i++) {
it.previous();
@@ -62,4 +51,23 @@
}
return count;
}
+
+ public static void moveInstructionsUpToCurrentPosition(
+ InstructionListIterator it, List<Instruction> instructions) {
+ assert !instructions.isEmpty();
+ for (Instruction instruction : instructions) {
+ for (Value inValue : instruction.inValues()) {
+ inValue.addUser(instruction);
+ }
+ it.add(instruction);
+ }
+ Instruction current = it.nextUntil(i -> i == instructions.get(0));
+ for (int i = 0; i < instructions.size(); i++) {
+ assert current == instructions.get(i);
+ it.removeOrReplaceByDebugLocalRead();
+ current = it.next();
+ }
+ it.previousUntil(i -> i == instructions.get(instructions.size() - 1));
+ it.next();
+ }
}
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/peepholes/StoreLoadToDupStorePeephole.java b/src/main/java/com/android/tools/r8/ir/optimize/peepholes/StoreLoadToDupStorePeephole.java
index a0f1c8c..58c50d8 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/peepholes/StoreLoadToDupStorePeephole.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/peepholes/StoreLoadToDupStorePeephole.java
@@ -11,7 +11,6 @@
import com.android.tools.r8.ir.code.StackValue;
import com.android.tools.r8.ir.code.StackValues;
import com.android.tools.r8.ir.code.Store;
-import com.android.tools.r8.ir.code.Value;
import java.util.List;
/**
@@ -45,63 +44,40 @@
if (match == null) {
return false;
}
- Store oldStore = storeExp.get(match).asStore();
+ Store store = storeExp.get(match).asStore();
Load load = loadExp.get(match).asLoad();
- if (load.src() != oldStore.outValue() || oldStore.outValue().numberOfAllUsers() <= 1) {
+ if (load.src() != store.outValue() || store.outValue().numberOfAllUsers() <= 1) {
return false;
}
List<Instruction> dups = dupsExp.get(match);
- Instruction lastDup = dups.isEmpty() ? null : dups.get(dups.size() - 1);
- StackValue oldStoreSrc = (StackValue) oldStore.src();
- Value loadOut = load.swapOutValue(null);
- if (lastDup == null) {
- // Replace
- // Store v0 <- oldStoreSrc
- // Load loadOut <- v0
- // with
- // Dup [loadOut, newStoreSrc] <- oldStoreSrc
- // Store v0 <- newStoreSrc
-
- StackValue newStoreSrc = oldStoreSrc.duplicate(oldStoreSrc.getHeight() + 1);
- Dup dup = new Dup((StackValue) loadOut, newStoreSrc, oldStoreSrc);
- dup.setPosition(oldStore.getPosition());
- oldStore.replaceValue(0, newStoreSrc);
- it.add(dup);
- PeepholeHelper.resetPrevious(it, 2);
- it.removeOrReplaceByDebugLocalRead();
+ StackValue oldStoreSrc = (StackValue) store.src();
+ StackValue dupIn;
+ StackValue lastOut;
+ // Store and load may be in two different basic blocks (see b/122445224) so move all
+ // instructions to the block with the store.
+ if (dups.isEmpty()) {
+ lastOut = (StackValue) load.outValue();
+ dupIn = oldStoreSrc;
} else {
- // Replace
- // Store storeOut <- oldStoreSrc
- // Load loadOut <- storeOut
- // Dup [sa, sb] <- loadOut
- // (Dup/Dup2)*
- // Dup/Dup2 [..., topAfterDups] <- [...] # lastDup
- // with
- // Dup [sa, sb] <- oldStoreSrc
- // (Dup/Dup2)*
- // Dup/Dup2 [..., newTopAfterDups] <- [...] # lastDup
- // Dup [topAfterDups, newStoreSrc] <- newTopAfterDups
- // Store storeOut <- newStoreSrc
- Value storeOut = oldStore.swapOutValue(null);
- it.removeOrReplaceByDebugLocalRead(); // Remove Store.
- it.next();
- it.removeOrReplaceByDebugLocalRead(); // Remove Load.
- assert dups.get(0).isDup() && dups.get(0).inValues().get(0) == loadOut;
+ assert dups.get(0).isDup() && dups.get(0).inValues().get(0) == load.outValue();
dups.get(0).replaceValue(0, oldStoreSrc);
- StackValues lastDupOut = (StackValues) lastDup.outValue();
- StackValue topAfterDups = lastDupOut.getStackValues()[lastDupOut.getStackValues().length - 1];
- StackValue newTopAfterDups = topAfterDups.duplicate(topAfterDups.getHeight());
- lastDupOut.getStackValues()[lastDupOut.getStackValues().length - 1] = newTopAfterDups;
- newTopAfterDups.definition = lastDup;
- StackValue newStoreSrc = oldStoreSrc.duplicate(topAfterDups.getHeight() + 1);
- Dup newDup = new Dup(topAfterDups, newStoreSrc, newTopAfterDups);
- newDup.setPosition(lastDup.getPosition());
- PeepholeHelper.resetPrevious(it, dups.size());
- it.add(newDup);
- Store newStore = new Store(storeOut, newStoreSrc);
- newStore.setPosition(lastDup.getPosition());
- it.add(newStore);
+ PeepholeHelper.moveInstructionsUpToCurrentPosition(it, dups);
+ StackValue[] lastDupOutValues =
+ ((StackValues) dups.get(dups.size() - 1).outValue()).getStackValues();
+ lastOut = lastDupOutValues[lastDupOutValues.length - 1];
+ dupIn = lastOut;
}
+ StackValue newLastOut = lastOut.duplicate(lastOut.getHeight());
+ StackValue newStoreSrc = lastOut.duplicate(lastOut.getHeight() + 1);
+ lastOut.replaceUsers(newLastOut);
+ assert !load.outValue().isUsed();
+ Dup dup = new Dup(newLastOut, newStoreSrc, dupIn);
+ dup.setPosition(store.getPosition());
+ it.add(dup);
+ store.replaceValue(0, newStoreSrc);
+ it.nextUntil(i -> i == load);
+ it.removeOrReplaceByDebugLocalRead();
+ PeepholeHelper.resetNext(it, dups.size() + 1);
return true;
}
diff --git a/src/test/java/com/android/tools/r8/cf/CloserTestRunner.java b/src/test/java/com/android/tools/r8/cf/CloserTestRunner.java
index a59ee83..cac41cc 100644
--- a/src/test/java/com/android/tools/r8/cf/CloserTestRunner.java
+++ b/src/test/java/com/android/tools/r8/cf/CloserTestRunner.java
@@ -35,7 +35,6 @@
* If we produce something like this, the JVM verifier will throw a VerifyError on @bci: 3 since we
* have not stored CloserTest in locals[1] (that happens in @bci 5), as described in the
* StackMapTable. This is because the exception handler starts at @bci 3 and not later.
- * TODO(b/122445224)
*/
public class CloserTestRunner extends TestBase {
@@ -50,6 +49,6 @@
.enableInliningAnnotations()
.compile()
.run(CloserTest.class)
- .assertFailure();
+ .assertSuccess();
}
}
