From e5347d3d7dcebffff43084f1875411fa585481e2 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 19:21:22 +0100
Subject: [PATCH 01/12] feat(ssa refactor): dominator tree

---
 crates/noirc_evaluator/src/ssa_refactor/ir.rs |   1 +
 .../src/ssa_refactor/ir/dom.rs                | 443 ++++++++++++++++++
 .../src/ssa_refactor/ir/dom/post_order.rs     | 136 ++++++
 3 files changed, 580 insertions(+)
 create mode 100644 crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
 create mode 100644 crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir.rs b/crates/noirc_evaluator/src/ssa_refactor/ir.rs
index 1f6cca9157d..423f197a632 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir.rs
@@ -2,6 +2,7 @@ pub(crate) mod basic_block;
 pub(crate) mod cfg;
 pub(crate) mod constant;
 pub(crate) mod dfg;
+pub(crate) mod dom;
 pub(crate) mod function;
 pub(crate) mod instruction;
 pub(crate) mod map;
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
new file mode 100644
index 00000000000..ebc481db6ac
--- /dev/null
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -0,0 +1,443 @@
+use std::{cmp::Ordering, collections::HashMap};
+
+use self::post_order::compute_post_order;
+
+use super::{basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function};
+
+mod post_order;
+
+/// Dominator tree node. We keep one of these per reachable block.
+#[derive(Clone, Default)]
+struct DomNode {
+    /// The block's idx in the control flow graph's reverse post-order
+    reverse_post_order_idx: u32,
+
+    /// The block that immediately dominated that of the node in question.
+    ///
+    /// This will be None for the entry block, which has no immediate dominator.
+    immediate_dominator: Option<BasicBlockId>,
+}
+
+impl DomNode {
+    /// Updates the immediate dominator estimate, returning true if it has changed.
+    ///
+    /// This is used internally as a shorthand during `compute_dominator_tree`.
+    pub(self) fn update_estimate(&mut self, immediate_dominator: BasicBlockId) -> bool {
+        let immediate_dominator = Some(immediate_dominator);
+        if self.immediate_dominator == immediate_dominator {
+            false
+        } else {
+            self.immediate_dominator = immediate_dominator;
+            true
+        }
+    }
+}
+
+/// The dominator tree for a single function.
+pub(crate) struct DominatorTree {
+    /// The nodes of the dominator tree
+    ///
+    /// After dominator tree computation has complete, this will contain a node for every
+    /// reachable block, and no nodes for unreachable blocks.
+    nodes: HashMap<BasicBlockId, DomNode>,
+
+    /// CFG post-order of all reachable blocks. This is cached and exposed as it is useful for
+    /// more than just computing the dominator tree.
+    post_order: Vec<BasicBlockId>,
+}
+
+/// Methods for querying the dominator tree.
+impl DominatorTree {
+    /// Is `block_id` reachable from the entry block?
+    pub(crate) fn is_reachable(&self, block_id: BasicBlockId) -> bool {
+        self.nodes.contains_key(&block_id)
+    }
+
+    /// Get the CFG post-order of blocks that was used to compute the dominator tree.
+    pub(crate) fn cfg_post_order(&self) -> &[BasicBlockId] {
+        &self.post_order
+    }
+
+    /// Returns the immediate dominator of `block_id`.
+    ///
+    /// A block is said to *dominate* `block_id` if all control flow paths from the function
+    /// entry to `block_id` must go through the block.
+    ///
+    /// The *immediate dominator* is the dominator that is closest to `block_id`. All other
+    /// dominators also dominate the immediate dominator.
+    ///
+    /// This returns `None` if `block_id` is not reachable from the entry block, or if it is the
+    /// entry block which has no dominators.
+    pub(crate) fn immediate_dominator(&self, block_id: BasicBlockId) -> Option<BasicBlockId> {
+        match self.nodes.get(&block_id) {
+            Some(node) => node.immediate_dominator,
+            _ => None,
+        }
+    }
+
+    /// Compare two blocks relative to the reverse post-order.
+    pub(crate) fn reverse_post_order_cmp(&self, a: BasicBlockId, b: BasicBlockId) -> Ordering {
+        match (self.nodes.get(&a), self.nodes.get(&b)) {
+            (Some(a), Some(b)) => a.reverse_post_order_idx.cmp(&b.reverse_post_order_idx),
+            _ => unreachable!("Post order for unreachable block is undefined"),
+        }
+    }
+
+    /// Returns `true` if `block_a_id` dominates `block_b_id`.
+    ///
+    /// This means that every control-flow path from the function entry to `block_b_id` must go
+    /// through `block_a_id`.
+    ///
+    /// This function panics if either of the blocks are unreachable.
+    ///
+    /// An instruction is considered to dominate itself.
+    pub(crate) fn dominates(&self, block_a_id: BasicBlockId, mut block_b_id: BasicBlockId) -> bool {
+        // Walk up the dominator tree from "b" until we encounter or pass "a". Doing the
+        // comparison on the reverse post-order may allows to test whether we have passed "a"
+        // without waiting until we reach the root of the tree.
+        loop {
+            match self.reverse_post_order_cmp(block_a_id, block_b_id) {
+                Ordering::Less => {
+                    block_b_id = match self.immediate_dominator(block_b_id) {
+                        Some(immediate_dominator) => immediate_dominator,
+                        None => return false, // a is unreachable, so we climbed past the entry
+                    }
+                }
+                Ordering::Greater => return false,
+                Ordering::Equal => return true,
+            }
+        }
+    }
+
+    /// Compute the common dominator of two basic blocks.
+    ///
+    /// Both basic blocks are assumed to be reachable.
+    fn common_dominator(
+        &self,
+        mut block_a_id: BasicBlockId,
+        mut block_b_id: BasicBlockId,
+    ) -> BasicBlockId {
+        loop {
+            match self.reverse_post_order_cmp(block_a_id, block_b_id) {
+                Ordering::Less => {
+                    // "a" comes before "b" in the reverse post-order. Move "b" up.
+                    block_b_id = self.nodes[&block_b_id]
+                        .immediate_dominator
+                        .expect("Unreachable basic block?");
+                }
+                Ordering::Greater => {
+                    // "b" comes before "a" in the reverse post-order. Move "a" up.
+                    block_a_id = self.nodes[&block_a_id]
+                        .immediate_dominator
+                        .expect("Unreachable basic block?");
+                }
+                Ordering::Equal => break,
+            }
+        }
+
+        debug_assert_eq!(block_a_id, block_b_id, "Unreachable block passed to common_dominator?");
+        block_a_id
+    }
+
+    /// Allocate and compute a dominator tree.
+    pub(crate) fn with_function(func: &Function, cfg: &ControlFlowGraph) -> Self {
+        let post_order = compute_post_order(func);
+        let mut domtree = DominatorTree { nodes: HashMap::new(), post_order };
+        domtree.compute_dominator_tree(cfg);
+        domtree
+    }
+
+    /// Build a dominator tree from a control flow graph using Keith D. Cooper's
+    /// "Simple, Fast Dominator Algorithm."
+    fn compute_dominator_tree(&mut self, cfg: &ControlFlowGraph) {
+        // We'll be iterating over a reverse post-order of the CFG, skipping the entry block.
+        let (entry_block_id, entry_free_post_order) = self
+            .post_order
+            .as_slice()
+            .split_last()
+            .expect("ICE: functions always have at least one block");
+
+        // Do a first pass where we assign reverse post-order indices to all reachable nodes. The
+        // entry block will be the only node with no immediate dominator.
+        self.nodes.insert(
+            *entry_block_id,
+            DomNode { reverse_post_order_idx: 0, immediate_dominator: None },
+        );
+        for (i, &block_id) in entry_free_post_order.iter().rev().enumerate() {
+            // Indices have been displaced by 1 by to the removal of the entry node
+            let reverse_post_order_idx = i as u32 + 1;
+
+            // Due to the nature of the post-order traversal, every node we visit will have at
+            // least one predecessor that has previously been assigned during this loop.
+            let immediate_dominator = self.compute_immediate_dominator(block_id, cfg);
+            self.nodes.insert(
+                block_id,
+                DomNode { immediate_dominator: Some(immediate_dominator), reverse_post_order_idx },
+            );
+        }
+
+        // Now that we have reverse post-order indices for everything and initial immediate
+        // dominator estimates, iterate until convergence.
+        //
+        // If the function is free of irreducible control flow, this will exit after one iteration.
+        let mut changed = true;
+        while changed {
+            changed = false;
+            for &block_id in entry_free_post_order.iter().rev() {
+                let immediate_dominator = self.compute_immediate_dominator(block_id, cfg);
+                changed = self
+                    .nodes
+                    .get_mut(&block_id)
+                    .expect("Assigned in first pass")
+                    .update_estimate(immediate_dominator);
+            }
+        }
+    }
+
+    // Compute the immediate dominator for `block_id` using the pre-calculate immediate dominators
+    // of reachable nodes.
+    fn compute_immediate_dominator(
+        &self,
+        block_id: BasicBlockId,
+        cfg: &ControlFlowGraph,
+    ) -> BasicBlockId {
+        // Get an iterator with just the reachable, already visited predecessors to `block_id`.
+        // Note that during the first pass `node` was pre-populated with all reachable blocks.
+        let mut reachable_preds =
+            cfg.pred_iter(block_id).filter(|pred_id| self.nodes.contains_key(&pred_id));
+
+        // This function isn't called on unreachable blocks or the entry block, so the reverse
+        // post-order will contain at least one predecessor to this block.
+        let mut immediate_dominator =
+            reachable_preds.next().expect("block node must have one reachable predecessor");
+
+        for pred in reachable_preds {
+            immediate_dominator = self.common_dominator(immediate_dominator, pred);
+        }
+
+        immediate_dominator
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::cmp::Ordering;
+
+    use crate::ssa_refactor::ir::{
+        basic_block::BasicBlockId, cfg::ControlFlowGraph, dom::DominatorTree, function::Function,
+        instruction::TerminatorInstruction, map::Id, types::Type,
+    };
+
+    #[test]
+    fn empty() {
+        let func_id = Id::test_new(0);
+        let mut func = Function::new("func".into(), func_id);
+        let block0_id = func.entry_block();
+        func.dfg.set_block_terminator(
+            block0_id,
+            TerminatorInstruction::Return { return_values: vec![] },
+        );
+        let cfg = ControlFlowGraph::with_function(&func);
+        let dom_tree = DominatorTree::with_function(&func, &cfg);
+        assert_eq!(dom_tree.cfg_post_order(), &[block0_id]);
+    }
+
+    // Testing setup for a function with an unreachable block2
+    fn unreachable_node_setup(
+    ) -> (DominatorTree, BasicBlockId, BasicBlockId, BasicBlockId, BasicBlockId) {
+        // func() {
+        //   block0(cond: u1):
+        //     jmpif v0 block2() block3()
+        //   block1():
+        //     jmp block2()
+        //   block2():
+        //     jmp block3()
+        //   block3():
+        //     return ()
+        // }
+        let func_id = Id::test_new(0);
+        let mut func = Function::new("func".into(), func_id);
+        let block0_id = func.entry_block();
+        let block1_id = func.dfg.make_block();
+        let block2_id = func.dfg.make_block();
+        let block3_id = func.dfg.make_block();
+
+        let cond = func.dfg.add_block_parameter(block0_id, Type::unsigned(1));
+        func.dfg.set_block_terminator(
+            block0_id,
+            TerminatorInstruction::JmpIf {
+                condition: cond,
+                then_destination: block2_id,
+                else_destination: block3_id,
+            },
+        );
+        func.dfg.set_block_terminator(
+            block1_id,
+            TerminatorInstruction::Jmp { destination: block2_id, arguments: vec![] },
+        );
+        func.dfg.set_block_terminator(
+            block2_id,
+            TerminatorInstruction::Jmp { destination: block3_id, arguments: vec![] },
+        );
+        func.dfg.set_block_terminator(
+            block3_id,
+            TerminatorInstruction::Return { return_values: vec![] },
+        );
+
+        let cfg = ControlFlowGraph::with_function(&func);
+        let dt = DominatorTree::with_function(&func, &cfg);
+        (dt, block0_id, block1_id, block2_id, block3_id)
+    }
+
+    // Expected dominator tree
+    // block0 {
+    //   block2
+    //   block3
+    // }
+
+    // Dominance matrix
+    // ✓: Row item dominates column item
+    // !: Querying row item's dominance of column item panics. (i.e. invalid)
+    //    b0  b1  b2  b3
+    // b0 ✓   !   ✓   ✓
+    // b1 !   !   !   !
+    // b2     !   ✓
+    // b3     !       ✓
+    // Note that from a local view block 1 dominates blocks 1,2 & 3, but since this block is
+    // unreachable, performing this query indicates an internal compiler error.
+    #[test]
+    fn unreachable_node_asserts() {
+        let (dt, b0, _b1, b2, b3) = unreachable_node_setup();
+
+        assert_eq!(dt.cfg_post_order(), &[b3, b2, b0]);
+
+        assert_eq!(dt.dominates(b0, b0), true);
+        assert_eq!(dt.dominates(b0, b2), true);
+        assert_eq!(dt.dominates(b0, b3), true);
+
+        assert_eq!(dt.dominates(b2, b0), false);
+        assert_eq!(dt.dominates(b2, b2), true);
+        assert_eq!(dt.dominates(b2, b3), false);
+
+        assert_eq!(dt.dominates(b3, b0), false);
+        assert_eq!(dt.dominates(b3, b2), false);
+        assert_eq!(dt.dominates(b3, b3), true);
+    }
+
+    #[test]
+    #[should_panic]
+    fn unreachable_node_panic_b0_b1() {
+        let (dt, b0, b1, _b2, _b3) = unreachable_node_setup();
+        dt.dominates(b0, b1);
+    }
+
+    #[test]
+    #[should_panic]
+    fn unreachable_node_panic_b1_b0() {
+        let (dt, b0, b1, _b2, _b3) = unreachable_node_setup();
+        dt.dominates(b1, b0);
+    }
+
+    #[test]
+    #[should_panic]
+    fn unreachable_node_panic_b1_b1() {
+        let (dt, _b0, b1, _b2, _b3) = unreachable_node_setup();
+        dt.dominates(b1, b1);
+    }
+
+    #[test]
+    #[should_panic]
+    fn unreachable_node_panic_b1_b2() {
+        let (dt, _b0, b1, b2, _b3) = unreachable_node_setup();
+        dt.dominates(b1, b2);
+    }
+
+    #[test]
+    #[should_panic]
+    fn unreachable_node_panic_b1_b3() {
+        let (dt, _b0, b1, _b2, b3) = unreachable_node_setup();
+        dt.dominates(b1, b3);
+    }
+
+    #[test]
+    #[should_panic]
+    fn unreachable_node_panic_b3_b1() {
+        let (dt, _b0, b1, b2, _b3) = unreachable_node_setup();
+        dt.dominates(b2, b1);
+    }
+
+    #[test]
+    fn backwards_layout() {
+        // func {
+        //   block0():
+        //     jmp block2()
+        //   block1():
+        //     return ()
+        //   block2():
+        //     jump block1()
+        // }
+        let func_id = Id::test_new(0);
+        let mut func = Function::new("func".into(), func_id);
+        let block0_id = func.entry_block();
+        let block1_id = func.dfg.make_block();
+        let block2_id = func.dfg.make_block();
+
+        func.dfg.set_block_terminator(
+            block0_id,
+            TerminatorInstruction::Jmp { destination: block2_id, arguments: vec![] },
+        );
+        func.dfg.set_block_terminator(
+            block1_id,
+            TerminatorInstruction::Return { return_values: vec![] },
+        );
+        func.dfg.set_block_terminator(
+            block2_id,
+            TerminatorInstruction::Jmp { destination: block1_id, arguments: vec![] },
+        );
+
+        let cfg = ControlFlowGraph::with_function(&func);
+        let dt = DominatorTree::with_function(&func, &cfg);
+
+        // Expected dominance tree:
+        // block0 {
+        //   block2 {
+        //     block1
+        //   }
+        // }
+
+        assert_eq!(dt.immediate_dominator(block0_id), None);
+        assert_eq!(dt.immediate_dominator(block1_id), Some(block2_id));
+        assert_eq!(dt.immediate_dominator(block2_id), Some(block0_id));
+
+        assert_eq!(dt.reverse_post_order_cmp(block0_id, block0_id), Ordering::Equal);
+        assert_eq!(dt.reverse_post_order_cmp(block0_id, block1_id), Ordering::Less);
+        assert_eq!(dt.reverse_post_order_cmp(block0_id, block2_id), Ordering::Less);
+
+        assert_eq!(dt.reverse_post_order_cmp(block1_id, block0_id), Ordering::Greater);
+        assert_eq!(dt.reverse_post_order_cmp(block1_id, block1_id), Ordering::Equal);
+        assert_eq!(dt.reverse_post_order_cmp(block1_id, block2_id), Ordering::Greater);
+
+        assert_eq!(dt.reverse_post_order_cmp(block2_id, block0_id), Ordering::Greater);
+        assert_eq!(dt.reverse_post_order_cmp(block2_id, block1_id), Ordering::Less);
+        assert_eq!(dt.reverse_post_order_cmp(block2_id, block2_id), Ordering::Equal);
+
+        // Dominance matrix:
+        // ✓: Row item dominates column item
+        //    b0  b1  b2
+        // b0 ✓   ✓   ✓
+        // b1     ✓
+        // b2     ✓   ✓
+
+        assert_eq!(dt.dominates(block0_id, block0_id), true);
+        assert_eq!(dt.dominates(block0_id, block1_id), true);
+        assert_eq!(dt.dominates(block0_id, block2_id), true);
+
+        assert_eq!(dt.dominates(block1_id, block0_id), false);
+        assert_eq!(dt.dominates(block1_id, block1_id), true);
+        assert_eq!(dt.dominates(block1_id, block2_id), false);
+
+        assert_eq!(dt.dominates(block2_id, block0_id), false);
+        assert_eq!(dt.dominates(block2_id, block1_id), true);
+        assert_eq!(dt.dominates(block2_id, block2_id), true);
+    }
+}
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
new file mode 100644
index 00000000000..ea26fcfb92b
--- /dev/null
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
@@ -0,0 +1,136 @@
+use std::collections::HashSet;
+
+use crate::ssa_refactor::ir::{basic_block::BasicBlockId, function::Function};
+
+/// DFT stack state marker for computing the cfg postorder.
+enum Visit {
+    First,
+    Last,
+}
+
+// Calculates the post-order of the
+pub(super) fn compute_post_order(func: &Function) -> Vec<BasicBlockId> {
+    let mut stack = vec![(Visit::First, func.entry_block())];
+    let mut visited: HashSet<BasicBlockId> = HashSet::new();
+    let mut post_order: Vec<BasicBlockId> = Vec::new();
+
+    while let Some((visit, block_id)) = stack.pop() {
+        match visit {
+            Visit::First => {
+                if !visited.contains(&block_id) {
+                    // This is the first time we pop the block, so we need to scan its
+                    // successors and then revisit it.
+                    visited.insert(block_id);
+                    stack.push((Visit::Last, block_id));
+                    // Note: cranelift choses to instead iterate successors backwards for reasons
+                    // that aren't yet clearly relevant to us:
+                    // https://github.com/bytecodealliance/wasmtime/commit/8abfe928d6073d76ebd991a2e991bf8268b4e5a2
+                    for successor_id in func.dfg[block_id].successors() {
+                        if !visited.contains(&successor_id) {
+                            // This not visited check would also be cover by the the next
+                            // iteration, but checking here two saves an iteration per successor.
+                            stack.push((Visit::First, successor_id))
+                        }
+                    }
+                }
+            }
+
+            Visit::Last => {
+                // We've finished all this node's successors.
+                post_order.push(block_id);
+            }
+        }
+    }
+    post_order
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::ssa_refactor::ir::{
+        function::Function, instruction::TerminatorInstruction, map::Id, types::Type,
+    };
+
+    use super::compute_post_order;
+
+    #[test]
+    fn single_block() {
+        let func_id = Id::test_new(0);
+        let func = Function::new("func".into(), func_id);
+        let post_order = compute_post_order(&func);
+        assert_eq!(post_order, [func.entry_block()]);
+    }
+
+    #[test]
+    fn arb_graph_with_unreachable() {
+        // A → B   C
+        // ↓ ↗ ↓   ↓
+        // D ← E → F
+        // Technically correct post-order:
+        // D, F, E, B, A, C
+        // Post-order for our purposes:
+        // F, E, B, D, A
+        // Differences:
+        // - Since C is unreachable we don't want to include it
+        // - Siblings are traversed "backwards" (i.e. "else" before "then") to simply to save on
+        //   an iterator conversion. We could change this if we find a motivation.
+
+        let func_id = Id::test_new(0);
+        let mut func = Function::new("func".into(), func_id);
+        let block_a_id = func.entry_block();
+        let block_b_id = func.dfg.make_block();
+        let block_c_id = func.dfg.make_block();
+        let block_d_id = func.dfg.make_block();
+        let block_e_id = func.dfg.make_block();
+        let block_f_id = func.dfg.make_block();
+
+        // A → B   •
+        // ↓
+        // D   •   •
+        let cond_a = func.dfg.add_block_parameter(block_a_id, Type::unsigned(1));
+        func.dfg.set_block_terminator(
+            block_a_id,
+            TerminatorInstruction::JmpIf {
+                condition: cond_a,
+                // Ordered backwards for test
+                then_destination: block_b_id,
+                else_destination: block_d_id,
+            },
+        );
+        //  •   B   •
+        //  •   ↓   •
+        //  •   E   •
+        func.dfg.set_block_terminator(
+            block_b_id,
+            TerminatorInstruction::Jmp { destination: block_e_id, arguments: vec![] },
+        );
+        // •   •   •
+        //
+        // D ← E → F
+        let cond_e = func.dfg.add_block_parameter(block_e_id, Type::unsigned(1));
+        func.dfg.set_block_terminator(
+            block_e_id,
+            TerminatorInstruction::JmpIf {
+                condition: cond_e,
+                then_destination: block_d_id,
+                else_destination: block_f_id,
+            },
+        );
+        // •   B   •
+        //   ↗
+        // D   •   •
+        func.dfg.set_block_terminator(
+            block_d_id,
+            TerminatorInstruction::Jmp { destination: block_b_id, arguments: vec![] },
+        );
+        // •   •   C
+        // •   •   ↓
+        // •   •   F
+        func.dfg.set_block_terminator(
+            block_c_id,
+            TerminatorInstruction::Jmp { destination: block_f_id, arguments: vec![] },
+        );
+
+        let post_order = compute_post_order(&func);
+        assert_eq!(post_order, [block_f_id, block_e_id, block_b_id, block_d_id, block_a_id]);
+    }
+}

From 765ba8fd82583a0213efec95b4f21b9abe45094e Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 19:24:20 +0100
Subject: [PATCH 02/12] reorder functions

---
 .../src/ssa_refactor/ir/dom.rs                | 60 +++++++++----------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index ebc481db6ac..855bc956008 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -109,36 +109,6 @@ impl DominatorTree {
         }
     }
 
