chore: SSA Refactor IR Review [DO NOT MERGE]

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

@@ -55,8 +55,9 @@ pub(crate) struct DataFlowGraph {
     intrinsics: HashMap<Intrinsic, ValueId>,
 
     /// Contains each foreign function that has been imported into the current function.
-    /// This map is used to ensure that the ValueId for any given foreign functôn is always
+    /// This map is used to ensure that the ValueId for any given foreign function is always
     /// represented by only 1 ValueId within this function.
+    /// TODO: can we merge this into intrinsics?
     foreign_functions: HashMap<String, ValueId>,
 
     /// Function signatures of external methods
@@ -68,6 +69,7 @@ pub(crate) struct DataFlowGraph {
     /// Debugging information about which `ValueId`s have had their underlying `Value` substituted
     /// for that of another. This information is purely used for printing the SSA, and has no
     /// material effect on the SSA itself.
+    /// TODO: Is this true? How are we doing aliasing?
     replaced_value_ids: HashMap<ValueId, ValueId>,
 }
 
@@ -103,6 +105,8 @@ impl DataFlowGraph {
     /// block's id.
     ///
     /// The pairs are order by id, which is not guaranteed to be meaningful.
+    /// TODO: do we need to order by reverse post order? Is this useful? Check brillig
+    /// TODO: ie do we need to get all successors/predecessors first?
     pub(crate) fn basic_blocks_iter(
         &self,
     ) -> impl ExactSizeIterator<Item = (BasicBlockId, &BasicBlock)> {
@@ -115,6 +119,7 @@ impl DataFlowGraph {
     }
 
     /// Inserts a new instruction into the DFG.
+    ///
     /// This does not add the instruction to the block.
     /// Returns the id of the new instruction and its results.
     ///
@@ -132,13 +137,18 @@ impl DataFlowGraph {
     }
 
     /// Inserts a new instruction at the end of the given block and returns its results
+    ///
+    /// TODO: This is doing more than just inserting an instruction and its results
     pub(crate) fn insert_instruction_and_results(
         &mut self,
         instruction: Instruction,
         block: BasicBlockId,
         ctrl_typevars: Option<Vec<Type>>,
     ) -> InsertInstructionResult {
         use InsertInstructionResult::*;
+        // TODO: Is simplify still inserting the instruction?
+        // TODO: or are we assuming that simplify will always return a ValueId that
+        // TODO: has already been inserted?
         match instruction.simplify(self, block) {
             SimplifyResult::SimplifiedTo(simplification) => SimplifiedTo(simplification),
             SimplifyResult::Remove => InstructionRemoved,

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

@@ -13,7 +13,7 @@ use super::{
 /// Dominator tree node. We keep one of these per reachable block.
 #[derive(Clone, Default)]
 struct DominatorTreeNode {
-    /// The block's idx in the control flow graph's reverse post-order
+    /// The block's index 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.
@@ -59,7 +59,7 @@ impl DominatorTree {
     /// 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.
+    /// entry to `block_id` must go through that block.
     ///
     /// The *immediate dominator* is the dominator that is closest to `block_id`. All other
     /// dominators also dominate the immediate dominator.

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

@@ -13,6 +13,7 @@ use super::{
 /// The FunctionInserter can be used to help modify existing Functions
 /// and map old values to new values after re-inserting optimized versions
 /// of old instructions.
+/// TODO: optimized meaning calls to the simplify method et al?
 pub(crate) struct FunctionInserter<'f> {
     pub(crate) function: &'f mut Function,
 

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

@@ -266,6 +266,7 @@ impl Instruction {
                 }
             }
             Instruction::Constrain(value) => {
+                // If the constraint value is known to be true, then we can remove the constraint instruction.
                 if let Some(constant) = dfg.get_numeric_constant(*value) {
                     if constant.is_one() {
                         return Remove;
@@ -277,10 +278,13 @@ impl Instruction {
                 let array = dfg.get_array_constant(*array);
                 let index = dfg.get_numeric_constant(*index);
 
+                // If we are getting a value from a constant array with a constant index
+                // Then we can substitute that value in place of the array_get instruction.
+                // TODO: what is a non-constant array?
                 if let (Some((array, _)), Some(index)) = (array, index) {
                     let index =
                         index.try_to_u64().expect("Expected array index to fit in u64") as usize;
-
+                    // TODO: Where do we check for Out of Bounds(OOB) error?
                     if index < array.len() {
                         return SimplifiedTo(array[index]);
                     }
@@ -379,6 +383,7 @@ fn simplify_call(func: ValueId, arguments: &[ValueId], dfg: &mut DataFlowGraph)
         arguments.iter().map(|value_id| dfg.get_numeric_constant(*value_id)).collect();
     let constant_args = match constant_args {
         Some(constant_args) => constant_args,
+        // Not a big fan of adding SimplifyResult to the namespace because of the None variant which looks like Option::None
         Option::None => return None,
     };
     match intrinsic {
@@ -393,6 +398,7 @@ fn simplify_call(func: ValueId, arguments: &[ValueId], dfg: &mut DataFlowGraph)
             let limb_count = constant_args[2].to_u128() as u32;
             SimplifiedTo(constant_to_radix(endian, field, radix, limb_count, dfg))
         }
+        // TODO: Can simplify here too!
         Intrinsic::BlackBox(_) | Intrinsic::Println | Intrinsic::Sort => None,
     }
 }
@@ -421,7 +427,7 @@ fn constant_to_radix(
     }
 
     // For legacy reasons (see #617) the to_radix interface supports 256 bits even though
-    // FieldElement::max_num_bits() is only 254 bits. Any limbs beyond the specified count
+    // FieldElement::max_num_bits() is only 254 bits for bn254. Any limbs beyond the specified count
     // become zero padding.
     let max_decomposable_bits: u32 = 256;
     let limb_count_with_padding = max_decomposable_bits / bit_size;
@@ -442,7 +448,7 @@ pub(crate) enum InstructionResultType {
     Known(Type),
 
     /// The result type of this function is unknown and separate from its operand types.
-    /// This occurs for function calls and load operations.
+    /// TODO:WHY? This occurs for function calls and load operations.
     Unknown,
 
     /// This instruction does not return any results.

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

@@ -52,7 +52,7 @@ pub(crate) enum Value {
     Intrinsic(Intrinsic),
 
     /// This Value refers to an external function in the IR.
-    /// ForeignFunction's always have the type Type::Function and have simlar semantics to Function,
+    /// ForeignFunction's always have the type Type::Function and have similar semantics to Function,
     /// other than generating different backend operations and being only accessible through Brillig.
     ForeignFunction(String),
 }