From 1f2f3060501dfe334fcdb646d23cefdd9dafaa35 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 10:42:04 +0100 Subject: [PATCH 01/12] improve register moves --- .../src/brillig/brillig_ir/codegen_stack.rs | 451 +++++++++--------- 1 file changed, 238 insertions(+), 213 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 e4feabaf9ac..b5ddd55ee05 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -1,125 +1,154 @@ use acvm::{AcirField, acir::brillig::MemoryAddress}; -use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; + +use crate::brillig::brillig_ir::registers::Stack; use super::{BrilligContext, debug_show::DebugToString, registers::RegisterAllocator}; impl BrilligContext { + fn to_index(adr: &MemoryAddress) -> usize { + match adr { + MemoryAddress::Relative(size) => size - Stack::start(), + MemoryAddress::Direct(_) => usize::MAX, + } + } + fn from_index(idx: usize) -> MemoryAddress { + assert!(idx != usize::MAX, "invalid index"); + MemoryAddress::Relative(idx + Stack::start()) + } + /// 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. + /// It assumes that: + /// - every destination needs to be written at most once. Will panic if not. + /// - destinations are relative addresses, starting from 0 and with 1 increment. Will panic if not + /// - sources are any relative addresses, and can also be direct address to the global space. TODO: panic if not + /// - sources and destinations have same length. Will panic if not. pub(crate) fn codegen_mov_registers_to_registers( &mut self, sources: Vec, destinations: Vec, ) { assert_eq!(sources.len(), destinations.len()); - // Remove all no-ops - let movements: Vec<_> = sources - .into_iter() - .zip(destinations) - .filter(|(source, destination)| source != destination) - .collect(); - - // 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)| { - map.entry(source).or_default().insert(destination); - map - }); - - let destinations_set: HashSet<_> = movements_map.values().flatten().copied().collect(); - 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; - // 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()); - for loop_found in loops { - let temp_register = self.allocate_register(); - temporaries.push(temp_register); - let first_source = loop_found.iter().next().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); + let n = sources.len(); + let start = Stack::start(); + let mut free_register = None; + let mut processed = 0; + // Compute the number of children for each node in the movement graph + let mut children = vec![0; n]; + for i in 0..n { + // Check that destinations are relatives to 0,..,n-1 + assert_eq!(destinations[i].unwrap_relative() - start, i); + let index = Self::to_index(&sources[i]); + if index < n && index != i { + children[index] += 1; + } } - // 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 - let heads: Vec<_> = movements_map - .keys() - .filter(|source| !destinations_set.contains(source)) - .copied() - .collect(); - - for head in heads { - self.perform_movements(&movements_map, head); + // Process all sinks in the graph and follow their parents. + // Keep track of nodes having more than 2 children, in case they belong to a loop. + let mut tail_candidates = Vec::new(); + for i in 0..n { + // Generate a movement for each sink in the graph + if children[i] == 0 { + // A sink has no child + let mut node = i; + while children[node] == 0 { + if Self::to_index(&sources[node]) == node { + //no-op: mark the node as processed + children[node] = usize::MAX; + processed += 1; + break; + } + // Generates a move instruction + self.perform_movement(node, sources[node], &mut children, &mut processed); + // Follow the parent + let index = Self::to_index(&sources[node]); + if index < n { + children[index] -= 1; + if children[index] > 0 { + // The parent node has another child, so we cannot process it yet. + tail_candidates.push((sources[node], node)); + break; + } + // process the parent node + node = index; + } else { + // End of the path; when all the sinks will be processed, this register will be 'free' for re-use. + free_register = Some(sources[i]); + break; + } + } + if processed == n { + return; + } + } } - - // Deallocate all temporaries - for temp in temporaries { + // All sinks and their parents have been processed, remaining nodes are part of a loop + // Check if a tail_candidate is a branch to a loop + for (entry, free) in tail_candidates { + let entry_idx = Self::to_index(&entry); + if entry_idx < n && children[entry_idx] == 1 { + // Use the branch as the temporary register for the loop + self.process_loop( + entry_idx, + &Self::from_index(free), + &mut children, + &sources, + &mut processed, + ); + } + } + if processed == n { + return; + } + // Now process all the remaining loops with a temporary register if needed. + let temp_register = + if free_register.is_none() { Some(self.allocate_register()) } else { None }; + let register = free_register.unwrap_or(temp_register.unwrap()); + for i in 0..n { + if children[i] == 1 { + let src = Self::from_index(i); + // Copy the loop entry to the free register + self.mov_instruction(register, src); + self.process_loop(i, ®ister, &mut children, &sources, &mut processed); + } + } + if let Some(temp) = temp_register { self.deallocate_register(temp); } } - fn perform_movements( + /// Generates mov opcodes corresponding to a loop, given a node from the loop (entry) + /// and a register not in the loop that contains its value (free) + fn process_loop( &mut self, - movements: &HashMap>, - current_source: MemoryAddress, + entry: usize, + free: &MemoryAddress, + children: &mut [usize], + source: &[MemoryAddress], + processed: &mut usize, ) { - if let Some(destinations) = movements.get(¤t_source) { - for destination in destinations { - self.perform_movements(movements, *destination); - } - for destination in destinations { - self.mov_instruction(*destination, current_source); - } - } - } -} - -#[derive(Default)] -struct LoopDetector { - visited_sources: HashSet, - loops: Vec>, -} - -impl LoopDetector { - fn collect_loops(&mut self, movements: &HashMap>) { - for source in movements.keys() { - self.find_loop_recursive(*source, movements, im::OrdSet::default()); + let mut current = entry; + while Self::to_index(&source[current]) != entry { + self.perform_movement(current, source[current], children, processed); + current = Self::to_index(&source[current]); } + self.perform_movement(current, *free, children, processed); + children[current] = usize::MAX; } - fn find_loop_recursive( + /// Generates a move opcode from 'src' to 'dest'. + fn perform_movement( &mut self, - source: MemoryAddress, - movements: &HashMap>, - mut previous_sources: im::OrdSet, + dest: usize, + src: MemoryAddress, + children: &mut [usize], + processed: &mut usize, ) { - if self.visited_sources.contains(&source) { - return; - } - // Mark as visited - self.visited_sources.insert(source); - - previous_sources.insert(source); - // Get all destinations - 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(); - self.loops.push(loop_sources); - } else { - self.find_loop_recursive(*destination, movements, previous_sources.clone()); - } - } - } + let destination = Self::from_index(dest); + self.mov_instruction(destination, src); + // set the node as 'processed' + children[dest] = usize::MAX; + *processed += 1; } } @@ -129,69 +158,19 @@ mod tests { FieldElement, acir::brillig::{MemoryAddress, Opcode}, }; - use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::{ brillig::{ BrilligOptions, - brillig_ir::{BrilligContext, LayoutConfig, artifact::Label, registers::Stack}, + brillig_ir::{ + BrilligContext, LayoutConfig, + artifact::Label, + registers::{RegisterAllocator, Stack}, + }, }, ssa::ir::function::FunctionId, }; - // Tests for the loop finder - - fn generate_movements_map( - movements: Vec<(usize, usize)>, - ) -> HashMap> { - movements.into_iter().fold(HashMap::default(), |mut map, (source, destination)| { - map.entry(MemoryAddress::relative(source)) - .or_default() - .insert(MemoryAddress::relative(destination)); - map - }) - } - - #[test] - 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); - } - - #[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); - } - - #[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); - } - - #[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); - } - // Tests for mov_registers_to_registers fn movements_to_source_and_destinations( @@ -219,9 +198,20 @@ mod tests { } #[test] - #[should_panic(expected = "Multiple moves to the same register found")] + fn test_no_op() { + let movements = vec![(1, 1), (2, 2), (1, 3), (2, 4)]; + 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.len(), 2); + } + + #[test] + #[should_panic] fn test_mov_registers_to_registers_overwrite() { - let movements = vec![(10, 11), (12, 11), (10, 13)]; + let movements = vec![(10, 1), (12, 1), (10, 3)]; let (sources, destinations) = movements_to_source_and_destinations(movements); let mut context = create_context(); @@ -229,8 +219,9 @@ mod tests { } #[test] - fn test_mov_registers_to_registers_no_loop() { - let movements = vec![(10, 11), (11, 12), (12, 13), (13, 14)]; + fn test_basic_no_loop() { + let movements = vec![(2, 1), (3, 2), (4, 3), (5, 4)]; + // let movements_map = generate_movements_map(movements); let (sources, destinations) = movements_to_source_and_destinations(movements); let mut context = create_context(); @@ -240,52 +231,84 @@ mod tests { opcodes, vec![ Opcode::Mov { - destination: MemoryAddress::relative(14), - source: MemoryAddress::relative(13) + destination: MemoryAddress::relative(1), + source: MemoryAddress::relative(2) }, Opcode::Mov { - destination: MemoryAddress::relative(13), - source: MemoryAddress::relative(12) + destination: MemoryAddress::relative(2), + source: MemoryAddress::relative(3) }, Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) + destination: MemoryAddress::relative(3), + source: MemoryAddress::relative(4) }, Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(10) - }, + destination: MemoryAddress::relative(4), + source: MemoryAddress::relative(5) + } ] ); } + #[test] - fn test_mov_registers_to_registers_no_op_filter() { - let movements = vec![(10, 11), (11, 11), (11, 12)]; + fn test_basic_loop() { + let movements = vec![(4, 1), (1, 2), (2, 3), (3, 4)]; + let mut context = create_context(); + for _ in 0..movements.len() { + context.registers.allocate_register(); + } let (sources, destinations) = movements_to_source_and_destinations(movements); + + context.codegen_mov_registers_to_registers(sources, destinations); + let opcodes = context.artifact().byte_code; + assert_eq!(opcodes.len(), 5); + } + + #[test] + fn test_no_loop() { + let movements = vec![(6, 1), (1, 2), (2, 3), (3, 4), (4, 5)]; let mut context = create_context(); + let (sources, destinations) = movements_to_source_and_destinations(movements); + 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.len(), 5); } #[test] - fn test_mov_registers_to_registers_loop() { - let movements = vec![(10, 11), (11, 12), (12, 13), (13, 10)]; + fn test_loop_with_branch() { + let movements = vec![(3, 1), (1, 2), (2, 3), (1, 4), (4, 5)]; + let mut context = create_context(); + let (sources, destinations) = movements_to_source_and_destinations(movements); + + context.codegen_mov_registers_to_registers(sources, destinations); + let opcodes = context.artifact().byte_code; + assert_eq!(opcodes.len(), 5); + } + + #[test] + fn test_two_loops() { + let movements = vec![(3, 1), (1, 2), (2, 3), (6, 4), (4, 5), (5, 6)]; + let mut context = create_context(); + for _ in 0..movements.len() { + context.registers.allocate_register(); + } + let (sources, destinations) = movements_to_source_and_destinations(movements); + + context.codegen_mov_registers_to_registers(sources, destinations); + let opcodes = context.artifact().byte_code; + assert_eq!(opcodes.len(), 8); + } + + #[test] + fn test_another_loop_with_branch() { + let movements = vec![(2, 1), (1, 2), (2, 3), (3, 4), (4, 5)]; + let mut context = create_context(); + + let (sources, destinations) = movements_to_source_and_destinations(movements); context.codegen_mov_registers_to_registers(sources, destinations); let opcodes = context.artifact().byte_code; @@ -293,34 +316,36 @@ mod tests { opcodes, vec![ Opcode::Mov { - destination: MemoryAddress::relative(1), - source: MemoryAddress::relative(10) + destination: MemoryAddress::relative(5), + source: MemoryAddress::relative(4) }, Opcode::Mov { - destination: MemoryAddress::relative(10), - source: MemoryAddress::relative(13) + destination: MemoryAddress::relative(4), + source: MemoryAddress::relative(3) }, Opcode::Mov { - destination: MemoryAddress::relative(13), - source: MemoryAddress::relative(12) + destination: MemoryAddress::relative(3), + source: MemoryAddress::relative(2) }, Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) + destination: MemoryAddress::relative(2), + source: MemoryAddress::relative(1) }, Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(1) + destination: MemoryAddress::relative(1), + source: MemoryAddress::relative(3) } ] ); } - #[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); + fn test_one_loop() { + let movements = vec![(2, 1), (4, 2), (5, 3), (3, 4), (1, 5)]; let mut context = create_context(); + for _ in 0..movements.len() { + context.registers.allocate_register(); + } + let (sources, destinations) = movements_to_source_and_destinations(movements); context.codegen_mov_registers_to_registers(sources, destinations); let opcodes = context.artifact().byte_code; @@ -328,29 +353,29 @@ mod tests { opcodes, vec![ Opcode::Mov { - destination: MemoryAddress::relative(1), - source: MemoryAddress::relative(10) - }, // Temporary + destination: MemoryAddress::relative(6), + source: MemoryAddress::relative(1) + }, Opcode::Mov { - destination: MemoryAddress::relative(10), - source: MemoryAddress::relative(12) - }, // Branch + destination: MemoryAddress::relative(1), + source: MemoryAddress::relative(2) + }, Opcode::Mov { - destination: MemoryAddress::relative(12), - source: MemoryAddress::relative(11) - }, // Loop + destination: MemoryAddress::relative(2), + source: MemoryAddress::relative(4) + }, Opcode::Mov { - destination: MemoryAddress::relative(14), - source: MemoryAddress::relative(13) - }, // Loop + destination: MemoryAddress::relative(4), + source: MemoryAddress::relative(3) + }, Opcode::Mov { - destination: MemoryAddress::relative(11), - source: MemoryAddress::relative(1) - }, // Finish branch + destination: MemoryAddress::relative(3), + source: MemoryAddress::relative(5) + }, Opcode::Mov { - destination: MemoryAddress::relative(13), - source: MemoryAddress::relative(1) - } // Finish loop + destination: MemoryAddress::relative(5), + source: MemoryAddress::relative(6) + } ] ); } From d7861b284bdefcadb62997fff0ade82888f925ee Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 16:21:41 +0100 Subject: [PATCH 02/12] fix merge issue --- .../src/brillig/brillig_ir/codegen_stack.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 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 b3d225d411e..8eddf4dce37 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -101,7 +101,7 @@ impl BrilligContext< return; } // Now process all the remaining loops with a temporary register if needed. - let register = free_register.unwrap_or(*self.allocate_register()); + let register = free_register.unwrap_or_else(|| self.registers_mut().allocate_register()); for i in 0..n { if children[i] == 1 { let src = Self::from_index(i); @@ -253,7 +253,7 @@ mod tests { let movements = vec![(4, 1), (1, 2), (2, 3), (3, 4)]; let mut context = create_context(); for _ in 0..movements.len() { - context.registers.allocate_register(); + context.registers_mut().allocate_register(); } let (sources, destinations) = movements_to_source_and_destinations(movements); @@ -292,7 +292,7 @@ mod tests { let mut context = create_context(); for _ in 0..movements.len() { - context.registers.allocate_register(); + context.registers_mut().allocate_register(); } let (sources, destinations) = movements_to_source_and_destinations(movements); @@ -341,7 +341,7 @@ mod tests { let movements = vec![(2, 1), (4, 2), (5, 3), (3, 4), (1, 5)]; let mut context = create_context(); for _ in 0..movements.len() { - context.registers.allocate_register(); + context.registers_mut().allocate_register(); } let (sources, destinations) = movements_to_source_and_destinations(movements); From fa08af7ea78ee00efb226bded8205fc86127e348 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 17:21:51 +0100 Subject: [PATCH 03/12] end of path is the parent, not the sink! --- .../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 8eddf4dce37..19fd8556bef 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -73,7 +73,7 @@ impl BrilligContext< node = index; } else { // End of the path; when all the sinks will be processed, this register will be 'free' for re-use. - free_register = Some(sources[i]); + free_register = Some(sources[node]); break; } } From f118aef2629934dfa0bf1acdc7eebdf95e6bdc0c Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 19:18:16 +0100 Subject: [PATCH 04/12] one temp register per loop --- .../src/brillig/brillig_ir/codegen_stack.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 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 19fd8556bef..7d5c58ab8e3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -30,7 +30,6 @@ impl BrilligContext< assert_eq!(sources.len(), destinations.len()); let n = sources.len(); let start = Stack::start(); - let mut free_register = None; let mut processed = 0; // Compute the number of children for each node in the movement graph let mut children = vec![0; n]; @@ -72,8 +71,7 @@ impl BrilligContext< // process the parent node node = index; } else { - // End of the path; when all the sinks will be processed, this register will be 'free' for re-use. - free_register = Some(sources[node]); + // End of the path break; } } @@ -100,19 +98,21 @@ impl BrilligContext< if processed == n { return; } - // Now process all the remaining loops with a temporary register if needed. - let register = free_register.unwrap_or_else(|| self.registers_mut().allocate_register()); + // Now process all the remaining loops with a temporary register. + // Allocate one temporary per loop to avoid type confusion when reusing registers, + // since different loops may contain values of different types. for i in 0..n { if children[i] == 1 { let src = Self::from_index(i); - // Copy the loop entry to the free register - self.mov_instruction(register, src); - self.process_loop(i, ®ister, &mut children, &sources, &mut processed); + // Copy the loop entry to a temporary register. + // Unfortunately, we cannot use one register for all the loops + // when the sources do not have the same type + let temp_register = self.registers_mut().allocate_register(); + self.mov_instruction(temp_register, src); + self.process_loop(i, &temp_register, &mut children, &sources, &mut processed); + self.deallocate_register(temp_register); } } - if free_register.is_none() { - self.deallocate_register(register); - } } /// Generates mov opcodes corresponding to a loop, given a node from the loop (entry) From 4ce99f75aad0f330013d506369ec5d4bb94fd938 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 19:39:01 +0100 Subject: [PATCH 05/12] code review --- .../noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs | 3 +++ 1 file changed, 3 insertions(+) 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 7d5c58ab8e3..3b97e163bad 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -5,12 +5,15 @@ use crate::brillig::brillig_ir::registers::Stack; use super::{BrilligContext, debug_show::DebugToString, registers::RegisterAllocator}; impl BrilligContext { + /// Map the address so that the first register of the stack will have index 0 fn to_index(adr: &MemoryAddress) -> usize { match adr { MemoryAddress::Relative(size) => size - Stack::start(), MemoryAddress::Direct(_) => usize::MAX, } } + + /// Construct the register corresponding to the given mapped 'index' fn from_index(idx: usize) -> MemoryAddress { assert!(idx != usize::MAX, "invalid index"); MemoryAddress::Relative(idx + Stack::start()) From a9b1c9f2aefa78f9a01d5a2c81184684c3bec35d Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 19:46:11 +0100 Subject: [PATCH 06/12] code review --- .../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 3b97e163bad..c325cd21934 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -22,7 +22,7 @@ impl BrilligContext< /// This function moves values from a set of registers to another set of registers. /// It assumes that: /// - every destination needs to be written at most once. Will panic if not. - /// - destinations are relative addresses, starting from 0 and with 1 increment. Will panic if not + /// - destinations are relative addresses, starting from 1 and with 1 increment. Will panic if not /// - sources are any relative addresses, and can also be direct address to the global space. TODO: panic if not /// - sources and destinations have same length. Will panic if not. pub(crate) fn codegen_mov_registers_to_registers( From 8015e5d2ffddb92ab46c5ec5b81bc447f238384a Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 28 Oct 2025 19:48:22 +0100 Subject: [PATCH 07/12] code review --- .../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 c325cd21934..0e32201cc47 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -34,7 +34,7 @@ impl BrilligContext< let n = sources.len(); let start = Stack::start(); let mut processed = 0; - // Compute the number of children for each node in the movement graph + // Compute the number of children for each destination node in the movement graph let mut children = vec![0; n]; for i in 0..n { // Check that destinations are relatives to 0,..,n-1 From ef75afc97ea30ccde27787c523f24d63f537f8c9 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 29 Oct 2025 11:38:35 +0100 Subject: [PATCH 08/12] code review --- .../src/brillig/brillig_ir/codegen_stack.rs | 55 ++++++++++--------- 1 file changed, 29 insertions(+), 26 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 0e32201cc47..6672ac956e8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -5,20 +5,6 @@ use crate::brillig::brillig_ir::registers::Stack; use super::{BrilligContext, debug_show::DebugToString, registers::RegisterAllocator}; impl BrilligContext { - /// Map the address so that the first register of the stack will have index 0 - fn to_index(adr: &MemoryAddress) -> usize { - match adr { - MemoryAddress::Relative(size) => size - Stack::start(), - MemoryAddress::Direct(_) => usize::MAX, - } - } - - /// Construct the register corresponding to the given mapped 'index' - fn from_index(idx: usize) -> MemoryAddress { - assert!(idx != usize::MAX, "invalid index"); - MemoryAddress::Relative(idx + Stack::start()) - } - /// This function moves values from a set of registers to another set of registers. /// It assumes that: /// - every destination needs to be written at most once. Will panic if not. @@ -34,12 +20,12 @@ impl BrilligContext< let n = sources.len(); let start = Stack::start(); let mut processed = 0; - // Compute the number of children for each destination node in the movement graph + // Compute the number of children for each source node that is also a destination node (i.e within 0,..n-1) in the movement graph let mut children = vec![0; n]; for i in 0..n { // Check that destinations are relatives to 0,..,n-1 assert_eq!(destinations[i].unwrap_relative() - start, i); - let index = Self::to_index(&sources[i]); + let index = to_index(&sources[i]); if index < n && index != i { children[index] += 1; } @@ -54,7 +40,7 @@ impl BrilligContext< // A sink has no child let mut node = i; while children[node] == 0 { - if Self::to_index(&sources[node]) == node { + if to_index(&sources[node]) == node { //no-op: mark the node as processed children[node] = usize::MAX; processed += 1; @@ -63,7 +49,7 @@ impl BrilligContext< // Generates a move instruction self.perform_movement(node, sources[node], &mut children, &mut processed); // Follow the parent - let index = Self::to_index(&sources[node]); + let index = to_index(&sources[node]); if index < n { children[index] -= 1; if children[index] > 0 { @@ -86,12 +72,12 @@ impl BrilligContext< // All sinks and their parents have been processed, remaining nodes are part of a loop // Check if a tail_candidate is a branch to a loop for (entry, free) in tail_candidates { - let entry_idx = Self::to_index(&entry); + let entry_idx = to_index(&entry); if entry_idx < n && children[entry_idx] == 1 { // Use the branch as the temporary register for the loop self.process_loop( entry_idx, - &Self::from_index(free), + &from_index(free), &mut children, &sources, &mut processed, @@ -106,7 +92,7 @@ impl BrilligContext< // since different loops may contain values of different types. for i in 0..n { if children[i] == 1 { - let src = Self::from_index(i); + let src = from_index(i); // Copy the loop entry to a temporary register. // Unfortunately, we cannot use one register for all the loops // when the sources do not have the same type @@ -114,6 +100,9 @@ impl BrilligContext< self.mov_instruction(temp_register, src); self.process_loop(i, &temp_register, &mut children, &sources, &mut processed); self.deallocate_register(temp_register); + } else { + // Nodes must have been processed, or are part of a loop. + assert_eq!(children[i], usize::MAX); } } } @@ -129,12 +118,11 @@ impl BrilligContext< processed: &mut usize, ) { let mut current = entry; - while Self::to_index(&source[current]) != entry { + while to_index(&source[current]) != entry { self.perform_movement(current, source[current], children, processed); - current = Self::to_index(&source[current]); + current = to_index(&source[current]); } self.perform_movement(current, *free, children, processed); - children[current] = usize::MAX; } /// Generates a move opcode from 'src' to 'dest'. @@ -145,7 +133,7 @@ impl BrilligContext< children: &mut [usize], processed: &mut usize, ) { - let destination = Self::from_index(dest); + let destination = from_index(dest); self.mov_instruction(destination, src); // set the node as 'processed' children[dest] = usize::MAX; @@ -153,6 +141,20 @@ impl BrilligContext< } } +/// Map the address so that the first register of the stack will have index 0 +fn to_index(adr: &MemoryAddress) -> usize { + match adr { + MemoryAddress::Relative(size) => size - Stack::start(), + MemoryAddress::Direct(_) => usize::MAX, + } +} + +/// Construct the register corresponding to the given mapped 'index' +fn from_index(idx: usize) -> MemoryAddress { + assert!(idx != usize::MAX, "invalid index"); + MemoryAddress::Relative(idx + Stack::start()) +} + #[cfg(test)] mod tests { use acvm::{ @@ -222,7 +224,6 @@ mod tests { #[test] fn test_basic_no_loop() { let movements = vec![(2, 1), (3, 2), (4, 3), (5, 4)]; - // let movements_map = generate_movements_map(movements); let (sources, destinations) = movements_to_source_and_destinations(movements); let mut context = create_context(); @@ -353,6 +354,7 @@ mod tests { assert_eq!( opcodes, vec![ + // Save to a temporary register (6) before processing the loop. Opcode::Mov { destination: MemoryAddress::relative(6), source: MemoryAddress::relative(1) @@ -373,6 +375,7 @@ mod tests { destination: MemoryAddress::relative(3), source: MemoryAddress::relative(5) }, + // Copy back from the temporary register at the end of the loop. Opcode::Mov { destination: MemoryAddress::relative(5), source: MemoryAddress::relative(6) From 16ed5e205480fa7b6243f77ad7f5d9a3e17c7cbb Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 29 Oct 2025 12:48:52 +0100 Subject: [PATCH 09/12] optional index --- .../src/brillig/brillig_ir/codegen_stack.rs | 46 ++++++++++--------- 1 file changed, 24 insertions(+), 22 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 26de00ae4bf..69c7d36b0b4 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -26,9 +26,10 @@ impl BrilligContext< for i in 0..n { // Check that destinations are relatives to 0,..,n-1 assert_eq!(destinations[i].unwrap_relative() - start, i); - let index = to_index(&sources[i]); - if index < n && index != i { - children[index] += 1; + if let Some(index) = to_index(&sources[i]) { + if index < n && index != i { + children[index] += 1; + } } } @@ -41,7 +42,7 @@ impl BrilligContext< // A sink has no child let mut node = i; while children[node] == 0 { - if to_index(&sources[node]) == node { + if to_index(&sources[node]) == Some(node) { //no-op: mark the node as processed children[node] = usize::MAX; processed += 1; @@ -50,20 +51,21 @@ impl BrilligContext< // Generates a move instruction self.perform_movement(node, sources[node], &mut children, &mut processed); // Follow the parent - let index = to_index(&sources[node]); - if index < n { - children[index] -= 1; - if children[index] > 0 { - // The parent node has another child, so we cannot process it yet. - tail_candidates.push((sources[node], node)); - break; + if let Some(index) = to_index(&sources[node]) { + if index < n { + children[index] -= 1; + if children[index] > 0 { + // The parent node has another child, so we cannot process it yet. + tail_candidates.push((sources[node], node)); + break; + } + // process the parent node + node = index; + continue; } - // process the parent node - node = index; - } else { - // End of the path - break; } + // End of the path + break; } if processed == n { return; @@ -73,7 +75,7 @@ impl BrilligContext< // All sinks and their parents have been processed, remaining nodes are part of a loop // Check if a tail_candidate is a branch to a loop for (entry, free) in tail_candidates { - let entry_idx = to_index(&entry); + let entry_idx = to_index(&entry).unwrap(); if entry_idx < n && children[entry_idx] == 1 { // Use the branch as the temporary register for the loop self.process_loop( @@ -119,9 +121,9 @@ impl BrilligContext< processed: &mut usize, ) { let mut current = entry; - while to_index(&source[current]) != entry { + while to_index(&source[current]).unwrap() != entry { self.perform_movement(current, source[current], children, processed); - current = to_index(&source[current]); + current = to_index(&source[current]).unwrap(); } self.perform_movement(current, *free, children, processed); } @@ -143,10 +145,10 @@ impl BrilligContext< } /// Map the address so that the first register of the stack will have index 0 -fn to_index(adr: &MemoryAddress) -> usize { +fn to_index(adr: &MemoryAddress) -> Option { match adr { - MemoryAddress::Relative(size) => size - Stack::start(), - MemoryAddress::Direct(_) => usize::MAX, + MemoryAddress::Relative(size) => Some(size - Stack::start()), + MemoryAddress::Direct(_) => None, } } From 927398abbcb1906a03856aa6d07b1300087f0a1f Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 11 Nov 2025 19:55:32 +0100 Subject: [PATCH 10/12] code review --- .../src/brillig/brillig_ir/codegen_stack.rs | 56 +++++++++---------- 1 file changed, 27 insertions(+), 29 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 69c7d36b0b4..838d59c9a8d 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -38,38 +38,36 @@ impl BrilligContext< let mut tail_candidates = Vec::new(); for i in 0..n { // Generate a movement for each sink in the graph - if children[i] == 0 { - // A sink has no child - let mut node = i; - while children[node] == 0 { - if to_index(&sources[node]) == Some(node) { - //no-op: mark the node as processed - children[node] = usize::MAX; - processed += 1; - break; - } - // Generates a move instruction - self.perform_movement(node, sources[node], &mut children, &mut processed); - // Follow the parent - if let Some(index) = to_index(&sources[node]) { - if index < n { - children[index] -= 1; - if children[index] > 0 { - // The parent node has another child, so we cannot process it yet. - tail_candidates.push((sources[node], node)); - break; - } - // process the parent node - node = index; - continue; - } - } - // End of the path + let mut node = i; + // A sink has no child + while children[node] == 0 { + if to_index(&sources[node]) == Some(node) { + //no-op: mark the node as processed + children[node] = usize::MAX; + processed += 1; break; } - if processed == n { - return; + // Generates a move instruction + self.perform_movement(node, sources[node], &mut children, &mut processed); + // Follow the parent + if let Some(index) = to_index(&sources[node]) { + if index < n { + children[index] -= 1; + if children[index] > 0 { + // The parent node has another child, so we cannot process it yet. + tail_candidates.push((sources[node], node)); + break; + } + // process the parent node + node = index; + continue; + } } + // End of the path + break; + } + if processed == n { + return; } } // All sinks and their parents have been processed, remaining nodes are part of a loop From a8ee54aaafbe884b34147ccd89f3310bda2afc40 Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Wed, 12 Nov 2025 20:14:47 +0100 Subject: [PATCH 11/12] Update compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs Co-authored-by: Maxim Vezenov --- .../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 838d59c9a8d..28b0b610ce9 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -9,7 +9,7 @@ impl BrilligContext< /// This function moves values from a set of registers to another set of registers. /// It assumes that: /// - every destination needs to be written at most once. Will panic if not. - /// - destinations are relative addresses, starting from 1 and with 1 increment. Will panic if not + /// - destinations are relative addresses, starting from 1 and consecutively incremented by one. Will panic if not /// - sources are any relative addresses, and can also be direct address to the global space. TODO: panic if not /// - sources and destinations have same length. Will panic if not. pub(crate) fn codegen_mov_registers_to_registers( From ca7b53ec0567da267bd66b6546e190d8a1ea86d1 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 12 Nov 2025 20:46:48 +0100 Subject: [PATCH 12/12] code review --- .../src/brillig/brillig_ir/codegen_stack.rs | 63 ++++++++++++------- 1 file changed, 42 insertions(+), 21 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 28b0b610ce9..67d1cb4ccf8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -19,41 +19,40 @@ impl BrilligContext< ) { assert_eq!(sources.len(), destinations.len(), "sources and destinations length must match"); let n = sources.len(); - let start = Stack::start(); let mut processed = 0; - // Compute the number of children for each source node that is also a destination node (i.e within 0,..n-1) in the movement graph - let mut children = vec![0; n]; + // Compute the number of destinations for each source node that is also a destination node (i.e within 0,..n-1) in the movement graph + let mut num_destinations = vec![0; n]; for i in 0..n { // Check that destinations are relatives to 0,..,n-1 - assert_eq!(destinations[i].unwrap_relative() - start, i); + assert_eq!(to_index(&destinations[i]), Some(i)); if let Some(index) = to_index(&sources[i]) { if index < n && index != i { - children[index] += 1; + num_destinations[index] += 1; } } } // Process all sinks in the graph and follow their parents. - // Keep track of nodes having more than 2 children, in case they belong to a loop. + // Keep track of nodes having more than 2 destinations, in case they belong to a loop. let mut tail_candidates = Vec::new(); for i in 0..n { // Generate a movement for each sink in the graph let mut node = i; // A sink has no child - while children[node] == 0 { + while num_destinations[node] == 0 { if to_index(&sources[node]) == Some(node) { //no-op: mark the node as processed - children[node] = usize::MAX; + num_destinations[node] = usize::MAX; processed += 1; break; } // Generates a move instruction - self.perform_movement(node, sources[node], &mut children, &mut processed); + self.perform_movement(node, sources[node], &mut num_destinations, &mut processed); // Follow the parent if let Some(index) = to_index(&sources[node]) { if index < n { - children[index] -= 1; - if children[index] > 0 { + num_destinations[index] -= 1; + if num_destinations[index] > 0 { // The parent node has another child, so we cannot process it yet. tail_candidates.push((sources[node], node)); break; @@ -74,12 +73,12 @@ impl BrilligContext< // Check if a tail_candidate is a branch to a loop for (entry, free) in tail_candidates { let entry_idx = to_index(&entry).unwrap(); - if entry_idx < n && children[entry_idx] == 1 { + if entry_idx < n && num_destinations[entry_idx] == 1 { // Use the branch as the temporary register for the loop self.process_loop( entry_idx, &from_index(free), - &mut children, + &mut num_destinations, sources, &mut processed, ); @@ -92,18 +91,24 @@ impl BrilligContext< // Allocate one temporary per loop to avoid type confusion when reusing registers, // since different loops may contain values of different types. for i in 0..n { - if children[i] == 1 { + if num_destinations[i] == 1 { let src = from_index(i); // Copy the loop entry to a temporary register. // Unfortunately, we cannot use one register for all the loops // when the sources do not have the same type let temp_register = self.registers_mut().allocate_register(); self.mov_instruction(temp_register, src); - self.process_loop(i, &temp_register, &mut children, sources, &mut processed); + self.process_loop( + i, + &temp_register, + &mut num_destinations, + sources, + &mut processed, + ); self.deallocate_register(temp_register); } else { // Nodes must have been processed, or are part of a loop. - assert_eq!(children[i], usize::MAX); + assert_eq!(num_destinations[i], usize::MAX); } } } @@ -114,16 +119,16 @@ impl BrilligContext< &mut self, entry: usize, free: &MemoryAddress, - children: &mut [usize], + num_destinations: &mut [usize], source: &[MemoryAddress], processed: &mut usize, ) { let mut current = entry; while to_index(&source[current]).unwrap() != entry { - self.perform_movement(current, source[current], children, processed); + self.perform_movement(current, source[current], num_destinations, processed); current = to_index(&source[current]).unwrap(); } - self.perform_movement(current, *free, children, processed); + self.perform_movement(current, *free, num_destinations, processed); } /// Generates a move opcode from 'src' to 'dest'. @@ -131,13 +136,13 @@ impl BrilligContext< &mut self, dest: usize, src: MemoryAddress, - children: &mut [usize], + num_destinations: &mut [usize], processed: &mut usize, ) { let destination = from_index(dest); self.mov_instruction(destination, src); // set the node as 'processed' - children[dest] = usize::MAX; + num_destinations[dest] = usize::MAX; *processed += 1; } } @@ -290,6 +295,22 @@ mod tests { assert_generated_opcodes(movements, vec![(1, 6), (2, 1), (4, 2), (3, 4), (5, 3), (6, 5)]); } + /// This creates a chain (N+1)->1->2->...->N where N is large enough to overflow the stack + #[test] + fn test_deep_chain() { + // Each movement is i -> i+1, creating a single long chain + const CHAIN_DEPTH: usize = 10_000; + + let mut movements: Vec<(usize, usize)> = (0..CHAIN_DEPTH).map(|i| (i, i + 1)).collect(); + movements[0] = (CHAIN_DEPTH + 1, 1); + let (sources, destinations) = movements_to_source_and_destinations(movements); + + let mut context = create_context(); + + // This should overflow the stack with recursive implementation + context.codegen_mov_registers_to_registers(&sources, &destinations); + } + #[test] fn prop_mov_registers_to_registers() { const MEM_SIZE: usize = 10;