-    /// Compute the common dominator of two basic blocks.
-    ///
-    /// Both basic blocks are assumed to be reachable.
-    fn common_dominator(
-        &self,
-        mut block_a_id: BasicBlockId,
-        mut block_b_id: BasicBlockId,
-    ) -> BasicBlockId {
-        loop {
-            match self.reverse_post_order_cmp(block_a_id, block_b_id) {
-                Ordering::Less => {
-                    // "a" comes before "b" in the reverse post-order. Move "b" up.
-                    block_b_id = self.nodes[&block_b_id]
-                        .immediate_dominator
-                        .expect("Unreachable basic block?");
-                }
-                Ordering::Greater => {
-                    // "b" comes before "a" in the reverse post-order. Move "a" up.
-                    block_a_id = self.nodes[&block_a_id]
-                        .immediate_dominator
-                        .expect("Unreachable basic block?");
-                }
-                Ordering::Equal => break,
-            }
-        }
-
-        debug_assert_eq!(block_a_id, block_b_id, "Unreachable block passed to common_dominator?");
-        block_a_id
-    }
-
     /// Allocate and compute a dominator tree.
     pub(crate) fn with_function(func: &Function, cfg: &ControlFlowGraph) -> Self {
         let post_order = compute_post_order(func);
@@ -217,6 +187,36 @@ impl DominatorTree {
 
         immediate_dominator
     }
+
+    /// Compute the common dominator of two basic blocks.
+    ///
+    /// Both basic blocks are assumed to be reachable.
+    fn common_dominator(
+        &self,
+        mut block_a_id: BasicBlockId,
+        mut block_b_id: BasicBlockId,
+    ) -> BasicBlockId {
+        loop {
+            match self.reverse_post_order_cmp(block_a_id, block_b_id) {
+                Ordering::Less => {
+                    // "a" comes before "b" in the reverse post-order. Move "b" up.
+                    block_b_id = self.nodes[&block_b_id]
+                        .immediate_dominator
+                        .expect("Unreachable basic block?");
+                }
+                Ordering::Greater => {
+                    // "b" comes before "a" in the reverse post-order. Move "a" up.
+                    block_a_id = self.nodes[&block_a_id]
+                        .immediate_dominator
+                        .expect("Unreachable basic block?");
+                }
+                Ordering::Equal => break,
+            }
+        }
+
+        debug_assert_eq!(block_a_id, block_b_id, "Unreachable block passed to common_dominator?");
+        block_a_id
+    }
 }
 
 #[cfg(test)]

