From 04a017e8e1b8ce694b89a1d3dafbfe812b5ebbe3 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 19:26:12 +0000 Subject: [PATCH 01/16] move code from Adams and Guillaume's branch for BrilligIR Co-authored-by: guipublic <47281315+guipublic@users.noreply.github.com> Co-authored-by: ludamad --- .../noirc_evaluator/src/brillig/brillig_ir.rs | 116 +++++++++++++++++- 1 file changed, 113 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 0c0696fa130..7f48d1a8e51 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -36,14 +36,18 @@ pub(crate) const BRILLIG_MEMORY_ADDRESSING_BIT_SIZE: u32 = 64; pub(crate) enum ReservedRegisters { /// This register stores the stack pointer. Allocations must be done after this pointer. StackPointer = 0, - /// Number of reserved registers - Len = 1, } impl ReservedRegisters { + /// The number of reserved registers. + /// + /// This is used to offset the general registers + /// which should not overwrite the special register + const NUM_RESERVED_REGISTERS: usize = 1; + /// Returns the length of the reserved registers pub(crate) fn len() -> usize { - ReservedRegisters::Len as usize + Self::NUM_RESERVED_REGISTERS } /// Returns the stack pointer register. This will get used to allocate memory in runtime. @@ -543,6 +547,112 @@ impl BrilligContext { ); self.deallocate_register(zero_register); } + + /// Adds a unresolved external `Call` instruction to the bytecode. + pub(crate) fn add_external_call_instruction(&mut self, func_label: T) { + self.obj.add_unresolved_external_call( + BrilligOpcode::Call { location: 0 }, + func_label.to_string(), + ); + } + + /// Returns the i'th register after the reserved ones + pub(crate) fn register(&self, i: usize) -> RegisterIndex { + RegisterIndex::from(ReservedRegisters::NUM_RESERVED_REGISTERS + i) + } + + /// Saves all of the registers that have been used up until this point. + fn save_all_used_registers(&mut self) -> Vec { + // Save all of the used registers at this point in memory + // because the function call will/may overwrite them. + // + // Note that here it is important that the stack pointer register is at register 0, + // as after the first register save we add to the pointer. + let used_registers: Vec<_> = self.registers.used_registers_iter().collect(); + for register in used_registers.iter() { + self.store_instruction(ReservedRegisters::stack_pointer(), *register); + // Add one to our stack pointer + self.usize_op(ReservedRegisters::stack_pointer(), BinaryIntOp::Add, 1); + } + used_registers + } + + /// Loads all of the registers that have been save by save_all_used_registers. + fn load_all_saved_registers(&mut self, used_registers: &[RegisterIndex]) { + // Load all of the used registers that we saved. + // We do all the reverse operations of save_all_used_registers. + // Iterate our registers in reverse + for register in used_registers.iter().rev() { + // Subtract one from our stack pointer + self.usize_op(ReservedRegisters::stack_pointer(), BinaryIntOp::Sub, 1); + self.load_instruction(*register, ReservedRegisters::stack_pointer()); + } + } + + /// Utility method to perform a binary instruction with a constant value + pub(crate) fn usize_op( + &mut self, + destination: RegisterIndex, + op: BinaryIntOp, + constant: usize, + ) { + let const_register = self.make_constant(Value::from(constant)); + self.binary_instruction( + destination, + destination, + const_register, + // TODO(AD): magic constant + BrilligBinaryOp::Integer { op, bit_size: 64 }, + ); + // Mark as no longer used for this purpose, frees for reuse + self.deallocate_register(const_register); + } + + // Used before a call instruction. + // Save all the registers we have used to the stack. + // Move argument values to the front of the register indices. + pub(crate) fn pre_call_save_registers_prep_args( + &mut self, + arguments: &[RegisterIndex], + ) -> Vec { + // Save all the registers we have used to the stack. + let saved_registers = self.save_all_used_registers(); + + // Move argument values to the front of the registers + // + // This means that the arguments will be in the first `n` registers after + // the special registers which are reserved. + for (i, argument) in arguments.iter().enumerate() { + self.push_opcode(BrilligOpcode::Mov { + destination: self.register(i), + source: *argument, + }); + } + + saved_registers + } + + // Used after a call instruction. + // Move return values to the front of the register indices. + // Load all the registers we have previous saved in save_registers_prep_args. + pub(crate) fn post_call_prep_returns_load_registers( + &mut self, + result_registers: &[RegisterIndex], + saved_registers: &[RegisterIndex], + ) { + // Allocate our result registers and write into them + // We assume the return values of our call are held in 0..num results register indices + for (i, result_register) in result_registers.iter().enumerate() { + self.mov_instruction(*result_register, self.register(i)); + } + + // Restore all the same registers we have, in exact reverse order. + // Note that we have allocated some registers above, which we will not be handling here, + // only restoring registers that were used prior to the call finishing. + // After the call instruction, the stack frame pointer should be back to where we left off, + // so we do our instructions in reverse order. + self.load_all_saved_registers(saved_registers); + } } /// Type to encapsulate the binary operation types in Brillig From 30cf67247599a5622ebeb0fcabaafdd28de2e073 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 19:27:02 +0000 Subject: [PATCH 02/16] add used registers method Co-authored-by: ludamad --- .../noirc_evaluator/src/brillig/brillig_ir/registers.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/registers.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/registers.rs index cad5670d76b..65018f41794 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/registers.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/registers.rs @@ -36,6 +36,14 @@ impl BrilligRegistersContext { } } + /// Lazily iterate over the used registers, + /// counting to next_free_register_index while excluding deallocated and reserved registers. + pub(crate) fn used_registers_iter(&self) -> impl Iterator + '_ { + (ReservedRegisters::NUM_RESERVED_REGISTERS..self.next_free_register_index) + .map(RegisterIndex::from) + .filter(|&index| !self.deallocated_registers.contains(&index)) + } + /// Creates a new register. pub(crate) fn allocate_register(&mut self) -> RegisterIndex { // If we have a register in our free list of deallocated registers, From 00a2e9bc1b85f6ace9926650c80938a3fa4b25c1 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 19:28:14 +0000 Subject: [PATCH 03/16] add method to handle calling normal functions and thread the function_id_to_block_id through necessary functions --- .../src/brillig/brillig_gen.rs | 18 ++++- .../src/brillig/brillig_gen/brillig_block.rs | 78 ++++++++++++++++--- crates/noirc_evaluator/src/brillig/mod.rs | 49 ++++++++++-- 3 files changed, 126 insertions(+), 19 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index 00d26f6c7fd..340d897b93f 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -7,14 +7,20 @@ use std::collections::HashMap; use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; -use super::brillig_ir::{artifact::BrilligArtifact, BrilligContext}; +use super::{ + brillig_ir::{artifact::BrilligArtifact, BrilligContext}, + FuncIdEntryBlockId, +}; /// Converting an SSA function into Brillig bytecode. /// /// TODO: Change this to use `dfg.basic_blocks_iter` which will return an /// TODO iterator of all of the basic blocks. /// TODO(Jake): what order is this ^ -pub(crate) fn convert_ssa_function(func: &Function) -> BrilligArtifact { +pub(crate) fn convert_ssa_function( + func: &Function, + ssa_function_id_to_block_id: &FuncIdEntryBlockId, +) -> BrilligArtifact { let mut reverse_post_order = Vec::new(); reverse_post_order.extend_from_slice(PostOrder::with_function(func).as_slice()); reverse_post_order.reverse(); @@ -25,7 +31,13 @@ pub(crate) fn convert_ssa_function(func: &Function) -> BrilligArtifact { let mut brillig_context = BrilligContext::new(); for block in reverse_post_order { - BrilligBlock::compile(&mut function_context, &mut brillig_context, block, &func.dfg); + BrilligBlock::compile( + &mut function_context, + &mut brillig_context, + ssa_function_id_to_block_id, + block, + &func.dfg, + ); } brillig_context.artifact() diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 8d74b11332e..3913f8c56a1 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1,6 +1,10 @@ +use std::collections::HashMap; + use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; +use crate::brillig::FuncIdEntryBlockId; +use crate::ssa_refactor::ir::function::FunctionId; use crate::ssa_refactor::ir::types::CompositeType; use crate::ssa_refactor::ir::{ basic_block::{BasicBlock, BasicBlockId}, @@ -22,6 +26,8 @@ pub(crate) struct BrilligBlock<'block> { block_id: BasicBlockId, /// Context for creating brillig opcodes brillig_context: &'block mut BrilligContext, + /// TODO: document + function_ids_to_block_ids: &'block HashMap, } impl<'block> BrilligBlock<'block> { @@ -29,17 +35,19 @@ impl<'block> BrilligBlock<'block> { pub(crate) fn compile( function_context: &'block mut FunctionContext, brillig_context: &'block mut BrilligContext, + function_ids_to_block_ids: &'block FuncIdEntryBlockId, block_id: BasicBlockId, dfg: &DataFlowGraph, ) { - let mut brillig_block = BrilligBlock { function_context, block_id, brillig_context }; + let mut brillig_block = + BrilligBlock { function_context, block_id, brillig_context, function_ids_to_block_ids }; brillig_block.convert_block(dfg); } fn convert_block(&mut self, dfg: &DataFlowGraph) { // Add a label for this block - let block_label = self.create_block_label(self.block_id); + let block_label = self.create_block_label_for_current_function(self.block_id); self.brillig_context.enter_context(block_label); // Convert the block parameters @@ -57,9 +65,22 @@ impl<'block> BrilligBlock<'block> { self.convert_ssa_terminator(terminator_instruction, dfg); } - /// Creates a unique global label for a block - fn create_block_label(&self, block_id: BasicBlockId) -> String { - format!("{}-{}", self.function_context.function_id, block_id) + /// Creates a unique global label for a block. + /// + /// This uses the current functions's function ID and the block ID + /// Making the assumption that the block ID passed in belongs to this + /// function. + fn create_block_label_for_current_function(&self, block_id: BasicBlockId) -> String { + Self::create_block_label(self.function_context.function_id, block_id) + } + /// Creates a unique label for a block using the function Id and the block ID. + /// + /// We implicitly assume that the function ID and the block ID is enough + /// for us to create a unique label across functions and blocks. + /// + /// This is so that during linking there are no duplicates or labels being overwritten. + fn create_block_label(function_id: FunctionId, block_id: BasicBlockId) -> String { + format!("{}-{}", function_id, block_id) } /// Converts an SSA terminator instruction into the necessary opcodes. @@ -74,9 +95,13 @@ impl<'block> BrilligBlock<'block> { match terminator_instruction { TerminatorInstruction::JmpIf { condition, then_destination, else_destination } => { let condition = self.convert_ssa_value(*condition, dfg); - self.brillig_context - .jump_if_instruction(condition, self.create_block_label(*then_destination)); - self.brillig_context.jump_instruction(self.create_block_label(*else_destination)); + self.brillig_context.jump_if_instruction( + condition, + self.create_block_label_for_current_function(*then_destination), + ); + self.brillig_context.jump_instruction( + self.create_block_label_for_current_function(*else_destination), + ); } TerminatorInstruction::Jmp { destination, arguments } => { let target = &dfg[*destination]; @@ -85,7 +110,8 @@ impl<'block> BrilligBlock<'block> { let source = self.convert_ssa_value(*src, dfg); self.brillig_context.mov_instruction(destination, source); } - self.brillig_context.jump_instruction(self.create_block_label(*destination)); + self.brillig_context + .jump_instruction(self.create_block_label_for_current_function(*destination)); } TerminatorInstruction::Return { return_values } => { let return_registers: Vec<_> = return_values @@ -189,6 +215,40 @@ impl<'block> BrilligBlock<'block> { &output_registers, ); } + Value::Function(func_id) => { + let function_arguments: Vec = + vecmap(arguments.clone(), |arg| self.convert_ssa_value(arg, dfg)); + let result_ids = dfg.instruction_results(instruction_id); + + // Create label for the function that will be called + // + // TODO: We _could_ avoid this by having the label for the entry block + // TODO be just the function_id. Since the entry_block won't have any predecessors + // TODO ie no block in this function, will be jumping to the entry block + // TODO: it should be fine; ie we don't need to check if we are jumping to the + // TODO entry block or a regular block when creating the label. + let entry_block_id = self.function_ids_to_block_ids[func_id]; + let label_of_function_to_call = + Self::create_block_label(*func_id, entry_block_id); + + let saved_registers = + self.brillig_context.pre_call_save_registers_prep_args(&function_arguments); + + // Call instruction, which will interpret above registers 0..num args + self.brillig_context.add_external_call_instruction(label_of_function_to_call); + + // Important: resolve after pre_call_save_registers_prep_args + // This ensures we don't save the results to registers unnecessarily. + let result_registers = vecmap(result_ids, |a| { + self.function_context.get_or_create_register(self.brillig_context, *a) + }); + assert!( + !saved_registers.iter().any(|x| result_registers.contains(x)), + "should not save registers used as function results" + ); + self.brillig_context + .post_call_prep_returns_load_registers(&result_registers, &saved_registers); + } _ => { unreachable!("only foreign function calls supported in unconstrained functions") } diff --git a/crates/noirc_evaluator/src/brillig/mod.rs b/crates/noirc_evaluator/src/brillig/mod.rs index 19d750a4544..d7c9b0bd892 100644 --- a/crates/noirc_evaluator/src/brillig/mod.rs +++ b/crates/noirc_evaluator/src/brillig/mod.rs @@ -3,23 +3,37 @@ pub(crate) mod brillig_ir; use self::{brillig_gen::convert_ssa_function, brillig_ir::artifact::BrilligArtifact}; use crate::ssa_refactor::{ - ir::function::{Function, FunctionId, RuntimeType}, + ir::{ + basic_block::BasicBlockId, + function::{Function, FunctionId, RuntimeType}, + }, ssa_gen::Ssa, }; use std::collections::HashMap; +pub(crate) type FuncIdEntryBlockId = HashMap; + /// Context structure for the brillig pass. /// It stores brillig-related data required for brillig generation. #[derive(Default)] pub struct Brillig { + /// Maps SSA function IDs to their entry block IDs + /// + /// Used for external call instructions + ssa_function_id_to_block_id: HashMap, /// Maps SSA functions to their brillig opcode ssa_function_to_brillig: HashMap, } impl Brillig { + /// Creates a Brillig object with a prefilled map of function IDs to entry block IDs + pub(crate) fn new(ssa_function_id_to_block_id: FuncIdEntryBlockId) -> Brillig { + Brillig { ssa_function_id_to_block_id, ssa_function_to_brillig: HashMap::new() } + } + /// Compiles a function into brillig and store the compilation artifacts pub(crate) fn compile(&mut self, func: &Function) { - let obj = convert_ssa_function(func); + let obj = convert_ssa_function(func, &self.ssa_function_id_to_block_id); self.ssa_function_to_brillig.insert(func.id(), obj); } } @@ -34,13 +48,34 @@ impl std::ops::Index for Brillig { impl Ssa { /// Generate compilation artifacts for brillig functions pub(crate) fn to_brillig(&self) -> Brillig { - let mut brillig = Brillig::default(); - for f in self.functions.values().filter(|func| func.runtime() == RuntimeType::Brillig) { - let id = f.id(); - if id != self.main_id { - brillig.compile(f); + // Collect all of the brillig functions + let brillig_functions = + self.functions.values().filter(|func| func.runtime() == RuntimeType::Brillig); + + // Collect the entry block IDs for each function. + // + // Call instructions only specify their function ID, not their entry block ID. + // But in order to jump to a function, we need to know the label that is assigned to + // the entry-block of the function. This will be the function_id + // concatenated with the entry_block_id. + let brillig_func_ids_to_entry_block_ids: HashMap = + brillig_functions + .clone() + .map(|func| { + let func_id = func.id(); + let entry_block_id = func.entry_block(); + (func_id, entry_block_id) + }) + .collect(); + + let mut brillig = Brillig::new(brillig_func_ids_to_entry_block_ids); + for brillig_function in brillig_functions { + // TODO: document why we are skipping the `main_id` for Brillig functions + if brillig_function.id() != self.main_id { + brillig.compile(brillig_function); } } + brillig } } From 75fd28eb270232bf719e9af4147f27f0b8dbff1f Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 19:28:36 +0000 Subject: [PATCH 04/16] add unresolved_call method into artifact struct --- .../src/brillig/brillig_ir/artifact.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 3c6be408a32..7ea5c87c8fd 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -11,6 +11,13 @@ pub(crate) struct BrilligArtifact { unresolved_jumps: Vec<(JumpInstructionPosition, UnresolvedJumpLocation)>, /// A map of labels to their position in byte code. labels: HashMap, + /// Set of labels which are external to the bytecode. + /// + /// This will most commonly contain the labels of functions + /// which are defined in other bytecode, that this bytecode has called. + /// TODO: perhaps we should combine this with the `unresolved_jumps` field + /// TODO: and have an enum which indicates whether the jump is internal or external + unresolved_external_call_labels: Vec<(JumpInstructionPosition, UnresolvedJumpLocation)>, } /// A pointer to a location in the opcode. @@ -37,6 +44,10 @@ pub(crate) type UnresolvedJumpLocation = Label; impl BrilligArtifact { /// Link two Brillig artifacts together and resolve all unresolved jump instructions. + /// + /// TODO: This method could be renamed to `link_and_resolve_jumps` + /// TODO: We could make this consume self, so the Clone is explicitly + /// TODO: done by the caller pub(crate) fn link(&mut self, obj: &BrilligArtifact) -> Vec { self.append_artifact(obj); self.resolve_jumps(); @@ -81,6 +92,17 @@ impl BrilligArtifact { self.unresolved_jumps.push((self.index_of_next_opcode(), destination)); self.push_opcode(jmp_instruction); } + /// Adds a unresolved external call that will be fixed once linking has been done. + pub(crate) fn add_unresolved_external_call( + &mut self, + call_instruction: BrilligOpcode, + destination: UnresolvedJumpLocation, + ) { + // TODO: Add a check to ensure that the opcode is a call instruction + + self.unresolved_external_call_labels.push((self.index_of_next_opcode(), destination)); + self.push_opcode(call_instruction); + } /// Returns true if the opcode is a jump instruction fn is_jmp_instruction(instruction: &BrilligOpcode) -> bool { From fd1e5cb4880ace8e0755e170dd7d890f79c2ac10 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 19:35:57 +0000 Subject: [PATCH 05/16] remove Vec::clone --- crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 3913f8c56a1..51125a63e2e 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -217,7 +217,7 @@ impl<'block> BrilligBlock<'block> { } Value::Function(func_id) => { let function_arguments: Vec = - vecmap(arguments.clone(), |arg| self.convert_ssa_value(arg, dfg)); + vecmap(arguments, |arg| self.convert_ssa_value(*arg, dfg)); let result_ids = dfg.instruction_results(instruction_id); // Create label for the function that will be called From bf0c13613f7fea1086ce536249725e558f97a57f Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 19:44:31 +0000 Subject: [PATCH 06/16] small clean-up: - remove magic constant - use mov_instruction method --- crates/noirc_evaluator/src/brillig/brillig_ir.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 7f48d1a8e51..6fbefb60da7 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -601,8 +601,7 @@ impl BrilligContext { destination, destination, const_register, - // TODO(AD): magic constant - BrilligBinaryOp::Integer { op, bit_size: 64 }, + BrilligBinaryOp::Integer { op, bit_size: BRILLIG_MEMORY_ADDRESSING_BIT_SIZE }, ); // Mark as no longer used for this purpose, frees for reuse self.deallocate_register(const_register); @@ -621,12 +620,9 @@ impl BrilligContext { // Move argument values to the front of the registers // // This means that the arguments will be in the first `n` registers after - // the special registers which are reserved. + // the number of reserved registers. for (i, argument) in arguments.iter().enumerate() { - self.push_opcode(BrilligOpcode::Mov { - destination: self.register(i), - source: *argument, - }); + self.mov_instruction(self.register(i), *argument); } saved_registers From da0d5f943eb6a7d0d8fc00e90c0a0e4b089d215e Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 21:54:38 +0000 Subject: [PATCH 07/16] modify API for linking --- crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index 9e61d579e48..bdf15e1f68a 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; -use crate::brillig::{brillig_gen::create_entry_point_function, Brillig}; +use crate::brillig::{brillig_ir::artifact::BrilligArtifact, Brillig}; use self::acir_ir::{ acir_variable::{AcirContext, AcirType, AcirVar}, @@ -182,8 +182,8 @@ impl Context { RuntimeType::Brillig => { let inputs = vecmap(arguments, |arg| self.convert_value(*arg, dfg)); - // Generate the brillig code of the function - let code = create_entry_point_function(arguments.len()).link(&brillig[*id]); + // Link the brillig code of the function using the artifacts and produce one stream of bytecode + let code = BrilligArtifact::link(&brillig[*id]); let outputs: Vec = vecmap(result_ids, |result_id| dfg.type_of_value(*result_id).into()); From 0ba8bedaa220c8ff3970a2d5c383ed7489589606 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 21:55:08 +0000 Subject: [PATCH 08/16] pass parameter length and return type length to BrilligContext --- .../src/brillig/brillig_gen.rs | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index 340d897b93f..f29d66b87cd 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -1,7 +1,9 @@ pub(crate) mod brillig_block; pub(crate) mod brillig_fn; -use crate::ssa_refactor::ir::{function::Function, post_order::PostOrder}; +use crate::ssa_refactor::ir::{ + function::Function, instruction::TerminatorInstruction, post_order::PostOrder, +}; use std::collections::HashMap; @@ -28,7 +30,19 @@ pub(crate) fn convert_ssa_function( let mut function_context = FunctionContext { function_id: func.id(), ssa_value_to_register: HashMap::new() }; - let mut brillig_context = BrilligContext::new(); + fn func_num_return_values(func: &Function) -> usize { + let dfg = &func.dfg; + let term = dfg[func.entry_block()] + .terminator() + .expect("expected a terminator instruction, as block is finished construction "); + match term { + TerminatorInstruction::Return { return_values } => return_values.len(), + _ => panic!("expected a return instruction, as block is finished construction "), + } + } + let num_parameters = func.parameters().len(); + let num_return_values = func_num_return_values(func); + let mut brillig_context = BrilligContext::new(num_parameters, num_return_values); for block in reverse_post_order { BrilligBlock::compile( @@ -42,10 +56,3 @@ pub(crate) fn convert_ssa_function( brillig_context.artifact() } - -/// Creates an entry point artifact, that will be linked with the brillig functions being called -pub(crate) fn create_entry_point_function(num_arguments: usize) -> BrilligArtifact { - let mut brillig_context = BrilligContext::new(); - brillig_context.entry_point_instruction(num_arguments); - brillig_context.artifact() -} From 2f50676f4ce983d76574b604f9ac818f75b9c525 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 21:55:51 +0000 Subject: [PATCH 09/16] move entry_point_instruction to artifact for now --- .../noirc_evaluator/src/brillig/brillig_ir.rs | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 6fbefb60da7..861318dd2b7 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -54,6 +54,11 @@ impl ReservedRegisters { pub(crate) fn stack_pointer() -> RegisterIndex { RegisterIndex::from(ReservedRegisters::StackPointer as usize) } + + /// Returns a user defined (non-reserved) register index. + fn user_register_index(index: usize) -> RegisterIndex { + RegisterIndex::from(index + ReservedRegisters::len()) + } } /// Brillig context object that is used while constructing the @@ -71,26 +76,15 @@ pub(crate) struct BrilligContext { impl BrilligContext { /// Initial context state - pub(crate) fn new() -> BrilligContext { + pub(crate) fn new(num_arguments: usize, num_return_parameters: usize) -> BrilligContext { BrilligContext { - obj: BrilligArtifact::default(), + obj: BrilligArtifact::new(num_arguments, num_return_parameters), registers: BrilligRegistersContext::new(), context_label: String::default(), section_label: 0, } } - /// Adds the instructions needed to handle entry point parameters - /// And sets the starting value of the reserved registers - pub(crate) fn entry_point_instruction(&mut self, num_arguments: usize) { - // Translate the inputs by the reserved registers offset - for i in (0..num_arguments).rev() { - self.mov_instruction(self.user_register_index(i), RegisterIndex::from(i)); - } - // Set the initial value of the stack pointer register - self.const_instruction(ReservedRegisters::stack_pointer(), Value::from(0_usize)); - } - /// Adds a brillig instruction to the brillig byte code pub(crate) fn push_opcode(&mut self, opcode: BrilligOpcode) { self.obj.byte_code.push(opcode); @@ -293,11 +287,6 @@ impl BrilligContext { self.obj.add_unresolved_jump(jmp_instruction, destination); } - /// Returns a user defined (non-reserved) register index. - fn user_register_index(&self, index: usize) -> RegisterIndex { - RegisterIndex::from(index + ReservedRegisters::len()) - } - /// Allocates an unused register. pub(crate) fn allocate_register(&mut self) -> RegisterIndex { self.registers.allocate_register() From 9471ad34fa679de9afcaa9670652365cead6c393 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 21:56:13 +0000 Subject: [PATCH 10/16] refactor API -- still has a lot of scaffolding --- .../src/brillig/brillig_ir/artifact.rs | 196 +++++++++++++++++- 1 file changed, 190 insertions(+), 6 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 7ea5c87c8fd..bef71d25394 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -1,6 +1,10 @@ -use acvm::acir::brillig_vm::Opcode as BrilligOpcode; +use acvm::acir::brillig_vm::{ + BinaryFieldOp, BinaryIntOp, Opcode as BrilligOpcode, RegisterIndex, Value, +}; use std::collections::HashMap; +use crate::brillig::brillig_ir::ReservedRegisters; + #[derive(Default, Debug, Clone)] /// Artifacts resulting from the compilation of a function into brillig byte code. /// Currently it is just the brillig bytecode of the function. @@ -18,6 +22,11 @@ pub(crate) struct BrilligArtifact { /// TODO: perhaps we should combine this with the `unresolved_jumps` field /// TODO: and have an enum which indicates whether the jump is internal or external unresolved_external_call_labels: Vec<(JumpInstructionPosition, UnresolvedJumpLocation)>, + /// The number of return values that this function will return. + number_of_return_parameters: usize, + + /// The number of arguments that this function will take. + number_of_arguments: usize, } /// A pointer to a location in the opcode. @@ -43,15 +52,89 @@ pub(crate) type JumpInstructionPosition = OpcodeLocation; pub(crate) type UnresolvedJumpLocation = Label; impl BrilligArtifact { - /// Link two Brillig artifacts together and resolve all unresolved jump instructions. + /// Initialize an artifact with the number of arguments and return parameters + pub(crate) fn new( + number_of_arguments: usize, + number_of_return_parameters: usize, + ) -> BrilligArtifact { + BrilligArtifact { + byte_code: Vec::new(), + unresolved_jumps: Vec::new(), + labels: HashMap::new(), + unresolved_external_call_labels: Vec::new(), + number_of_return_parameters, + number_of_arguments, + } + } + + /// Links Brillig artifact and resolve all unresolved jump instructions. + /// + /// Current usage of this method, does not link two independent Brillig artifacts. + /// `Self` at this point in time /// /// TODO: This method could be renamed to `link_and_resolve_jumps` /// TODO: We could make this consume self, so the Clone is explicitly /// TODO: done by the caller - pub(crate) fn link(&mut self, obj: &BrilligArtifact) -> Vec { - self.append_artifact(obj); - self.resolve_jumps(); - self.byte_code.clone() + pub(crate) fn link(artifact_to_append: &BrilligArtifact) -> Vec { + let mut linked_artifact = BrilligArtifact::default(); + + linked_artifact.entry_point_instruction(artifact_to_append.number_of_arguments); + // First we append the artifact to the end of the current artifact + // Updating the offsets of the appended artefact, so that the jumps + // are still correct. + linked_artifact.append_artifact(artifact_to_append); + + linked_artifact.exit_point_instruction(artifact_to_append.number_of_return_parameters); + + linked_artifact.resolve_jumps(); + + linked_artifact.byte_code.clone() + } + + /// Adds the instructions needed to handle entry point parameters + /// + /// And sets the starting value of the reserved registers + pub(crate) fn entry_point_instruction(&mut self, num_arguments: usize) { + // Translate the inputs by the reserved registers offset + for i in (0..num_arguments).rev() { + self.byte_code.push(BrilligOpcode::Mov { + destination: ReservedRegisters::user_register_index(i), + source: RegisterIndex::from(i), + }) + } + + // Set the initial value of the stack pointer register + self.byte_code.push(BrilligOpcode::Const { + destination: ReservedRegisters::stack_pointer(), + value: Value::from(0_usize), + }); + } + + /// Adds the instructions needed to handle return parameters + pub(crate) fn exit_point_instruction(&mut self, num_return_parameters: usize) { + // We want all functions to follow the calling convention of returning + // their results in the first `n` registers. So we modify the bytecode of the + // function to move the return values to the first `n` registers once completed. + // + // Remove the ending stop + // TODO: Shouldn't this be the case when we process a terminator instruction? + // TODO: If so, then entry_point_instruction and exit_point_instruction should be + // TODO put in brillig_gen. + // TODO: entry_point is called when we process a function, and exit_point is called + // TODO when we process a terminator instruction. + let expected_stop = self.byte_code.pop().expect("expected at least one opcode"); + assert_eq!(expected_stop, BrilligOpcode::Stop, "expected a stop code"); + + // TODO: this _seems_ like an abstraction leak, we need to know about the reserved + // TODO: registers in order to do this. + // Move the results to registers 0..n + for i in 0..num_return_parameters { + self.push_opcode(BrilligOpcode::Mov { + destination: i.into(), + source: ReservedRegisters::user_register_index(i), + }); + } + self.push_opcode(BrilligOpcode::Stop); } /// Link with an external brillig artifact. @@ -166,4 +249,105 @@ impl BrilligArtifact { } } } + + fn pretty_print_opcode(opcode: &BrilligOpcode) -> String { + fn binary_field_op_to_string(op: &BinaryFieldOp) -> &str { + match op { + BinaryFieldOp::Add => "+", + BinaryFieldOp::Sub => "-", + BinaryFieldOp::Mul => "*", + BinaryFieldOp::Div => "/", + BinaryFieldOp::Equals => "==", + } + } + + fn binary_int_op_to_string(op: &BinaryIntOp) -> &str { + match op { + BinaryIntOp::Add => "+", + BinaryIntOp::Sub => "-", + BinaryIntOp::Mul => "*", + BinaryIntOp::SignedDiv => "SDiv", + BinaryIntOp::UnsignedDiv => "UDiv", + BinaryIntOp::Equals => "==", + BinaryIntOp::LessThan => "<", + BinaryIntOp::LessThanEquals => "<=", + BinaryIntOp::And => "&&", + BinaryIntOp::Or => "||", + BinaryIntOp::Xor => "^", + BinaryIntOp::Shl => "<<", + BinaryIntOp::Shr => ">>", + } + } + + match opcode { + BrilligOpcode::BinaryFieldOp { destination, op, lhs, rhs } => { + format!( + "r{} = r{} {} r{}", + destination.to_usize(), + lhs.to_usize(), + binary_field_op_to_string(op), + rhs.to_usize() + ) + } + BrilligOpcode::BinaryIntOp { destination, op, bit_size, lhs, rhs } => { + format!( + "r{} = ({}-bit) r{} {} r{}", + destination.to_usize(), + bit_size, + lhs.to_usize(), + binary_int_op_to_string(op), + rhs.to_usize() + ) + } + BrilligOpcode::JumpIfNot { condition, location } => { + format!("IF NOT r{} GOTO LABEL {}", condition.to_usize(), location) + } + BrilligOpcode::JumpIf { condition, location } => { + format!("IF r{} GOTO LABEL {}", condition.to_usize(), location) + } + BrilligOpcode::Jump { location } => format!("GOTO LABEL {}", location), + BrilligOpcode::Call { location } => format!("CALL LABEL {}", location), + BrilligOpcode::Const { destination, value } => { + format!("r{} = CONST {}", destination.to_usize(), value.to_field().to_hex()) + } + BrilligOpcode::Return => String::from("RETURN"), + BrilligOpcode::ForeignCall { function, destinations, inputs } => { + // Assuming RegisterOrMemory also has a 'to_string' method + // let destinations: Vec = + // destinations.iter().map(|d| d.to_string()).collect(); + // let inputs: Vec = inputs.iter().map(|i| i.to_string()).collect(); + // format!( + // "FOREIGNCALL {} IN ({}) OUT ({})", + // function, + // inputs.join(", "), + // destinations.join(", ") + // ) + todo!() + } + BrilligOpcode::Mov { destination, source } => { + format!("r{} = MOV r{}", destination.to_usize(), source.to_usize()) + } + BrilligOpcode::Load { destination, source_pointer } => { + format!("r{} = LOAD r{}", destination.to_usize(), source_pointer.to_usize()) + } + BrilligOpcode::Store { destination_pointer, source } => { + format!("STORE r{} TO r{}", source.to_usize(), destination_pointer.to_usize()) + } + BrilligOpcode::Trap => String::from("TRAP"), + BrilligOpcode::Stop => String::from("STOP"), + } + } +} + +impl std::fmt::Display for BrilligArtifact { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str( + self.byte_code + .iter() + .map(Self::pretty_print_opcode) + .collect::>() + .join("\n") + .as_str(), + ) + } } From 1b39454f36d8718a5e6711af7d938a4ca6c5d305 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Sat, 17 Jun 2023 21:56:42 +0000 Subject: [PATCH 11/16] remove pretty_print_opcode --- .../src/brillig/brillig_ir/artifact.rs | 101 ------------------ 1 file changed, 101 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index bef71d25394..1838cdc6ce1 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -249,105 +249,4 @@ impl BrilligArtifact { } } } - - fn pretty_print_opcode(opcode: &BrilligOpcode) -> String { - fn binary_field_op_to_string(op: &BinaryFieldOp) -> &str { - match op { - BinaryFieldOp::Add => "+", - BinaryFieldOp::Sub => "-", - BinaryFieldOp::Mul => "*", - BinaryFieldOp::Div => "/", - BinaryFieldOp::Equals => "==", - } - } - - fn binary_int_op_to_string(op: &BinaryIntOp) -> &str { - match op { - BinaryIntOp::Add => "+", - BinaryIntOp::Sub => "-", - BinaryIntOp::Mul => "*", - BinaryIntOp::SignedDiv => "SDiv", - BinaryIntOp::UnsignedDiv => "UDiv", - BinaryIntOp::Equals => "==", - BinaryIntOp::LessThan => "<", - BinaryIntOp::LessThanEquals => "<=", - BinaryIntOp::And => "&&", - BinaryIntOp::Or => "||", - BinaryIntOp::Xor => "^", - BinaryIntOp::Shl => "<<", - BinaryIntOp::Shr => ">>", - } - } - - match opcode { - BrilligOpcode::BinaryFieldOp { destination, op, lhs, rhs } => { - format!( - "r{} = r{} {} r{}", - destination.to_usize(), - lhs.to_usize(), - binary_field_op_to_string(op), - rhs.to_usize() - ) - } - BrilligOpcode::BinaryIntOp { destination, op, bit_size, lhs, rhs } => { - format!( - "r{} = ({}-bit) r{} {} r{}", - destination.to_usize(), - bit_size, - lhs.to_usize(), - binary_int_op_to_string(op), - rhs.to_usize() - ) - } - BrilligOpcode::JumpIfNot { condition, location } => { - format!("IF NOT r{} GOTO LABEL {}", condition.to_usize(), location) - } - BrilligOpcode::JumpIf { condition, location } => { - format!("IF r{} GOTO LABEL {}", condition.to_usize(), location) - } - BrilligOpcode::Jump { location } => format!("GOTO LABEL {}", location), - BrilligOpcode::Call { location } => format!("CALL LABEL {}", location), - BrilligOpcode::Const { destination, value } => { - format!("r{} = CONST {}", destination.to_usize(), value.to_field().to_hex()) - } - BrilligOpcode::Return => String::from("RETURN"), - BrilligOpcode::ForeignCall { function, destinations, inputs } => { - // Assuming RegisterOrMemory also has a 'to_string' method - // let destinations: Vec = - // destinations.iter().map(|d| d.to_string()).collect(); - // let inputs: Vec = inputs.iter().map(|i| i.to_string()).collect(); - // format!( - // "FOREIGNCALL {} IN ({}) OUT ({})", - // function, - // inputs.join(", "), - // destinations.join(", ") - // ) - todo!() - } - BrilligOpcode::Mov { destination, source } => { - format!("r{} = MOV r{}", destination.to_usize(), source.to_usize()) - } - BrilligOpcode::Load { destination, source_pointer } => { - format!("r{} = LOAD r{}", destination.to_usize(), source_pointer.to_usize()) - } - BrilligOpcode::Store { destination_pointer, source } => { - format!("STORE r{} TO r{}", source.to_usize(), destination_pointer.to_usize()) - } - BrilligOpcode::Trap => String::from("TRAP"), - BrilligOpcode::Stop => String::from("STOP"), - } - } -} - -impl std::fmt::Display for BrilligArtifact { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str( - self.byte_code - .iter() - .map(Self::pretty_print_opcode) - .collect::>() - .join("\n") - .as_str(), - ) - } } From a055708e6cd0c16c58d2e4030681ba6e5470b9bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Mon, 19 Jun 2023 13:31:56 +0200 Subject: [PATCH 12/16] feat: implement call semantics (#1746) * fix: entry point and exit point handling * feat: function linking * docs: added comments * docs: more comments --- .../brillig_calls/Nargo.toml | 5 + .../brillig_calls/Prover.toml | 1 + .../brillig_calls/src/main.nr | 14 +++ .../src/brillig/brillig_gen.rs | 36 +++--- .../src/brillig/brillig_gen/brillig_block.rs | 18 +-- .../src/brillig/brillig_gen/brillig_fn.rs | 7 +- .../noirc_evaluator/src/brillig/brillig_ir.rs | 5 +- .../src/brillig/brillig_ir/artifact.rs | 117 ++++++++++++------ crates/noirc_evaluator/src/brillig/mod.rs | 54 +++----- .../src/ssa_refactor/acir_gen/mod.rs | 11 +- 10 files changed, 153 insertions(+), 115 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/src/main.nr diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Nargo.toml new file mode 100644 index 00000000000..e0b467ce5da --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.1" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Prover.toml new file mode 100644 index 00000000000..11497a473bc --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/Prover.toml @@ -0,0 +1 @@ +x = "0" diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/src/main.nr new file mode 100644 index 00000000000..7feaafd10ba --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_calls/src/main.nr @@ -0,0 +1,14 @@ +// Tests a very simple program. +// +// The features being tested is brillig calls +fn main(x: u32) { + assert(entry_point(x) == 2); +} + +unconstrained fn inner(x : u32) -> u32 { + x + 1 +} + +unconstrained fn entry_point(x : u32) -> u32 { + inner(x + 1) +} \ No newline at end of file diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index f29d66b87cd..6f6d7f8b6b1 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -9,20 +9,14 @@ use std::collections::HashMap; use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; -use super::{ - brillig_ir::{artifact::BrilligArtifact, BrilligContext}, - FuncIdEntryBlockId, -}; +use super::brillig_ir::{artifact::BrilligArtifact, BrilligContext}; /// Converting an SSA function into Brillig bytecode. /// /// TODO: Change this to use `dfg.basic_blocks_iter` which will return an /// TODO iterator of all of the basic blocks. /// TODO(Jake): what order is this ^ -pub(crate) fn convert_ssa_function( - func: &Function, - ssa_function_id_to_block_id: &FuncIdEntryBlockId, -) -> BrilligArtifact { +pub(crate) fn convert_ssa_function(func: &Function) -> BrilligArtifact { let mut reverse_post_order = Vec::new(); reverse_post_order.extend_from_slice(PostOrder::with_function(func).as_slice()); reverse_post_order.reverse(); @@ -32,26 +26,26 @@ pub(crate) fn convert_ssa_function( fn func_num_return_values(func: &Function) -> usize { let dfg = &func.dfg; - let term = dfg[func.entry_block()] - .terminator() - .expect("expected a terminator instruction, as block is finished construction "); - match term { - TerminatorInstruction::Return { return_values } => return_values.len(), - _ => panic!("expected a return instruction, as block is finished construction "), + let blocks = func.reachable_blocks(); + let mut function_return_values = None; + for block in blocks { + let terminator = dfg[block].terminator(); + if let Some(TerminatorInstruction::Return { return_values }) = terminator { + function_return_values = Some(return_values); + break; + } } + function_return_values + .expect("Expected a return instruction, as block is finished construction") + .len() } let num_parameters = func.parameters().len(); let num_return_values = func_num_return_values(func); let mut brillig_context = BrilligContext::new(num_parameters, num_return_values); + brillig_context.enter_context(FunctionContext::function_id_to_function_label(func.id())); for block in reverse_post_order { - BrilligBlock::compile( - &mut function_context, - &mut brillig_context, - ssa_function_id_to_block_id, - block, - &func.dfg, - ); + BrilligBlock::compile(&mut function_context, &mut brillig_context, block, &func.dfg); } brillig_context.artifact() diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 51125a63e2e..af3aa6bcc94 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1,9 +1,6 @@ -use std::collections::HashMap; - use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; -use crate::brillig::FuncIdEntryBlockId; use crate::ssa_refactor::ir::function::FunctionId; use crate::ssa_refactor::ir::types::CompositeType; use crate::ssa_refactor::ir::{ @@ -26,8 +23,6 @@ pub(crate) struct BrilligBlock<'block> { block_id: BasicBlockId, /// Context for creating brillig opcodes brillig_context: &'block mut BrilligContext, - /// TODO: document - function_ids_to_block_ids: &'block HashMap, } impl<'block> BrilligBlock<'block> { @@ -35,12 +30,10 @@ impl<'block> BrilligBlock<'block> { pub(crate) fn compile( function_context: &'block mut FunctionContext, brillig_context: &'block mut BrilligContext, - function_ids_to_block_ids: &'block FuncIdEntryBlockId, block_id: BasicBlockId, dfg: &DataFlowGraph, ) { - let mut brillig_block = - BrilligBlock { function_context, block_id, brillig_context, function_ids_to_block_ids }; + let mut brillig_block = BrilligBlock { function_context, block_id, brillig_context }; brillig_block.convert_block(dfg); } @@ -221,15 +214,8 @@ impl<'block> BrilligBlock<'block> { let result_ids = dfg.instruction_results(instruction_id); // Create label for the function that will be called - // - // TODO: We _could_ avoid this by having the label for the entry block - // TODO be just the function_id. Since the entry_block won't have any predecessors - // TODO ie no block in this function, will be jumping to the entry block - // TODO: it should be fine; ie we don't need to check if we are jumping to the - // TODO entry block or a regular block when creating the label. - let entry_block_id = self.function_ids_to_block_ids[func_id]; let label_of_function_to_call = - Self::create_block_label(*func_id, entry_block_id); + FunctionContext::function_id_to_function_label(*func_id); let saved_registers = self.brillig_context.pre_call_save_registers_prep_args(&function_arguments); diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 60de8743373..f5988d5cdf1 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use acvm::acir::brillig_vm::RegisterIndex; use crate::{ - brillig::brillig_ir::BrilligContext, + brillig::brillig_ir::{artifact::Label, BrilligContext}, ssa_refactor::ir::{function::FunctionId, value::ValueId}, }; @@ -38,4 +38,9 @@ impl FunctionContext { register } + + /// Creates a function label from a given SSA function id. + pub(crate) fn function_id_to_function_label(function_id: FunctionId) -> Label { + function_id.to_string() + } } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index 861318dd2b7..b4f8aa0c8b7 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -324,8 +324,9 @@ impl BrilligContext { pub(crate) fn return_instruction(&mut self, return_registers: &[RegisterIndex]) { for (destination_index, return_register) in return_registers.iter().enumerate() { // In case we have fewer return registers than indices to write to, ensure we've allocated this register - self.registers.ensure_register_is_allocated(destination_index.into()); - self.mov_instruction(destination_index.into(), *return_register); + let destination_register = ReservedRegisters::user_register_index(destination_index); + self.registers.ensure_register_is_allocated(destination_register); + self.mov_instruction(destination_register, *return_register); } self.stop_instruction(); } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 1838cdc6ce1..d38fdfebefb 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -1,6 +1,4 @@ -use acvm::acir::brillig_vm::{ - BinaryFieldOp, BinaryIntOp, Opcode as BrilligOpcode, RegisterIndex, Value, -}; +use acvm::acir::brillig_vm::{Opcode as BrilligOpcode, RegisterIndex, Value}; use std::collections::HashMap; use crate::brillig::brillig_ir::ReservedRegisters; @@ -67,40 +65,37 @@ impl BrilligArtifact { } } - /// Links Brillig artifact and resolve all unresolved jump instructions. - /// - /// Current usage of this method, does not link two independent Brillig artifacts. - /// `Self` at this point in time - /// - /// TODO: This method could be renamed to `link_and_resolve_jumps` - /// TODO: We could make this consume self, so the Clone is explicitly - /// TODO: done by the caller - pub(crate) fn link(artifact_to_append: &BrilligArtifact) -> Vec { - let mut linked_artifact = BrilligArtifact::default(); - - linked_artifact.entry_point_instruction(artifact_to_append.number_of_arguments); - // First we append the artifact to the end of the current artifact - // Updating the offsets of the appended artefact, so that the jumps - // are still correct. - linked_artifact.append_artifact(artifact_to_append); + /// Creates an entry point artifact wrapping the bytecode of the function provided. + pub(crate) fn to_entry_point_artifact(artifact: &BrilligArtifact) -> BrilligArtifact { + let mut entry_point_artifact = BrilligArtifact::new( + artifact.number_of_arguments, + artifact.number_of_return_parameters, + ); + entry_point_artifact.entry_point_instruction(); - linked_artifact.exit_point_instruction(artifact_to_append.number_of_return_parameters); + entry_point_artifact.add_unresolved_jumps_and_calls(artifact); + entry_point_artifact.byte_code.extend_from_slice(&artifact.byte_code); - linked_artifact.resolve_jumps(); + entry_point_artifact.exit_point_instruction(); + entry_point_artifact + } - linked_artifact.byte_code.clone() + /// Resolves all jumps and generates the final bytecode + pub(crate) fn finish(mut self) -> Vec { + self.resolve_jumps(); + self.byte_code } /// Adds the instructions needed to handle entry point parameters /// /// And sets the starting value of the reserved registers - pub(crate) fn entry_point_instruction(&mut self, num_arguments: usize) { + fn entry_point_instruction(&mut self) { // Translate the inputs by the reserved registers offset - for i in (0..num_arguments).rev() { + for i in (0..self.number_of_arguments).rev() { self.byte_code.push(BrilligOpcode::Mov { destination: ReservedRegisters::user_register_index(i), source: RegisterIndex::from(i), - }) + }); } // Set the initial value of the stack pointer register @@ -111,24 +106,25 @@ impl BrilligArtifact { } /// Adds the instructions needed to handle return parameters - pub(crate) fn exit_point_instruction(&mut self, num_return_parameters: usize) { + fn exit_point_instruction(&mut self) { // We want all functions to follow the calling convention of returning // their results in the first `n` registers. So we modify the bytecode of the // function to move the return values to the first `n` registers once completed. // - // Remove the ending stop - // TODO: Shouldn't this be the case when we process a terminator instruction? - // TODO: If so, then entry_point_instruction and exit_point_instruction should be - // TODO put in brillig_gen. - // TODO: entry_point is called when we process a function, and exit_point is called - // TODO when we process a terminator instruction. - let expected_stop = self.byte_code.pop().expect("expected at least one opcode"); - assert_eq!(expected_stop, BrilligOpcode::Stop, "expected a stop code"); + // Swap the stop opcode with a jump to the exit point section + + let stop_position = + self.byte_code.iter().position(|opcode| matches!(opcode, BrilligOpcode::Stop)); + + let stop_position = stop_position.expect("expected a stop opcode"); + + let exit_section = self.index_of_next_opcode(); + self.byte_code[stop_position] = BrilligOpcode::Jump { location: exit_section }; // TODO: this _seems_ like an abstraction leak, we need to know about the reserved // TODO: registers in order to do this. // Move the results to registers 0..n - for i in 0..num_return_parameters { + for i in 0..self.number_of_return_parameters { self.push_opcode(BrilligOpcode::Mov { destination: i.into(), source: ReservedRegisters::user_register_index(i), @@ -137,12 +133,48 @@ impl BrilligArtifact { self.push_opcode(BrilligOpcode::Stop); } - /// Link with an external brillig artifact. + /// Gets the first unresolved function call of this artifact. + pub(crate) fn first_unresolved_function_call(&self) -> Option