Simplify parallel-moves state machine (#158)

* Simplify parallel-moves state machine

Using a three-state enum instead of visited/onstack bools makes the
state transitions easier to reason about.

Also note that the behavior was the same for two cases ("there is no
next move", and "the next move has already been emitted"), so I merged
them.

This commit also cuts memory usage in half for the must_come_before
array and for the visited/onstack arrays.

* Simplify cycle-handling in parallel moves

The loop as previously written was guaranteed to run at least once and
the first iteration was quite different than later iterations, so peel
off the first iteration.

That in turn made it clear that scratch_src is always set, which allowed
simplifying emitting the final scratch-register move.

One thing which was tricky is that if the existing implementation was
handed a move of an allocation into itself, then it would previously
emit a move from that allocation to the scratch register followed by a
move from the scratch register back to the allocation. Peeling the first
iteration out of the loop made it a little trickier to ensure that the
loop didn't run at all in that case, but also emitting any moves is a
silly result.

So instead I've filtered out self-moves before starting the depth-first
traversal. Ideally we'd do this before checking whether the sources
overlap the destinations, since if the only overlaps are self-moves then
we can just return the original move list. However it should happen
after the debug assertion that there is at most one writer to each
allocation, since the input is malformed if there's both a self-move and
another move to the same allocation. After #157 lands this can be
re-ordered.

Because I'm filtering out self-moves and then running the depth-first
traversal, if all of the moves were self-moves then we may try to
traverse an empty graph. This was easily fixed by not blindly pushing
indices onto the stack, and instead letting the existing check for
un-visited moves fire because the stack is empty.

* Use more idiomatic while-let loops

Author: jameysharp

src/moves.rs

@@ -116,70 +116,63 @@ impl<T: Clone + Copy + Default + PartialEq> ParallelMoves<T> {
             }
         }
 
+        // Moving an allocation into itself is technically a cycle but
+        // should have no effect, as long as there are no other writes
+        // into that destination.
+        self.parallel_moves.retain(|&mut (src, dst, _)| src != dst);
+
         // Construct a mapping from move indices to moves they must
         // come before. Any given move must come before a move that
         // overwrites its destination; we have moves sorted by dest
         // above so we can efficiently find such a move, if any.
-        let mut must_come_before: SmallVec<[Option<usize>; 16]> =
-            smallvec![None; self.parallel_moves.len()];
-        for (i, &(src, _, _)) in self.parallel_moves.iter().enumerate() {
-            if let Ok(move_to_dst_idx) = self
-                .parallel_moves
-                .binary_search_by_key(&src, |&(_, dst, _)| dst)
-            {
-                must_come_before[i] = Some(move_to_dst_idx);
-            }
-        }
+        const NONE: usize = usize::MAX;
+        let must_come_before: SmallVec<[usize; 16]> = self
+            .parallel_moves
+            .iter()
+            .map(|&(src, _, _)| {
+                self.parallel_moves
+                    .binary_search_by_key(&src, |&(_, dst, _)| dst)
+                    .unwrap_or(NONE)
+            })
+            .collect();
 
         // Do a simple stack-based DFS and emit moves in postorder,
         // then reverse at the end for RPO. Unlike Tarjan's SCC
         // algorithm, we can emit a cycle as soon as we find one, as
         // noted above.
+        #[derive(Clone, Copy, Debug, Eq, PartialEq)]
+        enum State {
+            /// Not on stack, not visited
+            ToDo,
+            /// On stack, not yet visited
+            Pending,
+            /// Visited
+            Done,
+        }
         let mut ret: MoveVec<T> = smallvec![];
         let mut stack: SmallVec<[usize; 16]> = smallvec![];
-        let mut visited: SmallVec<[bool; 16]> = smallvec![false; self.parallel_moves.len()];
-        let mut onstack: SmallVec<[bool; 16]> = smallvec![false; self.parallel_moves.len()];
+        let mut state: SmallVec<[State; 16]> = smallvec![State::ToDo; self.parallel_moves.len()];
         let mut scratch_used = false;
 
-        stack.push(0);
-        onstack[0] = true;
-        loop {
-            if stack.is_empty() {
-                if let Some(next) = visited.iter().position(|&flag| !flag) {
-                    stack.push(next);
-                    onstack[next] = true;
-                } else {
-                    break;
-                }
-            }
+        while let Some(next) = state.iter().position(|&state| state == State::ToDo) {
+            stack.push(next);
+            state[next] = State::Pending;
 
-            let top = *stack.last().unwrap();
-            visited[top] = true;
-            match must_come_before[top] {
-                None => {
+            while let Some(&top) = stack.last() {
+                debug_assert_eq!(state[top], State::Pending);
+                let next = must_come_before[top];
+                if next == NONE || state[next] == State::Done {
                     ret.push(self.parallel_moves[top]);
-                    onstack[top] = false;
+                    state[top] = State::Done;
                     stack.pop();
                     while let Some(top) = stack.pop() {
                         ret.push(self.parallel_moves[top]);
-                        onstack[top] = false;
+                        state[top] = State::Done;
                     }
-                }
-                Some(next) if visited[next] && !onstack[next] => {
-                    ret.push(self.parallel_moves[top]);
-                    onstack[top] = false;
-                    stack.pop();
-                    while let Some(top) = stack.pop() {
-                        ret.push(self.parallel_moves[top]);
-                        onstack[top] = false;
-                    }
-                }
-                Some(next) if !visited[next] && !onstack[next] => {
+                } else if state[next] == State::ToDo {
                     stack.push(next);
-                    onstack[next] = true;
-                    continue;
-                }
-                Some(next) => {
+                    state[next] = State::Pending;
+                } else {
                     // Found a cycle -- emit a cyclic-move sequence
                     // for the cycle on the top of stack, then normal
                     // moves below it. Recall that these moves will be
@@ -201,29 +194,23 @@ impl<T: Clone + Copy + Default + PartialEq> ParallelMoves<T> {
                     //     C := B
                     //     B := A
                     //     A := scratch
-                    let mut last_dst = None;
-                    let mut scratch_src = None;
-                    while let Some(move_idx) = stack.pop() {
-                        onstack[move_idx] = false;
-                        let (mut src, dst, dst_t) = self.parallel_moves[move_idx];
-                        if last_dst.is_none() {
-                            scratch_src = Some(src);
-                            src = Allocation::none();
-                            scratch_used = true;
-                        } else {
-                            debug_assert_eq!(last_dst.unwrap(), src);
-                        }
-                        ret.push((src, dst, dst_t));
+                    debug_assert_ne!(top, next);
+                    state[top] = State::Done;
+                    stack.pop();
 
-                        last_dst = Some(dst);
+                    let (scratch_src, dst, dst_t) = self.parallel_moves[top];
+                    scratch_used = true;
+
+                    ret.push((Allocation::none(), dst, dst_t));
+                    while let Some(move_idx) = stack.pop() {
+                        state[move_idx] = State::Done;
+                        ret.push(self.parallel_moves[move_idx]);
 
                         if move_idx == next {
                             break;
                         }
                     }
-                    if let Some(src) = scratch_src {
-                        ret.push((src, Allocation::none(), T::default()));
-                    }
+                    ret.push((scratch_src, Allocation::none(), T::default()));
                 }
             }
         }