From 898bca183de6249ed600bb7fa7829f62c56884d6 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 19:38:22 +0100
Subject: [PATCH 03/12] chore(ssa refactor): use true post order

---
 .../src/ssa_refactor/ir/dom/post_order.rs     | 37 ++++++++++++-------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
index ea26fcfb92b..575882358d4 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
@@ -2,7 +2,7 @@ use std::collections::HashSet;
 
 use crate::ssa_refactor::ir::{basic_block::BasicBlockId, function::Function};
 
-/// DFT stack state marker for computing the cfg postorder.
+/// DFT stack state marker for computing the cfg post-order.
 enum Visit {
     First,
     Last,
@@ -22,10 +22,11 @@ pub(super) fn compute_post_order(func: &Function) -> Vec<BasicBlockId> {
                     // successors and then revisit it.
                     visited.insert(block_id);
                     stack.push((Visit::Last, block_id));
-                    // Note: cranelift choses to instead iterate successors backwards for reasons
-                    // that aren't yet clearly relevant to us:
-                    // https://github.com/bytecodealliance/wasmtime/commit/8abfe928d6073d76ebd991a2e991bf8268b4e5a2
-                    for successor_id in func.dfg[block_id].successors() {
+                    // Stack successors for visiting. Because items are taken from the top of the
+                    // stack, we push the item that's due for a visit first to the top.
+                    for successor_id in
+                        func.dfg[block_id].successors().collect::<Vec<_>>().into_iter().rev()
+                    {
                         if !visited.contains(&successor_id) {
                             // This not visited check would also be cover by the the next
                             // iteration, but checking here two saves an iteration per successor.
@@ -65,14 +66,22 @@ mod tests {
         // A → B   C
         // ↓ ↗ ↓   ↓
         // D ← E → F
-        // Technically correct post-order:
-        // D, F, E, B, A, C
-        // Post-order for our purposes:
-        // F, E, B, D, A
-        // Differences:
-        // - Since C is unreachable we don't want to include it
-        // - Siblings are traversed "backwards" (i.e. "else" before "then") to simply to save on
-        //   an iterator conversion. We could change this if we find a motivation.
+        // (`A` is entry block)
+        // Expected post-order working:
+        // A {
+        //   B {
+        //     E {
+        //       D {
+        //         B (seen)
+        //       } -> push(D)
+        //       F {
+        //       } -> push(F)
+        //     } -> push(E)
+        //   } -> push(B)
+        //   D (seen)
+        // } -> push(A)
+        // Result:
+        // D, F, E, B, A, (C dropped as unreachable)
 
         let func_id = Id::test_new(0);
         let mut func = Function::new("func".into(), func_id);
@@ -131,6 +140,6 @@ mod tests {
         );
 
         let post_order = compute_post_order(&func);
-        assert_eq!(post_order, [block_f_id, block_e_id, block_b_id, block_d_id, block_a_id]);
+        assert_eq!(post_order, [block_d_id, block_f_id, block_e_id, block_b_id, block_a_id]);
     }
 }

From 56331ba677681ba15a735131022d718867d50b6e Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 20:03:02 +0100
Subject: [PATCH 04/12] chore(ssa refactor): externalise PostOrder

---
 crates/noirc_evaluator/src/ssa_refactor/ir.rs |  1 +
 .../src/ssa_refactor/ir/dom.rs                | 42 ++++------
 .../ssa_refactor/ir/{dom => }/post_order.rs   | 84 +++++++++++--------
 3 files changed, 65 insertions(+), 62 deletions(-)
 rename crates/noirc_evaluator/src/ssa_refactor/ir/{dom => }/post_order.rs (55%)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir.rs b/crates/noirc_evaluator/src/ssa_refactor/ir.rs
index 423f197a632..d52f380d3d4 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir.rs
@@ -6,6 +6,7 @@ pub(crate) mod dom;
 pub(crate) mod function;
 pub(crate) mod instruction;
 pub(crate) mod map;
+pub(crate) mod post_order;
 pub(crate) mod printer;
 pub(crate) mod types;
 pub(crate) mod value;
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 855bc956008..4270283fa62 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -1,10 +1,6 @@
 use std::{cmp::Ordering, collections::HashMap};
 
-use self::post_order::compute_post_order;
-
-use super::{basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function};
-
-mod post_order;
+use super::{basic_block::BasicBlockId, cfg::ControlFlowGraph, post_order::PostOrder};
 
 /// Dominator tree node. We keep one of these per reachable block.
 #[derive(Clone, Default)]
@@ -40,10 +36,6 @@ pub(crate) struct DominatorTree {
     /// After dominator tree computation has complete, this will contain a node for every
     /// reachable block, and no nodes for unreachable blocks.
     nodes: HashMap<BasicBlockId, DomNode>,
-
-    /// CFG post-order of all reachable blocks. This is cached and exposed as it is useful for
-    /// more than just computing the dominator tree.
-    post_order: Vec<BasicBlockId>,
 }
 
 /// Methods for querying the dominator tree.
@@ -53,11 +45,6 @@ impl DominatorTree {
         self.nodes.contains_key(&block_id)
     }
 
-    /// Get the CFG post-order of blocks that was used to compute the dominator tree.
-    pub(crate) fn cfg_post_order(&self) -> &[BasicBlockId] {
-        &self.post_order
-    }
-
     /// Returns the immediate dominator of `block_id`.
     ///
     /// A block is said to *dominate* `block_id` if all control flow paths from the function
@@ -110,19 +97,17 @@ impl DominatorTree {
     }
 
     /// Allocate and compute a dominator tree.
-    pub(crate) fn with_function(func: &Function, cfg: &ControlFlowGraph) -> Self {
-        let post_order = compute_post_order(func);
-        let mut domtree = DominatorTree { nodes: HashMap::new(), post_order };
-        domtree.compute_dominator_tree(cfg);
+    pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self {
+        let mut domtree = DominatorTree { nodes: HashMap::new() };
+        domtree.compute_dominator_tree(cfg, post_order);
         domtree
     }
 
     /// Build a dominator tree from a control flow graph using Keith D. Cooper's
     /// "Simple, Fast Dominator Algorithm."
-    fn compute_dominator_tree(&mut self, cfg: &ControlFlowGraph) {
+    fn compute_dominator_tree(&mut self, cfg: &ControlFlowGraph, post_order: &PostOrder) {
         // We'll be iterating over a reverse post-order of the CFG, skipping the entry block.
-        let (entry_block_id, entry_free_post_order) = self
-            .post_order
+        let (entry_block_id, entry_free_post_order) = post_order
             .as_slice()
             .split_last()
             .expect("ICE: functions always have at least one block");
@@ -225,7 +210,7 @@ mod tests {
 
     use crate::ssa_refactor::ir::{
         basic_block::BasicBlockId, cfg::ControlFlowGraph, dom::DominatorTree, function::Function,
-        instruction::TerminatorInstruction, map::Id, types::Type,
+        instruction::TerminatorInstruction, map::Id, post_order::PostOrder, types::Type,
     };
 
     #[test]
@@ -238,8 +223,9 @@ mod tests {
             TerminatorInstruction::Return { return_values: vec![] },
         );
         let cfg = ControlFlowGraph::with_function(&func);
-        let dom_tree = DominatorTree::with_function(&func, &cfg);
-        assert_eq!(dom_tree.cfg_post_order(), &[block0_id]);
+        let post_order = PostOrder::with_function(&func);
+        let dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
+        assert_eq!(dom_tree.dominates(block0_id, block0_id), true);
     }
 
     // Testing setup for a function with an unreachable block2
@@ -285,7 +271,8 @@ mod tests {
         );
 
         let cfg = ControlFlowGraph::with_function(&func);
-        let dt = DominatorTree::with_function(&func, &cfg);
+        let post_order = PostOrder::with_function(&func);
+        let dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
         (dt, block0_id, block1_id, block2_id, block3_id)
     }
 
@@ -309,8 +296,6 @@ mod tests {
     fn unreachable_node_asserts() {
         let (dt, b0, _b1, b2, b3) = unreachable_node_setup();
 
-        assert_eq!(dt.cfg_post_order(), &[b3, b2, b0]);
-
         assert_eq!(dt.dominates(b0, b0), true);
         assert_eq!(dt.dominates(b0, b2), true);
         assert_eq!(dt.dominates(b0, b3), true);
@@ -396,7 +381,8 @@ mod tests {
         );
 
         let cfg = ControlFlowGraph::with_function(&func);
-        let dt = DominatorTree::with_function(&func, &cfg);
+        let post_order = PostOrder::with_function(&func);
+        let dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
 
         // Expected dominance tree:
         // block0 {
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
similarity index 55%
rename from crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
rename to crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
index 575882358d4..fd8a342683d 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
@@ -8,57 +8,73 @@ enum Visit {
     Last,
 }
 
-// Calculates the post-order of the
-pub(super) fn compute_post_order(func: &Function) -> Vec<BasicBlockId> {
-    let mut stack = vec![(Visit::First, func.entry_block())];
-    let mut visited: HashSet<BasicBlockId> = HashSet::new();
-    let mut post_order: Vec<BasicBlockId> = Vec::new();
+pub(crate) struct PostOrder(Vec<BasicBlockId>);
 
-    while let Some((visit, block_id)) = stack.pop() {
-        match visit {
-            Visit::First => {
-                if !visited.contains(&block_id) {
-                    // This is the first time we pop the block, so we need to scan its
-                    // successors and then revisit it.
-                    visited.insert(block_id);
-                    stack.push((Visit::Last, block_id));
-                    // Stack successors for visiting. Because items are taken from the top of the
-                    // stack, we push the item that's due for a visit first to the top.
-                    for successor_id in
-                        func.dfg[block_id].successors().collect::<Vec<_>>().into_iter().rev()
-                    {
-                        if !visited.contains(&successor_id) {
-                            // This not visited check would also be cover by the the next
-                            // iteration, but checking here two saves an iteration per successor.
-                            stack.push((Visit::First, successor_id))
+impl PostOrder {
+    pub(crate) fn as_slice(&self) -> &[BasicBlockId] {
+        self.0.as_slice()
+    }
+}
+
+impl PostOrder {
+    /// Allocate and compute a function's block post-order. Pos
+    pub(crate) fn with_function(func: &Function) -> Self {
+        PostOrder(Self::compute_post_order(func))
+    }
+
+    // Computes the post-order of the function by doing a depth-first traversal of the
+    // function's entry block's previously unvisited children. Each block is sequenced according
+    // to when the traversal exits it.
+    fn compute_post_order(func: &Function) -> Vec<BasicBlockId> {
+        let mut stack = vec![(Visit::First, func.entry_block())];
+        let mut visited: HashSet<BasicBlockId> = HashSet::new();
+        let mut post_order: Vec<BasicBlockId> = Vec::new();
+
+        while let Some((visit, block_id)) = stack.pop() {
+            match visit {
+                Visit::First => {
+                    if !visited.contains(&block_id) {
+                        // This is the first time we pop the block, so we need to scan its
+                        // successors and then revisit it.
+                        visited.insert(block_id);
+                        stack.push((Visit::Last, block_id));
+                        // Stack successors for visiting. Because items are taken from the top of the
+                        // stack, we push the item that's due for a visit first to the top.
+                        for successor_id in
+                            func.dfg[block_id].successors().collect::<Vec<_>>().into_iter().rev()
+                        {
+                            if !visited.contains(&successor_id) {
+                                // This not visited check would also be cover by the the next
+                                // iteration, but checking here two saves an iteration per successor.
+                                stack.push((Visit::First, successor_id))
+                            }
                         }
                     }
                 }
-            }
 
-            Visit::Last => {
-                // We've finished all this node's successors.
-                post_order.push(block_id);
+                Visit::Last => {
+                    // We've finished all this node's successors.
+                    post_order.push(block_id);
+                }
             }
         }
+        post_order
     }
-    post_order
 }
 
 #[cfg(test)]
 mod tests {
     use crate::ssa_refactor::ir::{
-        function::Function, instruction::TerminatorInstruction, map::Id, types::Type,
+        function::Function, instruction::TerminatorInstruction, map::Id, post_order::PostOrder,
+        types::Type,
     };
 
-    use super::compute_post_order;
-
     #[test]
     fn single_block() {
         let func_id = Id::test_new(0);
         let func = Function::new("func".into(), func_id);
-        let post_order = compute_post_order(&func);
-        assert_eq!(post_order, [func.entry_block()]);
+        let post_order = PostOrder::with_function(&func);
+        assert_eq!(post_order.0, [func.entry_block()]);
     }
 
     #[test]
@@ -139,7 +155,7 @@ mod tests {
             TerminatorInstruction::Jmp { destination: block_f_id, arguments: vec![] },
         );
 
-        let post_order = compute_post_order(&func);
-        assert_eq!(post_order, [block_d_id, block_f_id, block_e_id, block_b_id, block_a_id]);
+        let post_order = PostOrder::with_function(&func);
+        assert_eq!(post_order.0, [block_d_id, block_f_id, block_e_id, block_b_id, block_a_id]);
     }
 }

From 2eb5e5474bd348ff0730219f49ba35177015f922 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 20:52:43 +0100
Subject: [PATCH 05/12] chore(ssa refactor):  use FunctionBuilder in tests

---
 .../src/ssa_refactor/ir/dom.rs                | 81 +++++++++----------
 .../src/ssa_refactor/ssa_gen/program.rs       |  2 +-
 2 files changed, 37 insertions(+), 46 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 4270283fa62..751aa937f4f 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -208,9 +208,13 @@ impl DominatorTree {
 mod tests {
     use std::cmp::Ordering;
 
-    use crate::ssa_refactor::ir::{
-        basic_block::BasicBlockId, cfg::ControlFlowGraph, dom::DominatorTree, function::Function,
-        instruction::TerminatorInstruction, map::Id, post_order::PostOrder, types::Type,
+    use crate::ssa_refactor::{
+        ir::{
+            basic_block::BasicBlockId, cfg::ControlFlowGraph, dom::DominatorTree,
+            function::Function, instruction::TerminatorInstruction, map::Id, post_order::PostOrder,
+            types::Type,
+        },
+        ssa_builder::FunctionBuilder,
     };
 
     #[test]
@@ -242,33 +246,24 @@ mod tests {
         //     return ()
         // }
         let func_id = Id::test_new(0);
-        let mut func = Function::new("func".into(), func_id);
+        let mut builder = FunctionBuilder::new("func".into(), func_id);
+
+        let cond = builder.add_parameter(Type::unsigned(1));
+        let block1_id = builder.insert_block();
+        let block2_id = builder.insert_block();
+        let block3_id = builder.insert_block();
+
+        builder.terminate_with_jmpif(cond, block2_id, block3_id);
+        builder.switch_to_block(block1_id);
+        builder.terminate_with_jmp(block2_id, vec![]);
+        builder.switch_to_block(block2_id);
+        builder.terminate_with_jmp(block3_id, vec![]);
+        builder.switch_to_block(block3_id);
+        builder.terminate_with_return(vec![]);
+
+        let ssa = builder.finish();
+        let func = ssa.functions.first().unwrap();
         let block0_id = func.entry_block();
-        let block1_id = func.dfg.make_block();
-        let block2_id = func.dfg.make_block();
-        let block3_id = func.dfg.make_block();
-
-        let cond = func.dfg.add_block_parameter(block0_id, Type::unsigned(1));
-        func.dfg.set_block_terminator(
-            block0_id,
-            TerminatorInstruction::JmpIf {
-                condition: cond,
-                then_destination: block2_id,
-                else_destination: block3_id,
-            },
-        );
-        func.dfg.set_block_terminator(
-            block1_id,
-            TerminatorInstruction::Jmp { destination: block2_id, arguments: vec![] },
-        );
-        func.dfg.set_block_terminator(
-            block2_id,
-            TerminatorInstruction::Jmp { destination: block3_id, arguments: vec![] },
-        );
-        func.dfg.set_block_terminator(
-            block3_id,
-            TerminatorInstruction::Return { return_values: vec![] },
-        );
 
         let cfg = ControlFlowGraph::with_function(&func);
         let post_order = PostOrder::with_function(&func);
@@ -362,23 +357,19 @@ mod tests {
         //     jump block1()
         // }
         let func_id = Id::test_new(0);
-        let mut func = Function::new("func".into(), func_id);
+        let mut builder = FunctionBuilder::new("func".into(), func_id);
+        let block1_id = builder.insert_block();
+        let block2_id = builder.insert_block();
+
+        builder.terminate_with_jmp(block2_id, vec![]);
+        builder.switch_to_block(block1_id);
+        builder.terminate_with_return(vec![]);
+        builder.switch_to_block(block2_id);
+        builder.terminate_with_jmp(block1_id, vec![]);
+
+        let ssa = builder.finish();
+        let func = ssa.functions.first().unwrap();
         let block0_id = func.entry_block();
-        let block1_id = func.dfg.make_block();
-        let block2_id = func.dfg.make_block();
-
-        func.dfg.set_block_terminator(
-            block0_id,
-            TerminatorInstruction::Jmp { destination: block2_id, arguments: vec![] },
-        );
-        func.dfg.set_block_terminator(
-            block1_id,
-            TerminatorInstruction::Return { return_values: vec![] },
-        );
-        func.dfg.set_block_terminator(
-            block2_id,
-            TerminatorInstruction::Jmp { destination: block1_id, arguments: vec![] },
-        );
 
         let cfg = ControlFlowGraph::with_function(&func);
         let post_order = PostOrder::with_function(&func);
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs
index 99d49456210..de4f01fc613 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs
@@ -4,7 +4,7 @@ use crate::ssa_refactor::ir::function::Function;
 
 /// Contains the entire SSA representation of the program.
 pub struct Ssa {
-    functions: Vec<Function>,
+    pub functions: Vec<Function>,
 }
 
 impl Ssa {

From 98bfaebe8d969f7400ba8cd126e7c4f3b132a195 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 21:05:08 +0100
Subject: [PATCH 06/12] chore(ssa refactor): add header comments

---
 crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs        | 3 +++
 crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 751aa937f4f..8c8a30d2b79 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -1,3 +1,6 @@
+//! The dominator tree of a function, represented as a hash map of each reachable block id to its
+//! immediate dominator.
+
 use std::{cmp::Ordering, collections::HashMap};
 
 use super::{basic_block::BasicBlockId, cfg::ControlFlowGraph, post_order::PostOrder};
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
index fd8a342683d..f7b3ab9e8a6 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
@@ -1,3 +1,5 @@
+//! The post-order for a given function represented as a vector of basic block ids.
+
 use std::collections::HashSet;
 
 use crate::ssa_refactor::ir::{basic_block::BasicBlockId, function::Function};

From 303c828633d5eab4673a9a92ce77801307bcedc8 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 21:07:47 +0100
Subject: [PATCH 07/12] chore(ssa refactor): clippy

---
 crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs        | 2 +-
 crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 8c8a30d2b79..1d5a23aab05 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -162,7 +162,7 @@ impl DominatorTree {
         // Get an iterator with just the reachable, already visited predecessors to `block_id`.
         // Note that during the first pass `node` was pre-populated with all reachable blocks.
         let mut reachable_preds =
-            cfg.pred_iter(block_id).filter(|pred_id| self.nodes.contains_key(&pred_id));
+            cfg.pred_iter(block_id).filter(|pred_id| self.nodes.contains_key(pred_id));
 
         // This function isn't called on unreachable blocks or the entry block, so the reverse
         // post-order will contain at least one predecessor to this block.
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
index f7b3ab9e8a6..8a19f02c9df 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
@@ -48,7 +48,7 @@ impl PostOrder {
                             if !visited.contains(&successor_id) {
                                 // This not visited check would also be cover by the the next
                                 // iteration, but checking here two saves an iteration per successor.
-                                stack.push((Visit::First, successor_id))
+                                stack.push((Visit::First, successor_id));
                             }
                         }
                     }

From 3159c6b0da5704ff5634a51cc616d86673ace623 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Fri, 28 Apr 2023 21:20:42 +0100
Subject: [PATCH 08/12] chore(ssa refactor): elaborate header comments

---
 crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs        | 3 +++
 crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 1d5a23aab05..344297cda0f 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -1,5 +1,8 @@
 //! The dominator tree of a function, represented as a hash map of each reachable block id to its
 //! immediate dominator.
+//!
+//! Dominator trees are useful for tasks such as identifying back-edges in loop analysis or
+//! calculating dominance frontiers.
 
 use std::{cmp::Ordering, collections::HashMap};
 
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
index 8a19f02c9df..b8f04c51b13 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
@@ -1,4 +1,7 @@
 //! The post-order for a given function represented as a vector of basic block ids.
+//!
+//! This ordering is beneficial to the efficiency of various algorithms, such as those for dead
+//! code elimination and calculating dominance trees.
 
 use std::collections::HashSet;
 

From 3c362a4426d7cd545b8407679e84a97f6cb69bc2 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Tue, 2 May 2023 11:31:10 +0100
Subject: [PATCH 09/12] chore(ssa refactor): domtree/post-order cleanup: -
 Comment tweaks - rm unneeded collect iter into vec - clippy changes -
 DominatorTree::with_function helper - rename DomNode DominatorTreeNode

---
 .../src/ssa_refactor/ir/basic_block.rs        |  4 +-
 .../src/ssa_refactor/ir/dom.rs                | 84 +++++++++++--------
 .../src/ssa_refactor/ir/post_order.rs         |  7 +-
 3 files changed, 54 insertions(+), 41 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/basic_block.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/basic_block.rs
index 8a3f74c4a64..e8b09f518d8 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/basic_block.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/basic_block.rs
@@ -76,7 +76,9 @@ impl BasicBlock {
     /// Iterate over all the successors of the currently block, as determined by
     /// the blocks jumped to in the terminator instruction. If there is no terminator
     /// instruction yet, this will iterate 0 times.
-    pub(crate) fn successors(&self) -> impl ExactSizeIterator<Item = BasicBlockId> {
+    pub(crate) fn successors(
+        &self,
+    ) -> impl ExactSizeIterator<Item = BasicBlockId> + DoubleEndedIterator {
         match &self.terminator {
             Some(TerminatorInstruction::Jmp { destination, .. }) => vec![*destination].into_iter(),
             Some(TerminatorInstruction::JmpIf { then_destination, else_destination, .. }) => {
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 344297cda0f..221560143c2 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -6,11 +6,13 @@
 
 use std::{cmp::Ordering, collections::HashMap};
 
-use super::{basic_block::BasicBlockId, cfg::ControlFlowGraph, post_order::PostOrder};
+use super::{
+    basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function, post_order::PostOrder,
+};
 
 /// Dominator tree node. We keep one of these per reachable block.
 #[derive(Clone, Default)]
-struct DomNode {
+struct DominatorTreeNode {
     /// The block's idx in the control flow graph's reverse post-order
     reverse_post_order_idx: u32,
 
@@ -20,7 +22,7 @@ struct DomNode {
     immediate_dominator: Option<BasicBlockId>,
 }
 
-impl DomNode {
+impl DominatorTreeNode {
     /// Updates the immediate dominator estimate, returning true if it has changed.
     ///
     /// This is used internally as a shorthand during `compute_dominator_tree`.
@@ -41,7 +43,7 @@ pub(crate) struct DominatorTree {
     ///
     /// After dominator tree computation has complete, this will contain a node for every
     /// reachable block, and no nodes for unreachable blocks.
-    nodes: HashMap<BasicBlockId, DomNode>,
+    nodes: HashMap<BasicBlockId, DominatorTreeNode>,
 }
 
 /// Methods for querying the dominator tree.
@@ -62,10 +64,7 @@ impl DominatorTree {
     /// This returns `None` if `block_id` is not reachable from the entry block, or if it is the
     /// entry block which has no dominators.
     pub(crate) fn immediate_dominator(&self, block_id: BasicBlockId) -> Option<BasicBlockId> {
-        match self.nodes.get(&block_id) {
-            Some(node) => node.immediate_dominator,
-            _ => None,
-        }
+        self.nodes.get(&block_id).and_then(|node| node.immediate_dominator)
     }
 
     /// Compare two blocks relative to the reverse post-order.
@@ -102,13 +101,25 @@ impl DominatorTree {
         }
     }
 
-    /// Allocate and compute a dominator tree.
+    /// Allocate and compute a dominator tree from a pre-computed control flow graph and
+    /// post-order counterpart.
     pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self {
         let mut domtree = DominatorTree { nodes: HashMap::new() };
         domtree.compute_dominator_tree(cfg, post_order);
         domtree
     }
 
+    /// Allocate and compute a dominator tree for the given function.
+    ///
+    /// This approach computes the control flow graph and post-order internally and then
+    /// discards them. If either should be retained reuse it is better to instead pre-compute them
+    /// and build the dominator tree with `DominatorTree::with_cfg_and_post_order`.
+    pub(crate) fn with_function(func: &Function) -> Self {
+        let cfg = ControlFlowGraph::with_function(func);
+        let post_order = PostOrder::with_function(func);
+        Self::with_cfg_and_post_order(&cfg, &post_order)
+    }
+
     /// Build a dominator tree from a control flow graph using Keith D. Cooper's
     /// "Simple, Fast Dominator Algorithm."
     fn compute_dominator_tree(&mut self, cfg: &ControlFlowGraph, post_order: &PostOrder) {
@@ -122,10 +133,10 @@ impl DominatorTree {
         // entry block will be the only node with no immediate dominator.
         self.nodes.insert(
             *entry_block_id,
-            DomNode { reverse_post_order_idx: 0, immediate_dominator: None },
+            DominatorTreeNode { reverse_post_order_idx: 0, immediate_dominator: None },
         );
         for (i, &block_id) in entry_free_post_order.iter().rev().enumerate() {
-            // Indices have been displaced by 1 by to the removal of the entry node
+            // Indices have been displaced by 1 by the removal of the entry node
             let reverse_post_order_idx = i as u32 + 1;
 
             // Due to the nature of the post-order traversal, every node we visit will have at
@@ -133,7 +144,10 @@ impl DominatorTree {
             let immediate_dominator = self.compute_immediate_dominator(block_id, cfg);
             self.nodes.insert(
                 block_id,
-                DomNode { immediate_dominator: Some(immediate_dominator), reverse_post_order_idx },
+                DominatorTreeNode {
+                    immediate_dominator: Some(immediate_dominator),
+                    reverse_post_order_idx,
+                },
             );
         }
 
@@ -235,7 +249,7 @@ mod tests {
         let cfg = ControlFlowGraph::with_function(&func);
         let post_order = PostOrder::with_function(&func);
         let dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
-        assert_eq!(dom_tree.dominates(block0_id, block0_id), true);
+        assert!(dom_tree.dominates(block0_id, block0_id));
     }
 
     // Testing setup for a function with an unreachable block2
@@ -271,8 +285,8 @@ mod tests {
         let func = ssa.functions.first().unwrap();
         let block0_id = func.entry_block();
 
-        let cfg = ControlFlowGraph::with_function(&func);
-        let post_order = PostOrder::with_function(&func);
+        let cfg = ControlFlowGraph::with_function(func);
+        let post_order = PostOrder::with_function(func);
         let dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
         (dt, block0_id, block1_id, block2_id, block3_id)
     }
@@ -297,17 +311,17 @@ mod tests {
     fn unreachable_node_asserts() {
         let (dt, b0, _b1, b2, b3) = unreachable_node_setup();
 
-        assert_eq!(dt.dominates(b0, b0), true);
-        assert_eq!(dt.dominates(b0, b2), true);
-        assert_eq!(dt.dominates(b0, b3), true);
+        assert!(dt.dominates(b0, b0));
+        assert!(dt.dominates(b0, b2));
+        assert!(dt.dominates(b0, b3));
 
-        assert_eq!(dt.dominates(b2, b0), false);
-        assert_eq!(dt.dominates(b2, b2), true);
-        assert_eq!(dt.dominates(b2, b3), false);
+        assert!(!dt.dominates(b2, b0));
+        assert!(dt.dominates(b2, b2));
+        assert!(!dt.dominates(b2, b3));
 
-        assert_eq!(dt.dominates(b3, b0), false);
-        assert_eq!(dt.dominates(b3, b2), false);
-        assert_eq!(dt.dominates(b3, b3), true);
+        assert!(!dt.dominates(b3, b0));
+        assert!(!dt.dominates(b3, b2));
+        assert!(dt.dominates(b3, b3));
     }
 
     #[test]
@@ -377,8 +391,8 @@ mod tests {
         let func = ssa.functions.first().unwrap();
         let block0_id = func.entry_block();
 
-        let cfg = ControlFlowGraph::with_function(&func);
-        let post_order = PostOrder::with_function(&func);
+        let cfg = ControlFlowGraph::with_function(func);
+        let post_order = PostOrder::with_function(func);
         let dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
 
         // Expected dominance tree:
@@ -411,16 +425,16 @@ mod tests {
         // b1     ✓
         // b2     ✓   ✓
 
-        assert_eq!(dt.dominates(block0_id, block0_id), true);
-        assert_eq!(dt.dominates(block0_id, block1_id), true);
-        assert_eq!(dt.dominates(block0_id, block2_id), true);
+        assert!(dt.dominates(block0_id, block0_id));
+        assert!(dt.dominates(block0_id, block1_id));
+        assert!(dt.dominates(block0_id, block2_id));
 
-        assert_eq!(dt.dominates(block1_id, block0_id), false);
-        assert_eq!(dt.dominates(block1_id, block1_id), true);
-        assert_eq!(dt.dominates(block1_id, block2_id), false);
+        assert!(!dt.dominates(block1_id, block0_id));
+        assert!(dt.dominates(block1_id, block1_id));
+        assert!(!dt.dominates(block1_id, block2_id));
 
-        assert_eq!(dt.dominates(block2_id, block0_id), false);
-        assert_eq!(dt.dominates(block2_id, block1_id), true);
-        assert_eq!(dt.dominates(block2_id, block2_id), true);
+        assert!(!dt.dominates(block2_id, block0_id));
+        assert!(dt.dominates(block2_id, block1_id));
+        assert!(dt.dominates(block2_id, block2_id));
     }
 }
diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
index b8f04c51b13..984f10a64af 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs
@@ -7,7 +7,7 @@ use std::collections::HashSet;
 
 use crate::ssa_refactor::ir::{basic_block::BasicBlockId, function::Function};
 
-/// DFT stack state marker for computing the cfg post-order.
+/// Depth-first traversal stack state marker for computing the cfg post-order.
 enum Visit {
     First,
     Last,
@@ -45,9 +45,7 @@ impl PostOrder {
                         stack.push((Visit::Last, block_id));
                         // Stack successors for visiting. Because items are taken from the top of the
                         // stack, we push the item that's due for a visit first to the top.
-                        for successor_id in
-                            func.dfg[block_id].successors().collect::<Vec<_>>().into_iter().rev()
-                        {
+                        for successor_id in func.dfg[block_id].successors().rev() {
                             if !visited.contains(&successor_id) {
                                 // This not visited check would also be cover by the the next
                                 // iteration, but checking here two saves an iteration per successor.
@@ -121,7 +119,6 @@ mod tests {
             block_a_id,
             TerminatorInstruction::JmpIf {
                 condition: cond_a,
-                // Ordered backwards for test
                 then_destination: block_b_id,
                 else_destination: block_d_id,
             },

From 849cdf32cbeabc81380b94f0f948732bcfea6035 Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Tue, 2 May 2023 11:33:14 +0100
Subject: [PATCH 10/12] chore(ssa refactor): use domtree helper in tests

---
 crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 221560143c2..c8f6c8a55d1 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -285,9 +285,7 @@ mod tests {
         let func = ssa.functions.first().unwrap();
         let block0_id = func.entry_block();
 
-        let cfg = ControlFlowGraph::with_function(func);
-        let post_order = PostOrder::with_function(func);
-        let dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
+        let dt = DominatorTree::with_function(func);
         (dt, block0_id, block1_id, block2_id, block3_id)
     }
 
@@ -391,9 +389,7 @@ mod tests {
         let func = ssa.functions.first().unwrap();
         let block0_id = func.entry_block();
 
-        let cfg = ControlFlowGraph::with_function(func);
-        let post_order = PostOrder::with_function(func);
-        let dt = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
+        let dt = DominatorTree::with_function(func);
 
         // Expected dominance tree:
         // block0 {

From f8422ef16d9f2080762bc0123d25e50dbaac5a5b Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Tue, 2 May 2023 16:59:52 +0100
Subject: [PATCH 11/12] fix: update usage of cfg.pred_iter to cfg.predecessors

---
 crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index c8f6c8a55d1..0ba0d8a8739 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -179,7 +179,7 @@ impl DominatorTree {
         // Get an iterator with just the reachable, already visited predecessors to `block_id`.
         // Note that during the first pass `node` was pre-populated with all reachable blocks.
         let mut reachable_preds =
-            cfg.pred_iter(block_id).filter(|pred_id| self.nodes.contains_key(pred_id));
+            cfg.predecessors(block_id).filter(|pred_id| self.nodes.contains_key(pred_id));
 
         // This function isn't called on unreachable blocks or the entry block, so the reverse
         // post-order will contain at least one predecessor to this block.

From 24e7913117bfa2c1709b714b4ab3ba34d7306dea Mon Sep 17 00:00:00 2001
From: Joss <joss@aztecprotocol.com>
Date: Tue, 2 May 2023 19:21:58 +0100
Subject: [PATCH 12/12] chore(saa refactor): tidy up test and naming

---
 .../src/ssa_refactor/ir/dom.rs                | 23 ++++++++-----------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
index 0ba0d8a8739..9a0916f62c8 100644
--- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
+++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs
@@ -104,9 +104,9 @@ impl DominatorTree {
     /// Allocate and compute a dominator tree from a pre-computed control flow graph and
     /// post-order counterpart.
     pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self {
-        let mut domtree = DominatorTree { nodes: HashMap::new() };
-        domtree.compute_dominator_tree(cfg, post_order);
-        domtree
+        let mut dom_tree = DominatorTree { nodes: HashMap::new() };
+        dom_tree.compute_dominator_tree(cfg, post_order);
+        dom_tree
     }
 
     /// Allocate and compute a dominator tree for the given function.
@@ -178,16 +178,16 @@ impl DominatorTree {
     ) -> BasicBlockId {
         // Get an iterator with just the reachable, already visited predecessors to `block_id`.
         // Note that during the first pass `node` was pre-populated with all reachable blocks.
-        let mut reachable_preds =
+        let mut reachable_predecessors =
             cfg.predecessors(block_id).filter(|pred_id| self.nodes.contains_key(pred_id));
 
         // This function isn't called on unreachable blocks or the entry block, so the reverse
         // post-order will contain at least one predecessor to this block.
         let mut immediate_dominator =
-            reachable_preds.next().expect("block node must have one reachable predecessor");
+            reachable_predecessors.next().expect("block node must have one reachable predecessor");
 
-        for pred in reachable_preds {
-            immediate_dominator = self.common_dominator(immediate_dominator, pred);
+        for predecessor in reachable_predecessors {
+            immediate_dominator = self.common_dominator(immediate_dominator, predecessor);
         }
 
         immediate_dominator
@@ -230,9 +230,8 @@ mod tests {
 
     use crate::ssa_refactor::{
         ir::{
-            basic_block::BasicBlockId, cfg::ControlFlowGraph, dom::DominatorTree,
-            function::Function, instruction::TerminatorInstruction, map::Id, post_order::PostOrder,
-            types::Type,
+            basic_block::BasicBlockId, dom::DominatorTree, function::Function,
+            instruction::TerminatorInstruction, map::Id, types::Type,
         },
         ssa_builder::FunctionBuilder,
     };
@@ -246,9 +245,7 @@ mod tests {
             block0_id,
             TerminatorInstruction::Return { return_values: vec![] },
         );
-        let cfg = ControlFlowGraph::with_function(&func);
-        let post_order = PostOrder::with_function(&func);
-        let dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);
+        let dom_tree = DominatorTree::with_function(&func);
         assert!(dom_tree.dominates(block0_id, block0_id));
     }