chore(ssa refactor): Add all remaining doc comments to ssa generation…

… pass (#1256)

* Add remaining doc comments

* Update crates/noirc_evaluator/src/ssa_refactor/ir/cfg.rs

Co-authored-by: kevaundray <[email protected]>

* Update crates/noirc_evaluator/src/ssa_refactor/ir/cfg.rs

Co-authored-by: kevaundray <[email protected]>

* Address PR feedback

---------

Co-authored-by: kevaundray <[email protected]>

crates/noirc_evaluator/src/ssa_refactor/ir.rs

@@ -1,5 +1,4 @@
 pub(crate) mod basic_block;
-pub(crate) mod basic_block_visitors;
 pub(crate) mod cfg;
 pub(crate) mod constant;
 pub(crate) mod dfg;

crates/noirc_evaluator/src/ssa_refactor/ir/basic_block.rs

@@ -18,12 +18,6 @@ pub(crate) struct BasicBlock {
     /// Instructions in the basic block.
     instructions: Vec<InstructionId>,
 
-    /// A basic block is considered sealed
-    /// if no further predecessors will be added to it.
-    /// Since only filled blocks can have successors,
-    /// predecessors are always filled.
-    is_sealed: bool,
-
     /// The terminating instruction for the basic block.
     ///
     /// This will be a control flow instruction. This is only
@@ -35,14 +29,20 @@ pub(crate) struct BasicBlock {
 pub(crate) type BasicBlockId = Id<BasicBlock>;
 
 impl BasicBlock {
+    /// Create a new BasicBlock with the given parameters.
+    /// Parameters can also be added later via BasicBlock::add_parameter
     pub(crate) fn new(parameters: Vec<ValueId>) -> Self {
-        Self { parameters, instructions: Vec::new(), is_sealed: false, terminator: None }
+        Self { parameters, instructions: Vec::new(), terminator: None }
     }
 
+    /// Returns the parameters of this block
     pub(crate) fn parameters(&self) -> &[ValueId] {
         &self.parameters
     }
 
+    /// Adds a parameter to this BasicBlock.
+    /// Expects that the ValueId given should refer to a Value::Param
+    /// instance with its position equal to self.parameters.len().
     pub(crate) fn add_parameter(&mut self, parameter: ValueId) {
         self.parameters.push(parameter);
     }
@@ -52,14 +52,23 @@ impl BasicBlock {
         self.instructions.push(instruction);
     }
 
+    /// Retrieve a reference to all instructions in this block.
     pub(crate) fn instructions(&self) -> &[InstructionId] {
         &self.instructions
     }
 
+    /// Sets the terminator instruction of this block.
+    ///
+    /// A properly-constructed block will always terminate with a TerminatorInstruction -
+    /// which either jumps to another block or returns from the current function. A block
+    /// will only have no terminator if it is still under construction.
     pub(crate) fn set_terminator(&mut self, terminator: TerminatorInstruction) {
         self.terminator = Some(terminator);
     }
 
+    /// Returns the terminator of this block.
+    ///
+    /// Once this block has finished construction, this is expected to always be Some.
     pub(crate) fn terminator(&self) -> Option<&TerminatorInstruction> {
         self.terminator.as_ref()
     }

crates/noirc_evaluator/src/ssa_refactor/ir/cfg.rs

@@ -2,7 +2,6 @@ use std::collections::{HashMap, HashSet};
 
 use super::{
     basic_block::{BasicBlock, BasicBlockId},
-    basic_block_visitors,
     function::Function,
 };
 
@@ -33,25 +32,30 @@ impl ControlFlowGraph {
         cfg
     }
 
+    /// Compute all of the edges between each block in the function
     fn compute(&mut self, func: &Function) {
         for (basic_block_id, basic_block) in func.dfg.basic_blocks_iter() {
             self.compute_block(basic_block_id, basic_block);
         }
     }
 
+    /// Compute all of the edges for the current block given
     fn compute_block(&mut self, basic_block_id: BasicBlockId, basic_block: &BasicBlock) {
-        basic_block_visitors::visit_block_succs(basic_block, |dest| {
+        for dest in basic_block.successors() {
             self.add_edge(basic_block_id, dest);
-        });
+        }
     }
 
+    /// Clears out a given block's successors. This also removes the given block from
+    /// being a predecessor of any of its previous successors.
     fn invalidate_block_successors(&mut self, basic_block_id: BasicBlockId) {
         let node = self
             .data
             .get_mut(&basic_block_id)
             .expect("ICE: Attempted to invalidate cfg node successors for non-existent node.");
-        let old_successors = node.successors.clone();
-        node.successors.clear();
+
+        let old_successors = std::mem::take(&mut node.successors);
+
         for successor_id in old_successors {
             self.data
                 .get_mut(&successor_id)
@@ -71,6 +75,7 @@ impl ControlFlowGraph {
         self.compute_block(basic_block_id, basic_block);
     }
 
+    /// Add a directed edge making `from` a predecessor of `to`.
     fn add_edge(&mut self, from: BasicBlockId, to: BasicBlockId) {
         let predecessor_node = self.data.entry(from).or_default();
         assert!(
@@ -87,7 +92,7 @@ impl ControlFlowGraph {
     }
 
     /// Get an iterator over the CFG predecessors to `basic_block_id`.
-    pub(crate) fn pred_iter(
+    pub(crate) fn predecessors(
         &self,
         basic_block_id: BasicBlockId,
     ) -> impl ExactSizeIterator<Item = BasicBlockId> + '_ {
@@ -100,7 +105,7 @@ impl ControlFlowGraph {
     }
 
     /// Get an iterator over the CFG successors to `basic_block_id`.
-    pub(crate) fn succ_iter(
+    pub(crate) fn successors(
         &self,
         basic_block_id: BasicBlockId,
     ) -> impl ExactSizeIterator<Item = BasicBlockId> + '_ {
@@ -133,11 +138,11 @@ mod tests {
     fn jumps() {
         // Build function of form
         // fn func {
-        // block0(cond: u1):
+        //   block0(cond: u1):
         //     jmpif cond, then: block2, else: block1
-        // block1():
+        //   block1():
         //     jmpif cond, then: block1, else: block2
-        // block2():
+        //   block2():
         //     return ()
         // }
         let func_id = Id::test_new(0);
@@ -163,13 +168,13 @@ mod tests {
 
         #[allow(clippy::needless_collect)]
         {
-            let block0_predecessors: Vec<_> = cfg.pred_iter(block0_id).collect();
-            let block1_predecessors: Vec<_> = cfg.pred_iter(block1_id).collect();
-            let block2_predecessors: Vec<_> = cfg.pred_iter(block2_id).collect();
+            let block0_predecessors: Vec<_> = cfg.predecessors(block0_id).collect();
+            let block1_predecessors: Vec<_> = cfg.predecessors(block1_id).collect();
+            let block2_predecessors: Vec<_> = cfg.predecessors(block2_id).collect();
 
-            let block0_successors: Vec<_> = cfg.succ_iter(block0_id).collect();
-            let block1_successors: Vec<_> = cfg.succ_iter(block1_id).collect();
-            let block2_successors: Vec<_> = cfg.succ_iter(block2_id).collect();
+            let block0_successors: Vec<_> = cfg.successors(block0_id).collect();
+            let block1_successors: Vec<_> = cfg.successors(block1_id).collect();
+            let block2_successors: Vec<_> = cfg.successors(block2_id).collect();
 
             assert_eq!(block0_predecessors.len(), 0);
             assert_eq!(block1_predecessors.len(), 2);
@@ -192,13 +197,13 @@ mod tests {
 
         // Modify function to form:
         // fn func {
-        // block0(cond: u1):
+        //   block0(cond: u1):
         //     jmpif cond, then: block1, else: ret_block
-        // block1():
+        //   block1():
         //     jmpif cond, then: block1, else: block2
-        // block2():
+        //   block2():
         //     jmp ret_block()
-        // ret_block():
+        //   ret_block():
         //     return ()
         // }
         let ret_block_id = func.dfg.make_block();
@@ -221,13 +226,13 @@ mod tests {
 
         #[allow(clippy::needless_collect)]
         {
-            let block0_predecessors: Vec<_> = cfg.pred_iter(block0_id).collect();
-            let block1_predecessors: Vec<_> = cfg.pred_iter(block1_id).collect();
-            let block2_predecessors: Vec<_> = cfg.pred_iter(block2_id).collect();
+            let block0_predecessors: Vec<_> = cfg.predecessors(block0_id).collect();
+            let block1_predecessors: Vec<_> = cfg.predecessors(block1_id).collect();
+            let block2_predecessors: Vec<_> = cfg.predecessors(block2_id).collect();
 
-            let block0_successors: Vec<_> = cfg.succ_iter(block0_id).collect();
-            let block1_successors: Vec<_> = cfg.succ_iter(block1_id).collect();
-            let block2_successors: Vec<_> = cfg.succ_iter(block2_id).collect();
+            let block0_successors: Vec<_> = cfg.successors(block0_id).collect();
+            let block1_successors: Vec<_> = cfg.successors(block1_id).collect();
+            let block2_successors: Vec<_> = cfg.successors(block2_id).collect();
 
             assert_eq!(block0_predecessors.len(), 0);
             assert_eq!(block1_predecessors.len(), 2);

crates/noirc_evaluator/src/ssa_refactor/ir/constant.rs

@@ -2,7 +2,7 @@ use acvm::FieldElement;
 
 use super::map::Id;
 
-/// Represents a numeric constant in Ssa. Constants themselves are
+/// Represents a numeric constant in SSA. Constants themselves are
 /// uniqued in the DataFlowGraph and immutable.
 ///
 /// This is just a thin wrapper around FieldElement so that
@@ -12,17 +12,21 @@ use super::map::Id;
 pub(crate) struct NumericConstant(FieldElement);
 
 impl NumericConstant {
+    /// Create a new NumericConstant with the given Field value
     pub(crate) fn new(value: FieldElement) -> Self {
         Self(value)
     }
 
+    /// Retrieves the Field value for this constant
     pub(crate) fn value(&self) -> FieldElement {
         self.0
     }
 }
 
 pub(crate) type NumericConstantId = Id<NumericConstant>;
 
+// Implement some common numeric operations for NumericConstants
+// for convenience so developers do not always have to unwrap them to use them.
 impl std::ops::Add for NumericConstant {
     type Output = NumericConstant;
 

crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs

@@ -15,34 +15,10 @@ use super::{
 use acvm::FieldElement;
 use iter_extended::vecmap;
 
-#[derive(Debug, Default)]
-/// A convenience wrapper to store `Value`s.
-pub(crate) struct ValueList(Vec<Id<Value>>);
-
-impl ValueList {
-    /// Inserts an element to the back of the list and
-    /// returns the `position`
-    pub(crate) fn push(&mut self, value: ValueId) -> usize {
-        self.0.push(value);
-        self.len() - 1
-    }
-
-    /// Returns the number of values in the list.
-    fn len(&self) -> usize {
-        self.0.len()
-    }
-
-    /// Removes all items from the list.
-    fn clear(&mut self) {
-        self.0.clear();
-    }
-
-    /// Returns the ValueId's as a slice.
-    pub(crate) fn as_slice(&self) -> &[ValueId] {
-        &self.0
-    }
-}
-
+/// The DataFlowGraph contains most of the actual data in a function including
+/// its blocks, instructions, and values. This struct is largely responsible for
+/// owning most data in a function and handing out Ids to this data that can be
+/// shared without worrying about ownership.
 #[derive(Debug, Default)]
 pub(crate) struct DataFlowGraph {
     /// All of the instructions in a function
@@ -57,7 +33,7 @@ pub(crate) struct DataFlowGraph {
     /// Currently, we need to define them in a better way
     /// Call instructions require the func signature, but
     /// other instructions may need some more reading on my part
-    results: HashMap<InstructionId, ValueList>,
+    results: HashMap<InstructionId, Vec<ValueId>>,
 
     /// Storage for all of the values defined in this
     /// function.
@@ -243,8 +219,7 @@ impl DataFlowGraph {
         });
 
         // Add value to the list of results for this instruction
-        let actual_res_position = results.push(value_id);
-        assert_eq!(actual_res_position, expected_res_position);
+        results.push(value_id);
         value_id
     }
 
@@ -259,6 +234,7 @@ impl DataFlowGraph {
         self.results.get(&instruction_id).expect("expected a list of Values").as_slice()
     }
 
+    /// Add a parameter to the given block
     pub(crate) fn add_block_parameter(&mut self, block_id: BasicBlockId, typ: Type) -> Id<Value> {
         let block = &mut self.blocks[block_id];
         let position = block.parameters().len();
@@ -267,6 +243,8 @@ impl DataFlowGraph {
         parameter
     }
 
+    /// Insert an instruction at the end of a given block.
+    /// If the block already has a terminator, the instruction is inserted before the terminator.
     pub(crate) fn insert_instruction_in_block(
         &mut self,
         block: BasicBlockId,