From dcb07aa32453d54620a54421de5a7225c2732e01 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 12:17:27 +0100 Subject: [PATCH 01/17] Reset next_section to 1 when entering a new context --- .../src/brillig/brillig_gen/brillig_block.rs | 22 +++++++++++-------- .../src/brillig/brillig_gen/brillig_fn.rs | 4 ++++ .../noirc_evaluator/src/brillig/brillig_ir.rs | 7 +++--- .../src/brillig/brillig_ir/artifact.rs | 2 +- .../src/brillig/brillig_ir/instructions.rs | 13 ++++++----- compiler/noirc_evaluator/src/brillig/mod.rs | 2 +- 6 files changed, 29 insertions(+), 21 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index a33e6406bc4..9bc8c7dcd36 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -51,11 +51,11 @@ pub(crate) struct BrilligBlock<'block, Registers: RegisterAllocator> { } impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { - /// Converts an SSA Basic block into a sequence of Brillig opcodes + /// Converts an SSA basic block into a sequence of Brillig opcodes. /// /// This method contains the necessary initial variable and register setup for compiling /// an SSA block by accessing the pre-computed liveness context. - pub(crate) fn compile( + pub(crate) fn compile_block( function_context: &'block mut FunctionContext, brillig_context: &'block mut BrilligContext, block_id: BasicBlockId, @@ -179,9 +179,9 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { new_hoisted_constants } - /// Internal method for [BrilligBlock::compile] that actually kicks off the Brillig compilation process + /// Internal method for [BrilligBlock::compile_block] that actually kicks off the Brillig compilation process. /// - /// At this point any Brillig context, should be contained in [BrilligBlock] and this function should + /// At this point any Brillig context should be contained in [BrilligBlock], and this function should /// only need to accept external SSA and debugging structures. fn convert_block(&mut self, dfg: &DataFlowGraph, call_stacks: &mut CallStackHelper) { // Add a label for this block @@ -287,12 +287,16 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } - /// Allocates the block parameters that the given block is defining + /// Allocates the block parameters that the given block is defining. + /// + /// We don't allocate the block parameters of the block itself here, we allocate the parameters the block is defining + /// for the descendant blocks it immediately dominates. Since predecessors to a block have to know where the parameters + /// of the block are allocated to pass data to it in [Self::convert_ssa_terminator], the block parameters need to be + /// defined/allocated before the given block. [VariableLiveness](crate::brillig::brillig_gen::variable_liveness::VariableLiveness) + /// decides when the block parameters are defined. + /// + /// For the entry block, the defined block params will be the params of the function + any extra params of blocks it's the immediate dominator of. fn convert_block_params(&mut self, dfg: &DataFlowGraph) { - // We don't allocate the block parameters here, we allocate the parameters the block is defining. - // Since predecessors to a block have to know where the parameters of the block are allocated to pass data to it, - // the block parameters need to be defined/allocated before the given block. Variable liveness provides when the block parameters are defined. - // For the entry block, the defined block params will be the params of the function + any extra params of blocks it's the immediate dominator of. for param_id in self.function_context.liveness.defined_block_params(&self.block_id) { let value = &dfg[param_id]; let param_type = match value { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index b135eab5be2..b59e7c31a35 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -70,6 +70,10 @@ impl FunctionContext { } } + /// Get the ID of the function this context was created for. + /// + /// Panics if we call it when in the context for created to hold + /// data structures for global codegen only. pub(crate) fn function_id(&self) -> FunctionId { self.function_id.expect("ICE: function_id should already be set") } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index aacac530176..1761483f811 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -83,12 +83,11 @@ pub(crate) struct BrilligContext { obj: BrilligArtifact, /// Tracks register allocations registers: Rc>, - /// Context label, must be unique with respect to the function - /// being linked. + /// Context label, must be unique with respect to the function being linked. context_label: Label, - /// Section label, used to separate sections of code + /// Section label, used to separate sections of code within a context. current_section: usize, - /// Stores the next available section + /// Stores the next available section. next_section: usize, /// IR printer debug_show: DebugShow, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 401bac1acae..091ebb91d63 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -306,7 +306,7 @@ impl BrilligArtifact { /// Returns the index of the next opcode. /// /// This is useful for labelling regions of code - /// before you have generated the opcodes for the region. + /// before we start generating the opcodes for the region. pub(crate) fn index_of_next_opcode(&self) -> OpcodeLocation { self.byte_code.len() } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs index b48b0e21038..3dab9f7c190 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs @@ -248,19 +248,20 @@ impl BrilligContext< self.obj.add_unresolved_jump(jmp_instruction, destination); } - /// Adds a label to the next opcode + /// Adds a context and a section label to the next opcode. + /// + /// Entering a context resets the current section to 0 and the next section to 1. pub(crate) fn enter_context(&mut self, label: Label) { + debug_assert!(label.section.is_none(), "new context should have no section"); self.debug_show.enter_context(label.to_string()); self.context_label = label.clone(); - self.current_section = 0; // Add a context label to the next opcode self.obj.add_label_at_position(label, self.obj.index_of_next_opcode()); - // Add a section label to the next opcode - self.obj - .add_label_at_position(self.current_section_label(), self.obj.index_of_next_opcode()); + self.enter_section(0); + self.next_section = 1; } - /// Enter the given section within a basic block + /// Enter the given section within a basic block. pub(super) fn enter_section(&mut self, section: usize) { self.current_section = section; self.obj diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index d5348e3dfbc..22b596d97e3 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -116,7 +116,7 @@ impl Brillig { brillig_context.call_check_max_stack_depth_procedure(); for block in function_context.blocks.clone() { - BrilligBlock::compile( + BrilligBlock::compile_block( &mut function_context, &mut brillig_context, block, From 9182c9a16892eed6c464f0f409428e9283f377b9 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 13:11:14 +0100 Subject: [PATCH 02/17] Pass the location to initialize_constants --- .../src/brillig/brillig_gen/brillig_block.rs | 63 ++++++++++--------- .../brillig_gen/brillig_block_variables.rs | 12 +++- 2 files changed, 41 insertions(+), 34 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 9bc8c7dcd36..1a23bfda3d5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -188,19 +188,21 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let block_label = self.create_block_label_for_current_function(self.block_id); self.brillig_context.enter_context(block_label); + // Allocate variables for parameter passing between blocks. self.convert_block_params(dfg); let block = &dfg[self.block_id]; - // Convert all of the instructions into the block + // Convert all of the instructions into the block. for instruction_id in block.instructions() { self.convert_ssa_instruction(*instruction_id, dfg, call_stacks); } - // Process the block's terminator instruction + // Process the block's terminator instruction. let terminator_instruction = block.terminator().expect("block is expected to be constructed"); + // If we are exiting the entry point, we may want to print the array copy count, for debug purposes. if self.brillig_context.count_array_copies() && matches!(terminator_instruction, TerminatorInstruction::Return { .. }) && self.function_context.is_entry_point @@ -208,7 +210,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { self.brillig_context.emit_println_of_array_copy_counter(); } - self.convert_ssa_terminator(terminator_instruction, dfg); + self.convert_terminator(terminator_instruction, dfg); } /// Creates a unique global label for a block. @@ -229,19 +231,19 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { Label::block(function_id, block_id) } - /// Converts an SSA terminator instruction into the necessary opcodes. - fn convert_ssa_terminator( + /// Converts an SSA terminator instruction into the necessary opcodes: + /// * allocates the hoisted constants which are used by dominated blocks + /// * for jumps: + /// * copies the arguments to the registers allocated in [Self::convert_block_params] + /// * adds jump opcodes to the labels of the destination blocks + /// * for return it allocates registers for the return values and copies from variables. + fn convert_terminator( &mut self, terminator_instruction: &TerminatorInstruction, dfg: &DataFlowGraph, ) { - self.initialize_constants( - &self - .function_context - .constant_allocation - .allocated_at_location(self.block_id, InstructionLocation::Terminator), - dfg, - ); + self.initialize_constants(dfg, InstructionLocation::Terminator); + match terminator_instruction { TerminatorInstruction::JmpIf { condition, @@ -291,7 +293,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { /// /// We don't allocate the block parameters of the block itself here, we allocate the parameters the block is defining /// for the descendant blocks it immediately dominates. Since predecessors to a block have to know where the parameters - /// of the block are allocated to pass data to it in [Self::convert_ssa_terminator], the block parameters need to be + /// of the block are allocated to pass data to it in [Self::convert_terminator], the block parameters need to be /// defined/allocated before the given block. [VariableLiveness](crate::brillig::brillig_gen::variable_liveness::VariableLiveness) /// decides when the block parameters are defined. /// @@ -299,16 +301,14 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { fn convert_block_params(&mut self, dfg: &DataFlowGraph) { for param_id in self.function_context.liveness.defined_block_params(&self.block_id) { let value = &dfg[param_id]; - let param_type = match value { - Value::Param { typ, .. } => typ, - _ => unreachable!("ICE: Only Param type values should appear in block parameters"), + let Value::Param { typ: param_type, .. } = value else { + unreachable!("ICE: Only Param type values should appear in block parameters"); }; match param_type { - // Simple parameters and arrays are passed as already filled registers - // In the case of arrays, the values should already be in memory and the register should - // Be a valid pointer to the array. - // For slices, two registers are passed, the pointer to the data and a register holding the size of the slice. Type::Numeric(_) | Type::Array(..) | Type::Slice(..) | Type::Reference(_) => { + // Simple parameters and arrays are passed as already filled registers. + // In the case of arrays, the values should already be in memory and the register should be a valid pointer to the array. + // For slices, two registers are passed, the pointer to the data and a register holding the size of the slice. self.variables.define_variable( self.function_context, self.brillig_context, @@ -316,7 +316,9 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { dfg, ); } - Type::Function => todo!("ICE: Type::Function Param not supported"), + Type::Function => unreachable!( + "ICE: Type::Function Param not supported; should have been removed by defunctionalization." + ), } } } @@ -335,13 +337,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let call_stack_new_id = call_stacks.get_or_insert_locations(&call_stack); self.brillig_context.set_call_stack(call_stack_new_id); - self.initialize_constants( - &self.function_context.constant_allocation.allocated_at_location( - self.block_id, - InstructionLocation::Instruction(instruction_id), - ), - dfg, - ); + self.initialize_constants(dfg, InstructionLocation::Instruction(instruction_id)); + match instruction { Instruction::Binary(binary) => { self.binary_gen(instruction_id, binary, dfg); @@ -544,12 +541,16 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { self.brillig_context.cast_instruction(destination, source); } - /// Accepts a list of constant values to be initialized + /// Initializes constants allocated to a [InstructionLocation] by [ConstantAllocation](crate::brillig::brillig_gen::constant_allocation::ConstantAllocation). /// - /// This method does no checks as to whether the supplied constants are actually constants. /// It is expected that this method is called before converting an SSA instruction to Brillig /// and the constants to be initialized have been precomputed and stored in [FunctionContext::constant_allocation]. - fn initialize_constants(&mut self, constants: &[ValueId], dfg: &DataFlowGraph) { + fn initialize_constants(&mut self, dfg: &DataFlowGraph, location: InstructionLocation) { + let constants = &self + .function_context + .constant_allocation + .allocated_at_location(self.block_id, location); + for &constant_id in constants { self.convert_ssa_value(constant_id, dfg); } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index cf3bc081d8e..3a8115fa0f1 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -70,7 +70,13 @@ impl BlockVariables { .collect() } - /// For a given SSA value id, define the variable and return the corresponding cached allocation. + /// For a given SSA value id, define the variable and return the corresponding cached memory allocation. + /// + /// The allocation will be cached in [FunctionContext::ssa_value_allocations], which is how it will be + /// passed on to the next block as a pre-allocated registers, if it's still alive at that point. + /// + /// The variable is added to [Self::available_variables] to show that it's live, where it stays until + /// [Self::remove_variable] deletes it. pub(crate) fn define_variable( &mut self, function_context: &mut FunctionContext, @@ -158,7 +164,7 @@ pub(crate) fn compute_array_length(item_typ: &CompositeType, elem_count: usize) item_typ.len() * elem_count } -/// For a given value_id, allocates the necessary registers to hold it. +/// For a given [ValueId], allocates the necessary registers to hold it. pub(crate) fn allocate_value( value_id: ValueId, brillig_context: &mut BrilligContext, @@ -169,7 +175,7 @@ pub(crate) fn allocate_value( allocate_value_with_type(brillig_context, typ) } -/// For a given value_id, allocates the necessary registers to hold it. +/// For a given [Type], allocates the necessary registers to hold it. pub(crate) fn allocate_value_with_type( brillig_context: &mut BrilligContext, typ: Type, From e791c35ac6bfdbf0b361f84b886d99e987bc7a95 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 13:17:02 +0100 Subject: [PATCH 03/17] Avoid allocating empty vectors when no constants are allocated --- .../src/brillig/brillig_gen/brillig_block.rs | 9 ++++++--- .../src/brillig/brillig_gen/constant_allocation.rs | 8 ++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 1a23bfda3d5..74f25bf022e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -546,12 +546,15 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { /// It is expected that this method is called before converting an SSA instruction to Brillig /// and the constants to be initialized have been precomputed and stored in [FunctionContext::constant_allocation]. fn initialize_constants(&mut self, dfg: &DataFlowGraph, location: InstructionLocation) { - let constants = &self + let Some(constants) = self .function_context .constant_allocation - .allocated_at_location(self.block_id, location); + .allocated_at_location(self.block_id, location) + else { + return; + }; - for &constant_id in constants { + for constant_id in constants.to_vec() { self.convert_ssa_value(constant_id, dfg); } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index 43bc6c45838..461178a65cd 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -71,10 +71,10 @@ impl ConstantAllocation { &self, block_id: BasicBlockId, location: InstructionLocation, - ) -> Vec { - self.allocation_points.get(&block_id).map_or(Vec::default(), |allocations| { - allocations.get(&location).map_or(Vec::default(), |constants| constants.clone()) - }) + ) -> Option<&[ValueId]> { + let allocations = self.allocation_points.get(&block_id)?; + let constants = allocations.get(&location)?; + Some(constants.as_ref()) } /// Visit all constant variables in the function and record their locations. From b69b9b7ef8ce39c57a442efb5592e1dac4ecb928 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 14:31:23 +0100 Subject: [PATCH 04/17] Add BrilligVariable::from to allow codegen_return to take a BrilligVariable --- .../src/brillig/brillig_gen/brillig_block.rs | 44 +++++++------ .../brillig_gen/brillig_block_variables.rs | 6 +- .../brillig_call/brillig_slice_ops.rs | 20 +++--- .../brillig/brillig_ir/brillig_variable.rs | 61 ++++++++++++++++++- .../src/brillig/brillig_ir/codegen_calls.rs | 15 +++-- .../src/brillig/brillig_ir/codegen_stack.rs | 2 + .../src/brillig/brillig_ir/entry_point.rs | 19 +++--- .../brillig_ir/procedures/array_copy.rs | 2 +- 8 files changed, 115 insertions(+), 54 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 74f25bf022e..2459788e2e5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -210,13 +210,13 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { self.brillig_context.emit_println_of_array_copy_counter(); } - self.convert_terminator(terminator_instruction, dfg); + self.convert_ssa_terminator(terminator_instruction, dfg); } /// 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 + /// 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) -> Label { Self::create_block_label(self.function_context.function_id(), block_id) @@ -237,7 +237,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { /// * copies the arguments to the registers allocated in [Self::convert_block_params] /// * adds jump opcodes to the labels of the destination blocks /// * for return it allocates registers for the return values and copies from variables. - fn convert_terminator( + fn convert_ssa_terminator( &mut self, terminator_instruction: &TerminatorInstruction, dfg: &DataFlowGraph, @@ -260,26 +260,22 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { self.create_block_label_for_current_function(*else_destination), ); } - TerminatorInstruction::Jmp { - destination: destination_block, - arguments, - call_stack: _, - } => { - let target_block = &dfg[*destination_block]; - for (src, dest) in arguments.iter().zip(target_block.parameters()) { - // Destinations are block parameters so they should have been allocated previously. - let destination = self.variables.get_allocation(self.function_context, *dest); - let source = self.convert_ssa_value(*src, dfg); + TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { + let destination_block = &dfg[*destination]; + for (arg, param) in arguments.iter().zip(destination_block.parameters()) { + // Destinations are block parameters so they should have been allocated previously in `create_block_params`. + let param = self.variables.get_allocation(self.function_context, *param); + let arg = self.convert_ssa_value(*arg, dfg); self.brillig_context - .mov_instruction(destination.extract_register(), source.extract_register()); + .mov_instruction(param.extract_register(), arg.extract_register()); } - self.brillig_context.jump_instruction( - self.create_block_label_for_current_function(*destination_block), - ); + self.brillig_context + .jump_instruction(self.create_block_label_for_current_function(*destination)); } TerminatorInstruction::Return { return_values, .. } => { let return_registers = vecmap(return_values, |value_id| { - self.convert_ssa_value(*value_id, dfg).extract_register() + // Get the allocations of the values to be returned. + self.convert_ssa_value(*value_id, dfg) }); self.brillig_context.codegen_return(&return_registers); } @@ -293,7 +289,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { /// /// We don't allocate the block parameters of the block itself here, we allocate the parameters the block is defining /// for the descendant blocks it immediately dominates. Since predecessors to a block have to know where the parameters - /// of the block are allocated to pass data to it in [Self::convert_terminator], the block parameters need to be + /// of the block are allocated to pass data to it in [Self::convert_ssa_terminator], the block parameters need to be /// defined/allocated before the given block. [VariableLiveness](crate::brillig::brillig_gen::variable_liveness::VariableLiveness) /// decides when the block parameters are defined. /// @@ -559,7 +555,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } - /// Converts an SSA [ValueId] into a [BrilligVariable]. Initializes if necessary. + /// Converts an SSA [ValueId] into a [BrilligVariable]. Initializes if necessary, or returns an existing allocation. /// /// This method also first checks whether the SSA value is a hoisted global constant. /// If it has already been initialized in the global space, we return the already existing variable. @@ -632,12 +628,14 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { new_variable } Value::Intrinsic(_) | Value::ForeignFunction(_) => { - todo!("ICE: Cannot convert value {value:?}") + unreachable!("ICE: Cannot convert value to Brillig: {value:?}") } } } - /// Converts an SSA `ValueId` into a `MemoryAddress`. Initializes if necessary. + /// Converts an SSA `ValueId` into a single `MemoryAddress`. Initializes if necessary. + /// + /// Panics if the value was converted to an array or vector. pub(crate) fn convert_ssa_single_addr_value( &mut self, value_id: ValueId, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 3a8115fa0f1..91c5a53a8fb 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -134,12 +134,16 @@ impl BlockVariables { brillig_context.deallocate_register(register); } - /// Checks if a variable is allocated. + /// Checks if a variable is allocated and live. pub(crate) fn is_allocated(&self, value_id: &ValueId) -> bool { self.available_variables.contains(value_id) } /// For a given SSA value id, return the corresponding cached allocation. + /// + /// Panics if + /// * the variable is not in [Self::available_variables], which means it is no longer live + /// * the variable is not in [FunctionContext::ssa_value_allocations], which means it was never defined pub(crate) fn get_allocation( &mut self, function_context: &FunctionContext, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_call/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_call/brillig_slice_ops.rs index cfec2d374cd..3822d85c9f2 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_call/brillig_slice_ops.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_call/brillig_slice_ops.rs @@ -279,18 +279,18 @@ mod tests { *target_vector, *source_len_var, *source_vector, - &[BrilligVariable::SingleAddr(*item_to_insert)], + &[item_to_insert.to_var()], ); } else { block.slice_push_front_operation( *target_vector, *source_len_var, *source_vector, - &[BrilligVariable::SingleAddr(*item_to_insert)], + &[item_to_insert.to_var()], ); } - context.codegen_return(&[target_vector.pointer]); + context.codegen_return(&[target_vector.to_var()]); // Compile to byte code. let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; @@ -465,18 +465,18 @@ mod tests { *target_vector, *source_len_var, *source_vector, - &[BrilligVariable::SingleAddr(*removed_item)], + &[removed_item.to_var()], ); } else { block.slice_pop_front_operation( *target_vector, *source_len_var, *source_vector, - &[BrilligVariable::SingleAddr(*removed_item)], + &[removed_item.to_var()], ); } - context.codegen_return(&[removed_item.address, target_vector.pointer]); + context.codegen_return(&[removed_item.to_var(), target_vector.to_var()]); let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; @@ -614,10 +614,10 @@ mod tests { *target_vector, *source_vector, *index_to_insert, - &[BrilligVariable::SingleAddr(*item_to_insert)], + &[item_to_insert.to_var()], ); - context.codegen_return(&[target_vector.pointer]); + context.codegen_return(&[target_vector.to_var()]); let calldata = array.into_iter().chain(vec![item]).chain(vec![index]).collect(); let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; @@ -755,10 +755,10 @@ mod tests { *target_vector, *source_vector, *index_to_insert, - &[BrilligVariable::SingleAddr(*removed_item)], + &[removed_item.to_var()], ); - context.codegen_return(&[removed_item.address, target_vector.pointer]); + context.codegen_return(&[removed_item.to_var(), target_vector.to_var()]); let calldata: Vec<_> = array.into_iter().chain(vec![index]).collect(); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/brillig_variable.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/brillig_variable.rs index f2f3cd43462..dda54620bb9 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/brillig_variable.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/brillig_variable.rs @@ -1,3 +1,5 @@ +use std::ops::Deref; + use acvm::{ FieldElement, acir::{AcirField, brillig::BitSize}, @@ -5,7 +7,10 @@ use acvm::{ }; use serde::{Deserialize, Serialize}; -use crate::ssa::ir::types::Type; +use crate::{ + brillig::brillig_ir::registers::{Allocated, RegisterAllocator}, + ssa::ir::types::Type, +}; use super::BRILLIG_MEMORY_ADDRESSING_BIT_SIZE; @@ -51,6 +56,9 @@ pub(crate) enum BrilligVariable { } impl BrilligVariable { + /// Extract a [SingleAddrVariable]. + /// + /// Panics if the variable is an array or vector. pub(crate) fn extract_single_addr(self) -> SingleAddrVariable { match self { BrilligVariable::SingleAddr(single_addr) => single_addr, @@ -58,6 +66,9 @@ impl BrilligVariable { } } + /// Extract a [BrilligArray]. + /// + /// Panics if it's a single address variable or a vector. pub(crate) fn extract_array(self) -> BrilligArray { match self { BrilligVariable::BrilligArray(array) => array, @@ -65,6 +76,9 @@ impl BrilligVariable { } } + /// Extract a [BrilligVector]. + /// + /// Panics if it's a single address variable or an array. pub(crate) fn extract_vector(self) -> BrilligVector { match self { BrilligVariable::BrilligVector(vector) => vector, @@ -72,7 +86,9 @@ impl BrilligVariable { } } - /// Extract the memory address that can be deallocated to make the memory available for reuse. + /// Extract the [MemoryAddress] out of any [BrilligVariable]. + /// + /// This can be deallocated to make the memory available for reuse. /// /// Note that this is a single address even for vectors, because this is a `BrilligVector`, not a `HeapVector`. pub(crate) fn extract_register(self) -> MemoryAddress { @@ -84,6 +100,47 @@ impl BrilligVariable { } } +impl From for BrilligVariable { + fn from(value: SingleAddrVariable) -> Self { + Self::SingleAddr(value) + } +} + +impl From for BrilligVariable { + fn from(value: BrilligArray) -> Self { + Self::BrilligArray(value) + } +} + +impl From for BrilligVariable { + fn from(value: BrilligVector) -> Self { + Self::BrilligVector(value) + } +} + +impl From<&Allocated> for BrilligVariable +where + BrilligVariable: From, + T: Copy, +{ + fn from(value: &Allocated) -> Self { + Self::from(*value.deref()) + } +} + +/// Convenience method to convert e.g. an `Allocated` to a `BrilligVariable`. +#[cfg(test)] +impl Allocated +where + BrilligVariable: From, + T: Copy, +{ + /// Convert the allocated value into a [BrilligVariable]. + pub(crate) fn to_var(&self) -> BrilligVariable { + BrilligVariable::from(**self) + } +} + pub(crate) fn type_to_heap_value_type(typ: &Type) -> HeapValueType { match typ { Type::Numeric(_) | Type::Reference(_) | Type::Function => HeapValueType::Simple( diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs index f747af3cc9f..0542ff5e679 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs @@ -57,20 +57,19 @@ impl BrilligContext< } /// Codegens a return from the current function. - pub(crate) fn codegen_return(&mut self, return_registers: &[MemoryAddress]) { - let mut sources = Vec::with_capacity(return_registers.len()); - let mut destinations = Vec::with_capacity(return_registers.len()); + /// + /// Takes the addresses of the values to be returned. + pub(crate) fn codegen_return(&mut self, return_variables: &[BrilligVariable]) { + let mut sources = Vec::with_capacity(return_variables.len()); + let mut destinations = Vec::with_capacity(return_variables.len()); - for (destination_index, return_register) in return_registers.iter().enumerate() { + for (destination_index, return_variable) in return_variables.iter().enumerate() { // In case we have fewer return registers than indices to write to, ensure we've allocated this register let destination_register = MemoryAddress::relative(Stack::start() + destination_index); self.registers_mut().ensure_register_is_allocated(destination_register); - sources.push(*return_register); + sources.push(return_variable.extract_register()); destinations.push(destination_register); } - destinations.iter().for_each(|destination| { - self.registers_mut().ensure_register_is_allocated(*destination); - }); self.codegen_mov_registers_to_registers(sources, destinations); self.return_instruction(); } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index 81263f41860..eb9e0b7f5e8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -27,7 +27,9 @@ impl BrilligContext< map }); + // Unique addresses that get anything moved into them. let destinations_set: HashSet<_> = movements_map.values().flatten().copied().collect(); + assert_eq!( destinations_set.len(), movements_map.values().flatten().count(), diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index a124788ae64..9b7180f7c0f 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -157,14 +157,14 @@ impl BrilligContext { .iter() .map(|argument| match argument { BrilligParameter::SingleAddr(bit_size) => { - self.allocate_single_addr(*bit_size).map(BrilligVariable::SingleAddr) + self.allocate_single_addr(*bit_size).map(BrilligVariable::from) } BrilligParameter::Array(_, _) => { let flattened_size = Self::flattened_size(argument); - self.allocate_brillig_array(flattened_size).map(BrilligVariable::BrilligArray) + self.allocate_brillig_array(flattened_size).map(BrilligVariable::from) } BrilligParameter::Slice(_, _) => { - self.allocate_brillig_vector().map(BrilligVariable::BrilligVector) + self.allocate_brillig_vector().map(BrilligVariable::from) } }) .collect() @@ -322,11 +322,11 @@ impl BrilligContext { .iter() .map(|return_parameter| match return_parameter { BrilligParameter::SingleAddr(bit_size) => { - self.allocate_single_addr(*bit_size).map(BrilligVariable::SingleAddr) + self.allocate_single_addr(*bit_size).map(BrilligVariable::from) } BrilligParameter::Array(item_types, item_count) => self .allocate_brillig_array(item_types.len() * item_count) - .map(BrilligVariable::BrilligArray), + .map(BrilligVariable::from), BrilligParameter::Slice(..) => unreachable!("ICE: Cannot return slices"), }) .collect(); @@ -384,7 +384,7 @@ mod tests { use crate::{ brillig::brillig_ir::{ - brillig_variable::BrilligArray, + brillig_variable::{BrilligArray, BrilligVariable, SingleAddrVariable}, entry_point::BrilligParameter, tests::{create_and_run_vm, create_context, create_entry_point_bytecode}, }, @@ -426,7 +426,8 @@ mod tests { // Load the first item of the nested array. context.load_instruction(*array_value, *items_pointer); - context.codegen_return(&[*array_value]); + let return_value = BrilligVariable::from(SingleAddrVariable::new_usize(*array_value)); + context.codegen_return(&[return_value]); let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; let (vm, return_data_offset, return_data_size) = @@ -458,9 +459,9 @@ mod tests { let mut context = create_context(FunctionId::test_new(0)); // Allocate the parameter - let return_register = context.allocate_register(); + let return_register = context.allocate_brillig_array(2); - context.codegen_return(&[*return_register]); + context.codegen_return(&[return_register.to_var()]); let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; let (vm, return_data_pointer, return_data_size) = diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/array_copy.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/array_copy.rs index ce52f6df44e..cf7846bf232 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/array_copy.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/array_copy.rs @@ -135,7 +135,7 @@ fn initialize_constant_string BrilligContext { /// emit: `println(f"Total arrays copied: {array_copy_counter}")` pub(crate) fn emit_println_of_array_copy_counter(&mut self) { - let array_copy_counter = BrilligVariable::SingleAddr(SingleAddrVariable { + let array_copy_counter = BrilligVariable::from(SingleAddrVariable { address: self.array_copy_counter_address(), bit_size: 32, }); From f0364995a0e9d170dfacbd1dd75f7ebd950d71aa Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 16:20:05 +0100 Subject: [PATCH 05/17] Add generate_opcodes helper for tests --- .../src/brillig/brillig_ir/codegen_calls.rs | 10 +- .../src/brillig/brillig_ir/codegen_stack.rs | 180 +++++++----------- 2 files changed, 75 insertions(+), 115 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs index 0542ff5e679..58a35cf0d70 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs @@ -58,17 +58,21 @@ impl BrilligContext< /// Codegens a return from the current function. /// - /// Takes the addresses of the values to be returned. + /// Takes the variables with the addresses of the values that need to be returned. + /// The values are copied to the beginning of the stack space, into an equal number of slots. + /// + /// Any potential overlap between the source of the return variables and the final destination + /// on the beginning of the stack is handled by [Self::codegen_mov_registers_to_registers]. pub(crate) fn codegen_return(&mut self, return_variables: &[BrilligVariable]) { let mut sources = Vec::with_capacity(return_variables.len()); let mut destinations = Vec::with_capacity(return_variables.len()); for (destination_index, return_variable) in return_variables.iter().enumerate() { - // In case we have fewer return registers than indices to write to, ensure we've allocated this register + // In case we have fewer return registers than indices to write to, ensure we've allocated this register. let destination_register = MemoryAddress::relative(Stack::start() + destination_index); self.registers_mut().ensure_register_is_allocated(destination_register); - sources.push(return_variable.extract_register()); destinations.push(destination_register); + sources.push(return_variable.extract_register()); } self.codegen_mov_registers_to_registers(sources, destinations); self.return_instruction(); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index eb9e0b7f5e8..0756a1d0914 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -5,7 +5,12 @@ use super::{BrilligContext, debug_show::DebugToString, registers::RegisterAlloca impl BrilligContext { /// This function moves values from a set of registers to another set of registers. + /// /// The only requirement is that every destination needs to be written at most once. + /// The same source can be copied to multiple destinations. + /// + /// The method detects cycles in the movements and breaks them by introducing + /// temporary registers. pub(crate) fn codegen_mov_registers_to_registers( &mut self, sources: Vec, @@ -30,15 +35,14 @@ impl BrilligContext< // Unique addresses that get anything moved into them. let destinations_set: HashSet<_> = movements_map.values().flatten().copied().collect(); + // Ensure that all destinations are unique. assert_eq!( destinations_set.len(), movements_map.values().flatten().count(), "Multiple moves to the same register found" ); - let mut loop_detector = LoopDetector::default(); - loop_detector.collect_loops(&movements_map); - let loops = loop_detector.loops; + let loops = LoopDetector::detect_loops(&movements_map); // In order to break the loops we need to store one register from each in a temporary and then use that temporary as source. let mut temporaries = Vec::with_capacity(loops.len()); @@ -81,13 +85,29 @@ impl BrilligContext< } } -#[derive(Default)] +/// Addresses visited when a loop was found in the movements. +/// +/// It might contain addresses that aren't actually part of the loop, +/// e.g. A->B->C->D->C would contain [A,B,C,D] even though the loop is just [C,D]. +/// +/// They are ordered by their value, not their appearance in the loop. +/// The order provides determinism when we try to break the loop. +type AddressLoop = im::OrdSet; + struct LoopDetector { visited_sources: HashSet, - loops: Vec>, + loops: Vec, } impl LoopDetector { + fn detect_loops( + movements: &HashMap>, + ) -> Vec { + let mut detector = Self { visited_sources: Default::default(), loops: Default::default() }; + detector.collect_loops(movements); + detector.loops + } + fn collect_loops(&mut self, movements: &HashMap>) { for source in movements.keys() { self.find_loop_recursive(*source, movements, im::OrdSet::default()); @@ -98,16 +118,15 @@ impl LoopDetector { &mut self, source: MemoryAddress, movements: &HashMap>, - mut previous_sources: im::OrdSet, + mut previous_sources: AddressLoop, ) { - if self.visited_sources.contains(&source) { + // Mark as visited + if !self.visited_sources.insert(source) { return; } - // Mark as visited - self.visited_sources.insert(source); previous_sources.insert(source); - // Get all destinations + // Get all destinations (this can be empty when we treat destinations as sources during recursion). if let Some(destinations) = movements.get(&source) { for destination in destinations { if previous_sources.contains(destination) { @@ -133,13 +152,17 @@ mod tests { use crate::{ brillig::{ BrilligOptions, - brillig_ir::{BrilligContext, LayoutConfig, artifact::Label, registers::Stack}, + brillig_ir::{ + BrilligContext, LayoutConfig, artifact::Label, codegen_stack::LoopDetector, + registers::Stack, + }, }, ssa::ir::function::FunctionId, }; // Tests for the loop finder + /// Generate a movements map from test data, turning numbers into relative addresses. fn generate_movements_map( movements: Vec<(usize, usize)>, ) -> HashMap> { @@ -155,44 +178,51 @@ mod tests { fn test_loop_detector_basic_loop() { let movements = vec![(0, 1), (1, 2), (2, 3), (3, 0)]; let movements_map = generate_movements_map(movements); - let mut loop_detector = super::LoopDetector::default(); - loop_detector.collect_loops(&movements_map); - assert_eq!(loop_detector.loops.len(), 1); - assert_eq!(loop_detector.loops[0].len(), 4); + let loops = LoopDetector::detect_loops(&movements_map); + assert_eq!(loops.len(), 1); + assert_eq!(loops[0].len(), 4); } #[test] fn test_loop_detector_no_loop() { let movements = vec![(0, 1), (1, 2), (2, 3), (3, 4)]; let movements_map = generate_movements_map(movements); - let mut loop_detector = super::LoopDetector::default(); - loop_detector.collect_loops(&movements_map); - assert_eq!(loop_detector.loops.len(), 0); + let loops = LoopDetector::detect_loops(&movements_map); + assert_eq!(loops.len(), 0); } #[test] fn test_loop_detector_loop_with_branch() { let movements = vec![(0, 1), (1, 2), (2, 0), (0, 3), (3, 4)]; let movements_map = generate_movements_map(movements); - let mut loop_detector = super::LoopDetector::default(); - loop_detector.collect_loops(&movements_map); - assert_eq!(loop_detector.loops.len(), 1); - assert_eq!(loop_detector.loops[0].len(), 3); + let loops = LoopDetector::detect_loops(&movements_map); + assert_eq!(loops.len(), 1); + assert_eq!(loops[0].len(), 3); } #[test] fn test_loop_detector_two_loops() { let movements = vec![(0, 1), (1, 2), (2, 0), (3, 4), (4, 5), (5, 3)]; let movements_map = generate_movements_map(movements); - let mut loop_detector = super::LoopDetector::default(); - loop_detector.collect_loops(&movements_map); - assert_eq!(loop_detector.loops.len(), 2); - assert_eq!(loop_detector.loops[0].len(), 3); - assert_eq!(loop_detector.loops[1].len(), 3); + let loops = LoopDetector::detect_loops(&movements_map); + assert_eq!(loops.len(), 2); + assert_eq!(loops[0].len(), 3); + assert_eq!(loops[1].len(), 3); } // Tests for mov_registers_to_registers + /// Generate `Opcode::Mov` for a sequence of expected `dst <- src` moves. + fn generate_opcodes(movements: Vec<(usize, usize)>) -> Vec> { + movements + .into_iter() + .map(|(dst, src)| Opcode::Mov { + destination: MemoryAddress::relative(dst), + source: MemoryAddress::relative(src), + }) + .collect() + } + fn movements_to_source_and_destinations( movements: Vec<(usize, usize)>, ) -> (Vec, Vec) { @@ -235,27 +265,7 @@ mod tests { context.codegen_mov_registers_to_registers(sources, destinations); let opcodes = context.artifact().byte_code; - assert_eq!( - opcodes, - vec![ - Opcode::Mov { - destination: MemoryAddress::relative(14), - source: MemoryAddress::relative(13) - }, - Opcode::Mov { - destination: MemoryAddress::relative(13), - source: MemoryAddress::relative(12) - }, - Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) - }, - Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(10) - }, - ] - ); + assert_eq!(opcodes, generate_opcodes(vec![(14, 13), (13, 12), (12, 11), (11, 10)])); } #[test] fn test_mov_registers_to_registers_no_op_filter() { @@ -265,19 +275,7 @@ mod tests { context.codegen_mov_registers_to_registers(sources, destinations); let opcodes = context.artifact().byte_code; - assert_eq!( - opcodes, - vec![ - Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) - }, - Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(10) - }, - ] - ); + assert_eq!(opcodes, generate_opcodes(vec![(12, 11), (11, 10)])); } #[test] @@ -288,31 +286,7 @@ mod tests { context.codegen_mov_registers_to_registers(sources, destinations); let opcodes = context.artifact().byte_code; - assert_eq!( - opcodes, - vec![ - Opcode::Mov { - destination: MemoryAddress::relative(1), - source: MemoryAddress::relative(10) - }, - Opcode::Mov { - destination: MemoryAddress::relative(10), - source: MemoryAddress::relative(13) - }, - Opcode::Mov { - destination: MemoryAddress::relative(13), - source: MemoryAddress::relative(12) - }, - Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) - }, - Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(1) - } - ] - ); + assert_eq!(opcodes, generate_opcodes(vec![(1, 10), (10, 13), (13, 12), (12, 11), (11, 1)])); } #[test] @@ -325,32 +299,14 @@ mod tests { let opcodes = context.artifact().byte_code; assert_eq!( opcodes, - vec![ - Opcode::Mov { - destination: MemoryAddress::relative(1), - source: MemoryAddress::relative(10) - }, // Temporary - Opcode::Mov { - destination: MemoryAddress::relative(10), - source: MemoryAddress::relative(12) - }, // Branch - Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) - }, // Loop - Opcode::Mov { - destination: MemoryAddress::relative(14), - source: MemoryAddress::relative(13) - }, // Loop - Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(1) - }, // Finish branch - Opcode::Mov { - destination: MemoryAddress::relative(13), - source: MemoryAddress::relative(1) - } // Finish loop - ] + generate_opcodes(vec![ + (1, 10), // Temporary + (10, 12), // Branch + (12, 11), // Loop + (14, 13), // Loop + (11, 1), // Finish branch + (13, 1) // Finish loop + ]) ); } } From e4d1de8b5978ebf55ac6d1280157558e28188fad Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 16:42:05 +0100 Subject: [PATCH 06/17] Switch to BTree for determinism. Show that registers not actually part of the loop are added to the detection --- .../src/brillig/brillig_ir/codegen_stack.rs | 67 +++++++++++++------ 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index 0756a1d0914..e7c7db6bc0f 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -1,8 +1,14 @@ +use std::collections::{BTreeMap, BTreeSet, HashSet}; + use acvm::{AcirField, acir::brillig::MemoryAddress}; -use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; use super::{BrilligContext, debug_show::DebugToString, registers::RegisterAllocator}; +/// Map sources to potentially multiple destination. +/// +/// Using a BTree so we get deterministic loop detection. +type MovementsMap = BTreeMap>; + impl BrilligContext { /// This function moves values from a set of registers to another set of registers. /// @@ -26,14 +32,14 @@ impl BrilligContext< // Now we need to detect all cycles. // First build a map of the movements. Note that a source could have multiple destinations - let mut movements_map: HashMap> = - movements.into_iter().fold(HashMap::default(), |mut map, (source, destination)| { + let mut movements_map: MovementsMap = + movements.into_iter().fold(BTreeMap::default(), |mut map, (source, destination)| { map.entry(source).or_default().insert(destination); map }); // Unique addresses that get anything moved into them. - let destinations_set: HashSet<_> = movements_map.values().flatten().copied().collect(); + let destinations_set: BTreeSet<_> = movements_map.values().flatten().copied().collect(); // Ensure that all destinations are unique. assert_eq!( @@ -49,7 +55,7 @@ impl BrilligContext< for loop_found in loops { let temp_register = self.allocate_register(); - let first_source = loop_found.iter().next().unwrap(); + let first_source = loop_found.get_min().unwrap(); self.mov_instruction(*temp_register, *first_source); let destinations_of_temp = movements_map.remove(first_source).unwrap(); movements_map.insert(*temp_register, destinations_of_temp); @@ -57,7 +63,8 @@ impl BrilligContext< } // After removing loops we should have an DAG with each node having only one ancestor (but could have multiple successors) - // Now we should be able to move the registers just by performing a DFS on the movements map + // Now we should be able to move the registers just by performing a DFS on the movements map. + // Start from the heads, which are not destinations; anything else should be reachable from them by following the paths. let heads: Vec<_> = movements_map .keys() .filter(|source| !destinations_set.contains(source)) @@ -69,11 +76,12 @@ impl BrilligContext< } } - fn perform_movements( - &mut self, - movements: &HashMap>, - current_source: MemoryAddress, - ) { + /// Starting from the head do a DFS through the movements to find the last destination, + /// then generate movement opcodes _backwards_, unraveling the DFS path. + /// + /// By doing so, we can have a series of moves such as `[1->2, 2->3]` which become opcodes + /// [3<-2, 2<-1], without having 2 overwritten by 1 first, before it could be copied to 3. + fn perform_movements(&mut self, movements: &MovementsMap, current_source: MemoryAddress) { if let Some(destinations) = movements.get(¤t_source) { for destination in destinations { self.perform_movements(movements, *destination); @@ -92,6 +100,8 @@ impl BrilligContext< /// /// They are ordered by their value, not their appearance in the loop. /// The order provides determinism when we try to break the loop. +/// +/// Using an immutable data structure for structural sharing during DFS. type AddressLoop = im::OrdSet; struct LoopDetector { @@ -101,14 +111,14 @@ struct LoopDetector { impl LoopDetector { fn detect_loops( - movements: &HashMap>, + movements: &BTreeMap>, ) -> Vec { let mut detector = Self { visited_sources: Default::default(), loops: Default::default() }; detector.collect_loops(movements); detector.loops } - fn collect_loops(&mut self, movements: &HashMap>) { + fn collect_loops(&mut self, movements: &MovementsMap) { for source in movements.keys() { self.find_loop_recursive(*source, movements, im::OrdSet::default()); } @@ -117,7 +127,7 @@ impl LoopDetector { fn find_loop_recursive( &mut self, source: MemoryAddress, - movements: &HashMap>, + movements: &MovementsMap, mut previous_sources: AddressLoop, ) { // Mark as visited @@ -143,17 +153,20 @@ impl LoopDetector { #[cfg(test)] mod tests { + use std::collections::BTreeMap; + use acvm::{ FieldElement, acir::brillig::{MemoryAddress, Opcode}, }; - use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::{ brillig::{ BrilligOptions, brillig_ir::{ - BrilligContext, LayoutConfig, artifact::Label, codegen_stack::LoopDetector, + BrilligContext, LayoutConfig, + artifact::Label, + codegen_stack::{LoopDetector, MovementsMap}, registers::Stack, }, }, @@ -163,10 +176,8 @@ mod tests { // Tests for the loop finder /// Generate a movements map from test data, turning numbers into relative addresses. - fn generate_movements_map( - movements: Vec<(usize, usize)>, - ) -> HashMap> { - movements.into_iter().fold(HashMap::default(), |mut map, (source, destination)| { + fn generate_movements_map(movements: Vec<(usize, usize)>) -> MovementsMap { + movements.into_iter().fold(BTreeMap::default(), |mut map, (source, destination)| { map.entry(MemoryAddress::relative(source)) .or_default() .insert(MemoryAddress::relative(destination)); @@ -183,6 +194,20 @@ mod tests { assert_eq!(loops[0].len(), 4); } + #[test] + fn test_loop_detector_loop_with_init() { + // 0->1->2->3->2 + let movements = vec![(0, 1), (1, 2), (2, 3), (3, 2)]; + let movements_map = generate_movements_map(movements); + let loops = LoopDetector::detect_loops(&movements_map); + assert_eq!(loops.len(), 1); + assert_eq!( + loops[0], + im::OrdSet::from_iter([0, 1, 2, 3].map(MemoryAddress::relative)), + "0 and 1 are in the detection set, despite not being part of the loop body" + ); + } + #[test] fn test_loop_detector_no_loop() { let movements = vec![(0, 1), (1, 2), (2, 3), (3, 4)]; @@ -213,6 +238,8 @@ mod tests { // Tests for mov_registers_to_registers /// Generate `Opcode::Mov` for a sequence of expected `dst <- src` moves. + /// + /// Note that this is the opposite order of expected by `generate_movements_map`. fn generate_opcodes(movements: Vec<(usize, usize)>) -> Vec> { movements .into_iter() From 02adb6b69caf4739581f26231d498f85b750d3d1 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 16:49:00 +0100 Subject: [PATCH 07/17] Add assert_generated_opcodes helper --- .../src/brillig/brillig_ir/codegen_stack.rs | 70 +++++++++---------- 1 file changed, 32 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index e7c7db6bc0f..e2d37c3fc6c 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -274,66 +274,60 @@ mod tests { context } + fn assert_generated_opcodes( + // (src, dst) + movements: Vec<(usize, usize)>, + // (dst, src) + expected_moves: Vec<(usize, usize)>, + ) { + let (sources, destinations) = movements_to_source_and_destinations(movements); + + let mut context = create_context(); + context.codegen_mov_registers_to_registers(sources, destinations); + + let opcodes = context.artifact().byte_code; + + assert_eq!(opcodes, generate_opcodes(expected_moves)); + } + #[test] #[should_panic(expected = "Multiple moves to the same register found")] fn test_mov_registers_to_registers_overwrite() { let movements = vec![(10, 11), (12, 11), (10, 13)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let mut context = create_context(); - - context.codegen_mov_registers_to_registers(sources, destinations); + assert_generated_opcodes(movements, vec![]); } #[test] fn test_mov_registers_to_registers_no_loop() { let movements = vec![(10, 11), (11, 12), (12, 13), (13, 14)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let mut context = create_context(); - - context.codegen_mov_registers_to_registers(sources, destinations); - let opcodes = context.artifact().byte_code; - assert_eq!(opcodes, generate_opcodes(vec![(14, 13), (13, 12), (12, 11), (11, 10)])); + let expected_moves = vec![(14, 13), (13, 12), (12, 11), (11, 10)]; + assert_generated_opcodes(movements, expected_moves); } #[test] fn test_mov_registers_to_registers_no_op_filter() { let movements = vec![(10, 11), (11, 11), (11, 12)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let mut context = create_context(); - - context.codegen_mov_registers_to_registers(sources, destinations); - let opcodes = context.artifact().byte_code; - assert_eq!(opcodes, generate_opcodes(vec![(12, 11), (11, 10)])); + let expected_moves = vec![(12, 11), (11, 10)]; + assert_generated_opcodes(movements, expected_moves); } #[test] fn test_mov_registers_to_registers_loop() { let movements = vec![(10, 11), (11, 12), (12, 13), (13, 10)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let mut context = create_context(); - - context.codegen_mov_registers_to_registers(sources, destinations); - let opcodes = context.artifact().byte_code; - assert_eq!(opcodes, generate_opcodes(vec![(1, 10), (10, 13), (13, 12), (12, 11), (11, 1)])); + let expected_moves = vec![(1, 10), (10, 13), (13, 12), (12, 11), (11, 1)]; + assert_generated_opcodes(movements, expected_moves); } #[test] fn test_mov_registers_to_registers_loop_and_branch() { let movements = vec![(10, 11), (11, 12), (12, 10), (10, 13), (13, 14)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let mut context = create_context(); - - context.codegen_mov_registers_to_registers(sources, destinations); - let opcodes = context.artifact().byte_code; - assert_eq!( - opcodes, - generate_opcodes(vec![ - (1, 10), // Temporary - (10, 12), // Branch - (12, 11), // Loop - (14, 13), // Loop - (11, 1), // Finish branch - (13, 1) // Finish loop - ]) - ); + let expected_moves = vec![ + (1, 10), // Temporary + (10, 12), // Branch + (12, 11), // Loop + (14, 13), // Loop + (11, 1), // Finish branch + (13, 1), // Finish loop + ]; + assert_generated_opcodes(movements, expected_moves); } } From e9c5c6fee2fddf23f465ffad596b1918ee49720c Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 16:55:01 +0100 Subject: [PATCH 08/17] Swap dst<-src to src->dst in tests --- .../src/brillig/brillig_ir/codegen_stack.rs | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index e2d37c3fc6c..a501617ba35 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -237,13 +237,11 @@ mod tests { // Tests for mov_registers_to_registers - /// Generate `Opcode::Mov` for a sequence of expected `dst <- src` moves. - /// - /// Note that this is the opposite order of expected by `generate_movements_map`. + /// Generate `Opcode::Mov` for a sequence of expected `src -> dst` moves. fn generate_opcodes(movements: Vec<(usize, usize)>) -> Vec> { movements .into_iter() - .map(|(dst, src)| Opcode::Mov { + .map(|(src, dst)| Opcode::Mov { destination: MemoryAddress::relative(dst), source: MemoryAddress::relative(src), }) @@ -274,10 +272,9 @@ mod tests { context } + /// Test that a series of `src->dst` movements results in a series of `src->dst` move opcodes. fn assert_generated_opcodes( - // (src, dst) movements: Vec<(usize, usize)>, - // (dst, src) expected_moves: Vec<(usize, usize)>, ) { let (sources, destinations) = movements_to_source_and_destinations(movements); @@ -300,20 +297,20 @@ mod tests { #[test] fn test_mov_registers_to_registers_no_loop() { let movements = vec![(10, 11), (11, 12), (12, 13), (13, 14)]; - let expected_moves = vec![(14, 13), (13, 12), (12, 11), (11, 10)]; + let expected_moves = vec![(13, 14), (12, 13), (11, 12), (10, 11)]; assert_generated_opcodes(movements, expected_moves); } #[test] fn test_mov_registers_to_registers_no_op_filter() { let movements = vec![(10, 11), (11, 11), (11, 12)]; - let expected_moves = vec![(12, 11), (11, 10)]; + let expected_moves = vec![(11, 12), (10, 11)]; assert_generated_opcodes(movements, expected_moves); } #[test] fn test_mov_registers_to_registers_loop() { let movements = vec![(10, 11), (11, 12), (12, 13), (13, 10)]; - let expected_moves = vec![(1, 10), (10, 13), (13, 12), (12, 11), (11, 1)]; + let expected_moves = vec![(10, 1), (13, 10), (12, 13), (11, 12), (1, 11)]; assert_generated_opcodes(movements, expected_moves); } @@ -321,12 +318,12 @@ mod tests { fn test_mov_registers_to_registers_loop_and_branch() { let movements = vec![(10, 11), (11, 12), (12, 10), (10, 13), (13, 14)]; let expected_moves = vec![ - (1, 10), // Temporary - (10, 12), // Branch - (12, 11), // Loop - (14, 13), // Loop - (11, 1), // Finish branch - (13, 1), // Finish loop + (10, 1), // Temporary + (12, 10), // Branch + (11, 12), // Loop + (13, 14), // Loop + (1, 11), // Finish branch + (1, 13), // Finish loop ]; assert_generated_opcodes(movements, expected_moves); } From 6dc8fd47f27674d4afc9b0ba73ed6e9cd537bfa4 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 24 Oct 2025 17:03:01 +0100 Subject: [PATCH 09/17] Test to show that a loop with an 'init' is handled --- .../src/brillig/brillig_ir/codegen_stack.rs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index a501617ba35..aaaa530561f 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -64,7 +64,7 @@ impl BrilligContext< // After removing loops we should have an DAG with each node having only one ancestor (but could have multiple successors) // Now we should be able to move the registers just by performing a DFS on the movements map. - // Start from the heads, which are not destinations; anything else should be reachable from them by following the paths. + // Starting from the head of movement sequences, anything else should be reachable from them by following the paths. let heads: Vec<_> = movements_map .keys() .filter(|source| !destinations_set.contains(source)) @@ -80,7 +80,7 @@ impl BrilligContext< /// then generate movement opcodes _backwards_, unraveling the DFS path. /// /// By doing so, we can have a series of moves such as `[1->2, 2->3]` which become opcodes - /// [3<-2, 2<-1], without having 2 overwritten by 1 first, before it could be copied to 3. + /// `[3<-2, 2<-1]`, avoiding having 2 overwritten by 1 before it could be copied to 3. fn perform_movements(&mut self, movements: &MovementsMap, current_source: MemoryAddress) { if let Some(destinations) = movements.get(¤t_source) { for destination in destinations { @@ -95,9 +95,6 @@ impl BrilligContext< /// Addresses visited when a loop was found in the movements. /// -/// It might contain addresses that aren't actually part of the loop, -/// e.g. A->B->C->D->C would contain [A,B,C,D] even though the loop is just [C,D]. -/// /// They are ordered by their value, not their appearance in the loop. /// The order provides determinism when we try to break the loop. /// @@ -110,6 +107,12 @@ struct LoopDetector { } impl LoopDetector { + /// Detect all loops in a series of movements. + /// + /// The resulting loop might contain addresses that aren't actually part of the loop, + /// e.g. `0->1->2->3->2` would return `[0,1,2,3]` even though the loop is just `[2,3]`, + /// however such cases should already be rejected by `codegen_mov_registers_to_registers`, + /// since for this to happen, one of the destinations need to appear more than once. fn detect_loops( movements: &BTreeMap>, ) -> Vec { @@ -327,4 +330,11 @@ mod tests { ]; assert_generated_opcodes(movements, expected_moves); } + + #[test] + #[should_panic(expected = "Multiple moves to the same register found")] + fn test_mov_registers_to_registers_loop_with_init() { + let movements = vec![(0, 1), (1, 2), (2, 3), (3, 2)]; + assert_generated_opcodes(movements, vec![]); + } } From a96b6464e85afde9d6872dd998dcb16186ecaec2 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Sun, 26 Oct 2025 22:16:01 +0000 Subject: [PATCH 10/17] Add property based test to show that moving registers to registers works --- Cargo.lock | 1 + compiler/noirc_evaluator/Cargo.toml | 1 + .../src/brillig/brillig_ir/codegen_calls.rs | 2 +- .../src/brillig/brillig_ir/codegen_stack.rs | 90 ++++++++++++++++--- .../src/brillig/brillig_ir/registers.rs | 2 +- 5 files changed, 84 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6b3278629c4..4638edfb3c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3851,6 +3851,7 @@ name = "noirc_evaluator" version = "1.0.0-beta.14" dependencies = [ "acvm", + "arbtest", "bn254_blackbox_solver", "cfg-if", "chrono", diff --git a/compiler/noirc_evaluator/Cargo.toml b/compiler/noirc_evaluator/Cargo.toml index 2e2386e5978..8f9f429e54c 100644 --- a/compiler/noirc_evaluator/Cargo.toml +++ b/compiler/noirc_evaluator/Cargo.toml @@ -39,6 +39,7 @@ petgraph.workspace = true fm.workspace = true [dev-dependencies] +arbtest.workspace = true proptest.workspace = true similar-asserts.workspace = true tracing-test = "0.2.5" diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs index 58a35cf0d70..6c5d8352a4d 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs @@ -74,7 +74,7 @@ impl BrilligContext< destinations.push(destination_register); sources.push(return_variable.extract_register()); } - self.codegen_mov_registers_to_registers(sources, destinations); + self.codegen_mov_registers_to_registers(&sources, &destinations); self.return_instruction(); } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index aaaa530561f..99cea348536 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -19,22 +19,20 @@ impl BrilligContext< /// temporary registers. pub(crate) fn codegen_mov_registers_to_registers( &mut self, - sources: Vec, - destinations: Vec, + sources: &[MemoryAddress], + destinations: &[MemoryAddress], ) { - assert_eq!(sources.len(), destinations.len()); + assert_eq!(sources.len(), destinations.len(), "sources and destinations length must match"); + // Remove all no-ops - let movements: Vec<_> = sources - .into_iter() - .zip(destinations) - .filter(|(source, destination)| source != destination) - .collect(); + let movements = + sources.iter().zip(destinations).filter(|(source, destination)| source != destination); // Now we need to detect all cycles. // First build a map of the movements. Note that a source could have multiple destinations let mut movements_map: MovementsMap = movements.into_iter().fold(BTreeMap::default(), |mut map, (source, destination)| { - map.entry(source).or_default().insert(destination); + map.entry(*source).or_default().insert(*destination); map }); @@ -162,6 +160,7 @@ mod tests { FieldElement, acir::brillig::{MemoryAddress, Opcode}, }; + use iter_extended::vecmap; use crate::{ brillig::{ @@ -251,6 +250,7 @@ mod tests { .collect() } + /// Split numeric `src -> dst` movements into separate vectors and convert to `MemoryAddress` fn movements_to_source_and_destinations( movements: Vec<(usize, usize)>, ) -> (Vec, Vec) { @@ -264,6 +264,7 @@ mod tests { } pub(crate) fn create_context() -> BrilligContext { + // Show the opcodes if the test fails. let options = BrilligOptions { enable_debug_trace: true, enable_debug_assertions: true, @@ -283,7 +284,7 @@ mod tests { let (sources, destinations) = movements_to_source_and_destinations(movements); let mut context = create_context(); - context.codegen_mov_registers_to_registers(sources, destinations); + context.codegen_mov_registers_to_registers(&sources, &destinations); let opcodes = context.artifact().byte_code; @@ -337,4 +338,73 @@ mod tests { let movements = vec![(0, 1), (1, 2), (2, 3), (3, 2)]; assert_generated_opcodes(movements, vec![]); } + + /// Test that random movements have the expected effect. + #[test] + fn prop_mov_registers_to_registers() { + const MEM_SIZE: usize = 10; + arbtest::arbtest(|u| { + // Allocate more memory to allow for temporary variables. + let mut memory: Vec = vec![0; MEM_SIZE * 2]; + // Fill the memory with some random numbers. + for i in 0..memory.len() { + memory[i] = u.arbitrary()?; + } + + // Pick a random unique subset of the slots as destinations. + let num_destinations = u.int_in_range(0..=MEM_SIZE)?; + + // All potential memory slots; we can't address before the stack start. + let all_indexes = (0..MEM_SIZE).map(|i| i + Stack::start()).collect::>(); + let mut destinations = all_indexes.clone(); + + // Shuffle the destinations; using the random numbers in memory as key. + destinations.sort_by_key(|i| memory[*i]); + + // Keep the first N destinations. + destinations.truncate(num_destinations); + + // Pick random sources for each destination (same source can be repeated). + let mut sources = Vec::with_capacity(num_destinations); + for _ in 0..num_destinations { + sources.push(u.choose(&all_indexes).copied()?); + } + + // Take a snapshot of the source data; this is what we expect the destination to become. + let source_data = vecmap(&sources, |i| memory[*i]); + + // Generate the opcodes. + let opcodes = { + // Convert to MemoryAddress + let sources = vecmap(&sources, |i| MemoryAddress::relative(*i)); + let destinations = vecmap(&destinations, |i| MemoryAddress::relative(*i)); + + let mut context = create_context(); + + // Treat the memory we care about as pre-allocated, so temporary variables are created after them. + let all_registers = vecmap(all_indexes, MemoryAddress::relative); + context.set_allocated_registers(all_registers); + + context.codegen_mov_registers_to_registers(&sources, &destinations); + context.artifact().byte_code + }; + + // Execute the opcodes. + for opcode in opcodes { + let Opcode::Mov { destination, source } = opcode else { + unreachable!("only Mov expected"); + }; + memory[destination.to_usize()] = memory[source.to_usize()]; + } + + // Get the final values at the destination slots. + let destination_data = vecmap(&destinations, |i| memory[*i]); + + // At the end the destination should have the same value as the source had. + assert_eq!(destination_data, source_data); + + Ok(()) + }) + .run(); + } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs index 78c80669769..010ab04e462 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs @@ -188,7 +188,7 @@ impl RegisterAllocator for Stack { ) -> Self { let empty = Stack::new(layout); for register in &preallocated_registers { - assert!(empty.is_within_bounds(*register), "Register out of stack bounds"); + assert!(empty.is_within_bounds(*register), "Register out of stack bounds: {register}"); } Self { From 175ef447058ca19f62d0a6f9a66b04438b35879b Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Sun, 26 Oct 2025 22:30:14 +0000 Subject: [PATCH 11/17] Fix loop detection to only return the actual loop --- .../src/brillig/brillig_ir/codegen_stack.rs | 43 ++++++++++++------- 1 file changed, 27 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index 99cea348536..42605e6a5d0 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -91,7 +91,7 @@ impl BrilligContext< } } -/// Addresses visited when a loop was found in the movements. +/// A set of addresses that were part of a loop in the movements. /// /// They are ordered by their value, not their appearance in the loop. /// The order provides determinism when we try to break the loop. @@ -107,10 +107,7 @@ struct LoopDetector { impl LoopDetector { /// Detect all loops in a series of movements. /// - /// The resulting loop might contain addresses that aren't actually part of the loop, - /// e.g. `0->1->2->3->2` would return `[0,1,2,3]` even though the loop is just `[2,3]`, - /// however such cases should already be rejected by `codegen_mov_registers_to_registers`, - /// since for this to happen, one of the destinations need to appear more than once. + /// Returns a list of all loops detected. fn detect_loops( movements: &BTreeMap>, ) -> Vec { @@ -121,7 +118,12 @@ impl LoopDetector { fn collect_loops(&mut self, movements: &MovementsMap) { for source in movements.keys() { - self.find_loop_recursive(*source, movements, im::OrdSet::default()); + self.find_loop_recursive( + *source, + movements, + im::OrdSet::default(), + im::Vector::default(), + ); } } @@ -129,23 +131,32 @@ impl LoopDetector { &mut self, source: MemoryAddress, movements: &MovementsMap, - mut previous_sources: AddressLoop, + mut sources_in_path: im::OrdSet, + mut source_path: im::Vector, ) { // Mark as visited if !self.visited_sources.insert(source) { return; } - previous_sources.insert(source); + sources_in_path.insert(source); + source_path.push_back(source); + // Get all destinations (this can be empty when we treat destinations as sources during recursion). if let Some(destinations) = movements.get(&source) { for destination in destinations { - if previous_sources.contains(destination) { - // Found a loop - let loop_sources = previous_sources.clone(); + if sources_in_path.contains(destination) { + // Found a loop; find the suffix that is contained in the loop. + let idx = source_path.index_of(destination).unwrap(); + let loop_sources = AddressLoop::from_iter(source_path.skip(idx)); self.loops.push(loop_sources); } else { - self.find_loop_recursive(*destination, movements, previous_sources.clone()); + self.find_loop_recursive( + *destination, + movements, + sources_in_path.clone(), + source_path.clone(), + ); } } } @@ -198,15 +209,15 @@ mod tests { #[test] fn test_loop_detector_loop_with_init() { - // 0->1->2->3->2 - let movements = vec![(0, 1), (1, 2), (2, 3), (3, 2)]; + // 0->1->2->3->1 + let movements = vec![(0, 1), (1, 2), (2, 3), (3, 1)]; let movements_map = generate_movements_map(movements); let loops = LoopDetector::detect_loops(&movements_map); assert_eq!(loops.len(), 1); assert_eq!( loops[0], - im::OrdSet::from_iter([0, 1, 2, 3].map(MemoryAddress::relative)), - "0 and 1 are in the detection set, despite not being part of the loop body" + im::OrdSet::from_iter([1, 2, 3].map(MemoryAddress::relative)), + "Only the blocks actually in the loop should be included" ); } From 50bc2f435f6696025b3b33d2d4f296ba45880339 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Sun, 26 Oct 2025 23:17:18 +0000 Subject: [PATCH 12/17] Clippy --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 3 ++- .../noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 2459788e2e5..242c93d24da 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -546,11 +546,12 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .function_context .constant_allocation .allocated_at_location(self.block_id, location) + .map(|c| c.to_vec()) else { return; }; - for constant_id in constants.to_vec() { + for constant_id in constants { self.convert_ssa_value(constant_id, dfg); } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index 42605e6a5d0..c6937b6034e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -358,8 +358,8 @@ mod tests { // Allocate more memory to allow for temporary variables. let mut memory: Vec = vec![0; MEM_SIZE * 2]; // Fill the memory with some random numbers. - for i in 0..memory.len() { - memory[i] = u.arbitrary()?; + for slot in memory.iter_mut() { + *slot = u.arbitrary()?; } // Pick a random unique subset of the slots as destinations. From c4ce35c0547e69835bf021a6640134f5bacf2585 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 27 Oct 2025 08:28:52 +0000 Subject: [PATCH 13/17] Apply suggestions from code review --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- .../src/brillig/brillig_gen/brillig_block_variables.rs | 2 +- compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 242c93d24da..3e4eaa06fd3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -263,7 +263,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { let destination_block = &dfg[*destination]; for (arg, param) in arguments.iter().zip(destination_block.parameters()) { - // Destinations are block parameters so they should have been allocated previously in `create_block_params`. + // Destinations are block parameters, so they should have been allocated previously in `create_block_params`. let param = self.variables.get_allocation(self.function_context, *param); let arg = self.convert_ssa_value(*arg, dfg); self.brillig_context diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 91c5a53a8fb..266ad45d0c7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -73,7 +73,7 @@ impl BlockVariables { /// For a given SSA value id, define the variable and return the corresponding cached memory allocation. /// /// The allocation will be cached in [FunctionContext::ssa_value_allocations], which is how it will be - /// passed on to the next block as a pre-allocated registers, if it's still alive at that point. + /// passed on to the next block as a pre-allocated register, if it's still alive at that point. /// /// The variable is added to [Self::available_variables] to show that it's live, where it stays until /// [Self::remove_variable] deletes it. diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index b59e7c31a35..b95f6a07433 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -72,7 +72,7 @@ impl FunctionContext { /// Get the ID of the function this context was created for. /// - /// Panics if we call it when in the context for created to hold + /// Panics if we call it when in the context created to hold /// data structures for global codegen only. pub(crate) fn function_id(&self) -> FunctionId { self.function_id.expect("ICE: function_id should already be set") From e1e1bfb495ab1189fe5caf6429802dfd607b72fb Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Mon, 27 Oct 2025 08:29:07 +0000 Subject: [PATCH 14/17] Update compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs --- .../noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index c6937b6034e..f913970cd9a 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -4,7 +4,7 @@ use acvm::{AcirField, acir::brillig::MemoryAddress}; use super::{BrilligContext, debug_show::DebugToString, registers::RegisterAllocator}; -/// Map sources to potentially multiple destination. +/// Map sources to potentially multiple destinations. /// /// Using a BTree so we get deterministic loop detection. type MovementsMap = BTreeMap>; From 83c0e4535424ca40650b6cddbc2451bf397e3903 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 28 Oct 2025 20:07:32 +0000 Subject: [PATCH 15/17] Apply suggestions from code review Co-authored-by: Maxim Vezenov --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 2 +- .../noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 3e4eaa06fd3..b73e2014e01 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -215,7 +215,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { /// Creates a unique global label for a block. /// - /// This uses the current functions's function ID and the block ID, + /// This uses the current function'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) -> Label { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index f913970cd9a..351cc2fee84 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -30,8 +30,8 @@ impl BrilligContext< // Now we need to detect all cycles. // First build a map of the movements. Note that a source could have multiple destinations - let mut movements_map: MovementsMap = - movements.into_iter().fold(BTreeMap::default(), |mut map, (source, destination)| { + let mut movements_map = + movements.into_iter().fold(MovementsMap::default(), |mut map, (source, destination)| { map.entry(*source).or_default().insert(*destination); map }); From c7d18c29cac15f15b3e7b30b9b4d31e8f539757c Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 28 Oct 2025 20:08:11 +0000 Subject: [PATCH 16/17] Move 'function' up. --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index b73e2014e01..66899a0ec9b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -216,8 +216,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { /// Creates a unique global label for a block. /// /// This uses the current function's function ID and the block ID, - /// making the assumption that the block ID passed in belongs to this - /// function. + /// making the assumption that the block ID passed in belongs to this function. fn create_block_label_for_current_function(&self, block_id: BasicBlockId) -> Label { Self::create_block_label(self.function_context.function_id(), block_id) } From 238d2deacadc3bf0878911c90f8777cf6b36e0f6 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Tue, 28 Oct 2025 20:33:18 +0000 Subject: [PATCH 17/17] Format code --- .../src/brillig/brillig_ir/codegen_stack.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs index 351cc2fee84..d407340f8b9 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -30,11 +30,13 @@ impl BrilligContext< // Now we need to detect all cycles. // First build a map of the movements. Note that a source could have multiple destinations - let mut movements_map = - movements.into_iter().fold(MovementsMap::default(), |mut map, (source, destination)| { + let mut movements_map = movements.into_iter().fold( + MovementsMap::default(), + |mut map, (source, destination)| { map.entry(*source).or_default().insert(*destination); map - }); + }, + ); // Unique addresses that get anything moved into them. let destinations_set: BTreeSet<_> = movements_map.values().flatten().copied().collect();