Fix assertion error in register allocator with one-to-many moves
This fixes a harmless assertion error that does not account for the fact that the same register may be moved to more than one register in the input parallel move set.
This situation can arise when a value flows into a phi, meanwhile the value itself dominates the phi (e.g., the value is declared outside a loop, and the phi is declared inside the loop).
Fixes: b/418568424
Bug: b/417403930
Change-Id: I51f991980f49d2ce833e12971ff19263300537a7
diff --git a/src/main/java/com/android/tools/r8/ir/regalloc/RegisterMoveScheduler.java b/src/main/java/com/android/tools/r8/ir/regalloc/RegisterMoveScheduler.java
index b71ea38..eca043d 100644
--- a/src/main/java/com/android/tools/r8/ir/regalloc/RegisterMoveScheduler.java
+++ b/src/main/java/com/android/tools/r8/ir/regalloc/RegisterMoveScheduler.java
@@ -258,17 +258,19 @@
assert move.isDestUsedAsTemporaryForSelf(this);
return;
}
- assert verifyMovesHaveDifferentSources(movesWithSrc);
+ IntSet seenSrcs = new IntArraySet();
for (RegisterMove moveWithSrc : movesWithSrc) {
// TODO(b/375147902): Maybe seed the move scheduler with a set of registers known to be free
// at this point.
- int register = takeFreeRegister(moveWithSrc.isWide());
- Value to = new FixedRegisterValue(moveWithSrc.type, register);
- Value from = new FixedRegisterValue(moveWithSrc.type, valueMap.get(moveWithSrc.src));
- Move instruction = new Move(to, from);
- instruction.setPosition(position);
- insertAt.add(instruction);
- valueMap.put(moveWithSrc.src, register);
+ if (seenSrcs.add(moveWithSrc.src)) {
+ int register = takeFreeRegister(moveWithSrc.isWide());
+ Value to = new FixedRegisterValue(moveWithSrc.type, register);
+ Value from = new FixedRegisterValue(moveWithSrc.type, valueMap.get(moveWithSrc.src));
+ Move instruction = new Move(to, from);
+ instruction.setPosition(position);
+ insertAt.add(instruction);
+ valueMap.put(moveWithSrc.src, register);
+ }
}
}
@@ -378,14 +380,6 @@
return nextTempRegister++;
}
- private boolean verifyMovesHaveDifferentSources(List<RegisterMove> movesWithSrc) {
- IntSet seen = new IntOpenHashSet();
- for (RegisterMove move : movesWithSrc) {
- assert seen.add(move.src);
- }
- return true;
- }
-
private RegisterMove pickMoveToUnblock(TreeSet<RegisterMove> moves) {
// Pick a non-wide move to unblock if possible.
Iterable<RegisterMove> eligible =
diff --git a/src/test/java/com/android/tools/r8/ir/regalloc/B418568424Test.java b/src/test/java/com/android/tools/r8/ir/regalloc/B418568424Test.java
index 4558499..41b49c2 100644
--- a/src/test/java/com/android/tools/r8/ir/regalloc/B418568424Test.java
+++ b/src/test/java/com/android/tools/r8/ir/regalloc/B418568424Test.java
@@ -6,8 +6,6 @@
import com.android.tools.r8.TestBase;
import com.android.tools.r8.TestParameters;
import com.android.tools.r8.TestParametersCollection;
-import com.android.tools.r8.utils.AndroidApiLevel;
-import com.android.tools.r8.utils.codeinspector.AssertUtils;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
@@ -27,16 +25,12 @@
@Test
public void test() throws Exception {
- AssertUtils.assertFailsCompilationIf(
- parameters.isDexRuntime()
- && parameters.getApiLevel().isGreaterThanOrEqualTo(AndroidApiLevel.M),
- () ->
- testForR8(parameters)
- .addInnerClasses(getClass())
- .addKeepClassAndMembersRules(Main.class)
- .compile()
- .run(parameters.getRuntime(), Main.class)
- .assertSuccess());
+ testForR8(parameters)
+ .addInnerClasses(getClass())
+ .addKeepClassAndMembersRules(Main.class)
+ .compile()
+ .run(parameters.getRuntime(), Main.class)
+ .assertSuccess();
}
static class Main {
diff --git a/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java b/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java
index de6c3c6..afb6aec 100644
--- a/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java
+++ b/src/test/java/com/android/tools/r8/ir/regalloc/RegisterMoveSchedulerTest.java
@@ -616,6 +616,30 @@
assertEquals(2, scheduler.getUsedTempRegisters());
}
+ /**
+ * Regression test for the assertion error in b/417403930. In this test the parallel move set
+ * contains two moves to different destination registers for the same value. This can happen when
+ * a value flows into a phi meanwhile the value dominates the phi itself.
+ */
+ @Test
+ public void reproDifferentMovesWithSameSources() {
+ CollectMovesIterator moves = new CollectMovesIterator();
+ int temp = 42;
+ RegisterMoveScheduler scheduler = new RegisterMoveScheduler(moves, temp);
+ scheduler.addMove(new RegisterMove(2, 3, TypeElement.getLong()));
+ scheduler.addMove(new RegisterMove(4, 7, TypeElement.getInt()));
+ scheduler.addMove(new RegisterMove(6, 3, TypeElement.getLong()));
+ scheduler.schedule();
+ assertEquals(4, moves.size());
+ // We unblock the move 4 <- 7 by moving the long in register 3 to a temporary register. We then
+ // emit 4 <- 7 and the remaining moves which are now unblocked.
+ assertEquals("42 <- 3", toString(moves.get(0)));
+ assertEquals("4 <- 7", toString(moves.get(1)));
+ assertEquals("2 <- 42", toString(moves.get(2)));
+ assertEquals("6 <- 2", toString(moves.get(3)));
+ assertEquals(2, scheduler.getUsedTempRegisters());
+ }
+
// Debugging aid.
private void printMoves(List<Instruction> moves) {
System.out.println("Generated moves:");
