diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs index 393b85fdd2..238d321684 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs @@ -55,8 +55,9 @@ pub(crate) struct DataFlowGraph { intrinsics: HashMap, /// 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, /// 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, } @@ -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 { @@ -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,6 +137,8 @@ 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, @@ -139,6 +146,9 @@ impl DataFlowGraph { ctrl_typevars: Option>, ) -> 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, diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs index 4763ffffbd..a778f90f19 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs +++ b/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. diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs index d85ad46166..c8c75e1801 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/function_inserter.rs +++ b/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, diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index 5a7365e6bf..a84c1ff09a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/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. diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs index fca871ae89..c27d9fcc98 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/value.rs +++ b/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), } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs index 6609252f04..0200a25bdd 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs @@ -29,8 +29,14 @@ use super::{ /// Contrary to the name, this struct has the capacity to build as many /// functions as needed, although it is limited to one function at a time. pub(crate) struct FunctionBuilder { + /// The current function being built. + /// A function builder can be re-used to build different functions. + /// + /// Note: What is the benefit of this vs creating a function builder per function? pub(super) current_function: Function, + /// The current block being built. current_block: BasicBlockId, + /// List of all of the functions that one has built. finished_functions: Vec, } @@ -51,7 +57,7 @@ impl FunctionBuilder { Self { current_function: new_function, current_block, finished_functions: Vec::new() } } - /// Finish the current function and create a new function. + /// Finish the current function by storing it in a `finished_function` vector and create a new function. /// /// A FunctionBuilder can always only work on one function at a time, so care /// should be taken not to finish a function that is still in progress by calling @@ -80,7 +86,7 @@ impl FunctionBuilder { self.new_function_with_type(name, function_id, RuntimeType::Brillig); } - /// Consume the FunctionBuilder returning all the functions it has generated. + /// Consume the `FunctionBuilder` returning all the functions it has generated. pub(crate) fn finish(mut self) -> Ssa { self.finished_functions.push(self.current_function); Ssa::new(self.finished_functions) @@ -111,9 +117,10 @@ impl FunctionBuilder { pub(crate) fn array_constant( &mut self, elements: im::Vector, - element_types: Rc, + // arrays are homogenous, so there is a single type that all elements abide by + element_type: Rc, ) -> ValueId { - self.current_function.dfg.make_array(elements, element_types) + self.current_function.dfg.make_array(elements, element_type) } /// Returns the type of the given value. @@ -122,7 +129,7 @@ impl FunctionBuilder { } /// Insert a new block into the current function and return it. - /// Note that this block is unreachable until another block is set to jump to it. + /// Note: This block is unreachable until another block is set to jump to it. pub(crate) fn insert_block(&mut self) -> BasicBlockId { self.current_function.dfg.make_block() } @@ -139,6 +146,8 @@ impl FunctionBuilder { } /// Inserts a new instruction at the end of the current block and returns its results + /// + /// TODO: Noted elsewhere this is also doing simplification, can we do this separately? tradeoffs? pub(crate) fn insert_instruction( &mut self, instruction: Instruction, @@ -152,44 +161,52 @@ impl FunctionBuilder { } /// Switch to inserting instructions in the given block. + /// /// Expects the given block to be within the same function. If you want to insert - /// instructions into a new function, call new_function instead. + /// instructions into a new function, call `new_function` instead. pub(crate) fn switch_to_block(&mut self, block: BasicBlockId) { self.current_block = block; } - /// Returns the block currently being inserted into + /// Returns the block currently being modified pub(crate) fn current_block(&mut self) -> BasicBlockId { self.current_block } - /// Insert an allocate instruction at the end of the current block, allocating the - /// given amount of field elements. Returns the result of the allocate instruction, + /// Insert an allocate instruction at the end of the current block. + /// Returns the result of the allocate instruction, /// which is always a Reference to the allocated data. pub(crate) fn insert_allocate(&mut self) -> ValueId { + // TODO: Rust has .first on vectors which is confusing because + // TODO that returns Option whereas this returns T and expects there + // TODO to be only one T.Perhaps change this to be `first_and_only` + // TODO or something to not conflate this with .first, but also + // TODO to convey the fact that its the only one; `exactly_one` ? self.insert_instruction(Instruction::Allocate, None).first() } - /// Insert a Load instruction at the end of the current block, loading from the given offset - /// of the given address which should point to a previous Allocate instruction. Note that - /// this is limited to loading a single value. Loading multiple values (such as a tuple) - /// will require multiple loads. - /// 'offset' is in units of FieldElements here. So loading the fourth FieldElement stored in - /// an array will have an offset of 3. - /// Returns the element that was loaded. + /// Insert a Load instruction at the end of the current block. + /// TODO: check previous description was not outdated. + /// TODO: where do we check that `address` is a Reference? pub(crate) fn insert_load(&mut self, address: ValueId, type_to_load: Type) -> ValueId { self.insert_instruction(Instruction::Load { address }, Some(vec![type_to_load])).first() } - /// Insert a Store instruction at the end of the current block, storing the given element - /// at the given address. Expects that the address points somewhere - /// within a previous Allocate instruction. + /// Insert a Store instruction at the end of the current block. + /// Storing the given element at the given address. + /// TODO: check previous description was not outdated + /// TODO: where do we check that `address` is a Reference? + /// TODO: where do we check that `value` is the correct type for `address? + /// TODO: Is `address` still correct terminology, given that we are using stores for mutable variables? pub(crate) fn insert_store(&mut self, address: ValueId, value: ValueId) { self.insert_instruction(Instruction::Store { address, value }, None); } /// Insert a binary instruction at the end of the current block. + /// /// Returns the result of the binary instruction. + /// + /// All binary instructions return one result. pub(crate) fn insert_binary( &mut self, lhs: ValueId, @@ -201,18 +218,21 @@ impl FunctionBuilder { } /// Insert a not instruction at the end of the current block. + /// /// Returns the result of the instruction. pub(crate) fn insert_not(&mut self, rhs: ValueId) -> ValueId { self.insert_instruction(Instruction::Not(rhs), None).first() } /// Insert a cast instruction at the end of the current block. + /// /// Returns the result of the cast instruction. pub(crate) fn insert_cast(&mut self, value: ValueId, typ: Type) -> ValueId { self.insert_instruction(Instruction::Cast(value, typ), None).first() } /// Insert a truncate instruction at the end of the current block. + /// /// Returns the result of the truncate instruction. pub(crate) fn insert_truncate( &mut self, @@ -226,11 +246,14 @@ impl FunctionBuilder { /// Insert a constrain instruction at the end of the current block. pub(crate) fn insert_constrain(&mut self, boolean: ValueId) { - self.insert_instruction(Instruction::Constrain(boolean), None); + let results = self.insert_instruction(Instruction::Constrain(boolean), None).results(); + assert!(results.is_empty(), "constrain instructions do not return any results"); + // TODO: maybe put as a method on results like `.first` } - /// Insert a call instruction at the end of the current block and return - /// the results of the call. + /// Insert a call instruction at the end of the current block. + /// + /// Returns the results of the call. pub(crate) fn insert_call( &mut self, func: ValueId, @@ -240,7 +263,11 @@ impl FunctionBuilder { self.insert_instruction(Instruction::Call { func, arguments }, Some(result_types)).results() } - /// Insert an instruction to extract an element from an array + /// Insert array_get instruction at the end of the current block. + /// + /// This will extract an element from `array` at position `index`. + /// + /// Returns the `ValueId` of the fetched element. pub(crate) fn insert_array_get( &mut self, array: ValueId, @@ -251,7 +278,12 @@ impl FunctionBuilder { self.insert_instruction(Instruction::ArrayGet { array, index }, element_type).first() } - /// Insert an instruction to create a new array with the given index replaced with a new value + /// Insert an array_set instruction to the end of the block. + /// + /// This will create a new array with the given index replaced with a `value`. + /// Note: This will not modify `array`. Arrays are immutable in SSA. + /// + /// Returns the `ValueId` of the newly created array. This will be a reference. pub(crate) fn insert_array_set( &mut self, array: ValueId, @@ -261,13 +293,17 @@ impl FunctionBuilder { self.insert_instruction(Instruction::ArraySet { array, index, value }, None).first() } - /// Terminates the current block with the given terminator instruction + /// Terminates the current block with the given terminator instruction. + /// + /// This is used to denote the block being completed, since basic blocks + /// can only have control flow instructions at the end of the block. fn terminate_block_with(&mut self, terminator: TerminatorInstruction) { self.current_function.dfg.set_block_terminator(self.current_block, terminator); } - /// Terminate the current block with a jmp instruction to jmp to the given - /// block with the given arguments. + /// Terminate the current block with a jump instruction. + /// + /// Jump to the given block with the given arguments. pub(crate) fn terminate_with_jmp( &mut self, destination: BasicBlockId, @@ -276,8 +312,12 @@ impl FunctionBuilder { self.terminate_block_with(TerminatorInstruction::Jmp { destination, arguments }); } - /// Terminate the current block with a jmpif instruction to jmp with the given arguments - /// block with the given arguments. + /// Terminate the current block with a conditional jump instruction. + /// + /// Jump to the `then` block if the condition is true, else jump to + /// the `else` block. + /// + /// TODO: where are the block arguments being supplied? pub(crate) fn terminate_with_jmpif( &mut self, condition: ValueId, @@ -296,25 +336,23 @@ impl FunctionBuilder { self.terminate_block_with(TerminatorInstruction::Return { return_values }); } - /// Returns a ValueId pointing to the given function or imports the function - /// into the current function if it was not already, and returns that ID. + /// Returns a `ValueId` pointing to the given function. + /// + /// If the function has already been imported, its `ValueId` will be returned, + /// else a new `ValueId` will be returned. pub(crate) fn import_function(&mut self, function: FunctionId) -> ValueId { self.current_function.dfg.import_function(function) } - /// Returns a ValueId pointing to the given oracle/foreign function or imports the oracle - /// into the current function if it was not already, and returns that ID. + /// Returns a `ValueId` pointing to the given oracle/foreign function. + /// + /// If the function has already been imported, its `ValueId` will be returned, + /// else a new `ValueId` will be returned. pub(crate) fn import_foreign_function(&mut self, function: &str) -> ValueId { self.current_function.dfg.import_foreign_function(function) } - /// Retrieve a value reference to the given intrinsic operation. - /// Returns None if there is no intrinsic matching the given name. - pub(crate) fn import_intrinsic(&mut self, name: &str) -> Option { - Intrinsic::lookup(name).map(|intrinsic| self.import_intrinsic_id(intrinsic)) - } - - /// Retrieve a value reference to the given intrinsic operation. + /// Return a `ValueId` to the given intrinsic operation. pub(crate) fn import_intrinsic_id(&mut self, intrinsic: Intrinsic) -> ValueId { self.current_function.dfg.import_intrinsic(intrinsic) } @@ -370,6 +408,11 @@ mod tests { // `bits` should be an array of constants [1, 1, 1, 0...]: // let x = 7; // let bits = x.to_le_bits(8); + // + // This is because when we insert an instruction, we are checking to see if that + // instruction can be simplified. When the arguments are constant, we can compute this + // at compile time. + // TODO(NOTE): We can do this for blackbox functions too let func_id = Id::test_new(0); let mut builder = FunctionBuilder::new("func".into(), func_id, RuntimeType::Acir); let one = builder.numeric_constant(FieldElement::one(), Type::bool()); @@ -385,6 +428,7 @@ mod tests { Value::Array { array, .. } => array, _ => panic!(), }; + assert_eq!(array[0], one); assert_eq!(array[1], one); assert_eq!(array[2], one); diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs index 0dc003dd9d..bc3cda29c2 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs @@ -99,10 +99,11 @@ impl<'a> FunctionContext<'a> { ast::Definition::Function(id) => self.get_or_queue_function(*id), ast::Definition::Oracle(name) => self.builder.import_foreign_function(name).into(), ast::Definition::Builtin(name) | ast::Definition::LowLevel(name) => { - match self.builder.import_intrinsic(name) { - Some(builtin) => builtin.into(), + let intrinsic_id = match super::ir::instruction::Intrinsic::lookup(name) { + Some(intrinsic_id) => intrinsic_id, None => panic!("No builtin function named '{name}' found"), - } + }; + self.builder.import_intrinsic_id(intrinsic_id).into() } } } 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 ba98c65850..cd783dc3fc 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs @@ -17,10 +17,17 @@ pub(crate) struct Ssa { impl Ssa { /// Create a new Ssa object from the given SSA functions. /// The first function in this vector is expected to be the main function. + /// TODO: The above condition is somewhat hidden in the API, should we change it so that + /// TODO the caller needs to pass in the main function? + /// pub(crate) fn new(functions: Vec) -> Self { let main_id = functions.first().expect("Expected at least 1 SSA function").id(); + // TODO: why is this sorting being done here? + // TODO: does the max_id need to be the main_id initially + // TODO or do generally just need the max_id from all functions? let mut max_id = main_id; + // Fetch the max_id from all functions let functions = btree_map(functions, |f| { max_id = std::cmp::max(max_id, f.id()); (f.id(), f)