From bffe5a3bb6c8caf957ee38cbd7f10cdff5ee1253 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 18:57:35 +0000 Subject: [PATCH 1/7] feat: Implement solver for mov_registers_to_registers --- .../src/brillig/brillig_ir/codegen_stack.rs | 37 ++- .../codegen_stack/mov_registers_solver.rs | 216 ++++++++++++++++++ 2 files changed, 242 insertions(+), 11 deletions(-) create mode 100644 compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs 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 945b768efcf..fc6bfe82c47 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -1,4 +1,7 @@ +mod mov_registers_solver; + use acvm::{acir::brillig::MemoryAddress, AcirField}; +use mov_registers_solver::{is_loop, MoveRegistersSolver}; use super::{debug_show::DebugToString, registers::RegisterAllocator, BrilligContext}; @@ -10,17 +13,29 @@ impl BrilligContext< sources: Vec, destinations: Vec, ) { - let new_sources: Vec<_> = sources - .iter() - .map(|source| { - let new_source = self.allocate_register(); - self.mov_instruction(new_source, *source); - new_source - }) - .collect(); - for (new_source, destination) in new_sources.iter().zip(destinations.iter()) { - self.mov_instruction(*destination, *new_source); - self.deallocate_register(*new_source); + let chains = + MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); + for mut chain in chains { + assert!(chain.len() > 0, "Empty chain found"); + + // If the chain is a loop, we need a temporary register to break the loop + if is_loop(&chain) { + let temp_register = self.allocate_register(); + // Backup the first destination + self.mov_instruction(temp_register, chain[0].1); + // Do all operations but the last one + let last = chain.pop().unwrap(); + for (source, destination) in chain { + self.mov_instruction(destination, source); + } + // Move the backup to the last destination + self.mov_instruction(last.1, temp_register); + self.deallocate_register(temp_register); + } else { + for (source, destination) in chain { + self.mov_instruction(destination, source); + } + } } } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs new file mode 100644 index 00000000000..d4365e976c2 --- /dev/null +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs @@ -0,0 +1,216 @@ +use acvm::acir::brillig::MemoryAddress; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use im::Vector; + +type MoveChain = Vec<(MemoryAddress, MemoryAddress)>; + +pub(crate) struct MoveRegistersSolver { + // The source addresses and their corresponding destinations + movements: HashMap, + // The set of destination addresses + destinations: HashSet, + // The source addresses that have been visited + visited: HashSet, + // The chains of dependencies that we have found + dependency_chain: Vec>, +} + +impl MoveRegistersSolver { + pub(crate) fn sources_destinations_to_move_chains( + sources: Vec, + destinations: Vec, + ) -> Vec { + assert_eq!( + sources.len(), + destinations.len(), + "Different number of sources and destinations", + ); + // Filter no-op movements + let movements: Vec<_> = sources + .into_iter() + .zip(destinations.into_iter()) + .filter(|(source, destination)| source != destination) + .collect(); + + let total_movements = movements.len(); + let movements: HashMap<_, _> = movements.into_iter().collect(); + assert!(total_movements == movements.len(), "Multiple moves from the same register found"); + + // We collect the sources again after filtering no-ops + let sources = movements.keys().copied().collect(); + let destinations: HashSet<_> = movements.values().copied().collect(); + assert!(destinations.len() == total_movements, "Multiple moves to the same register found"); + + let mut solver = MoveRegistersSolver { + movements, + visited: HashSet::default(), + destinations, + dependency_chain: Vec::new(), + }; + + solver.solve(sources); + // Map the dependency chains to a chain of operations to perform + solver + .dependency_chain + .into_iter() + .map(|chain| { + chain + .into_iter() + .map(|source| { + let destination = solver.movements.get(&source).unwrap(); + (source, *destination) + }) + .rev() // We reverse the chain to express what needs to be applied first + .collect() + }) + .collect() + } + + fn solve(&mut self, sources: Vec) { + // First, we'll find all the non-cyclic chains of movements. + // All chains start with a source that is not written to + let chain_heads: Vec<_> = + sources.iter().filter(|source| !self.destinations.contains(source)).copied().collect(); + + for source in chain_heads { + self.explore(source, Vector::default()); + } + + // Then we handle the cycles + for &source in sources.iter() { + if self.visited.contains(&source) { + continue; + } + self.explore(source, Vector::new()); + } + } + + fn explore( + &mut self, + current_source: MemoryAddress, + mut current_dependency_chain: Vector, + ) { + // Record that we visited this source + self.visited.insert(current_source); + current_dependency_chain.push_back(current_source); + + // We need to check the target that this source is moving to + let target = self.movements.get(¤t_source).unwrap(); + let is_target_source = self.movements.contains_key(target); + + // Check if we are at the end of a chain or in a cycle + if !is_target_source + || current_dependency_chain + .get(0) + .and_then(|first: &MemoryAddress| Some(first == target)) + .unwrap_or(false) + { + self.dependency_chain.push(current_dependency_chain); + return; + } + + // Safety check that we are properly detecting cycles + assert!(!self.visited.contains(target), "Movement cycle went undetected"); + // If the target is also a non visited source, then we need to explore it + self.explore(*target, current_dependency_chain.clone()); + } +} + +/// We have a loop if the destination of the first movement needs to be the source for the last +pub(crate) fn is_loop(chain: &MoveChain) -> bool { + let first = chain.first().unwrap().1; + let last = chain.last().unwrap().0; + first == last +} + +#[cfg(test)] +mod tests { + use super::*; + + fn movements_to_source_and_destinations( + movements: Vec<(usize, usize)>, + ) -> (Vec, Vec) { + let sources = movements.iter().map(|(source, _)| MemoryAddress::from(*source)).collect(); + let destinations = + movements.iter().map(|(_, destination)| MemoryAddress::from(*destination)).collect(); + (sources, destinations) + } + + fn create_move_chains(operations: Vec>) -> Vec { + operations + .into_iter() + .map(|ops| { + ops.into_iter() + .map(|(source, destination)| { + (MemoryAddress::from(source), MemoryAddress::from(destination)) + }) + .collect() + }) + .collect() + } + + #[test] + fn test_solver_simple_op() { + let chains = MoveRegistersSolver::sources_destinations_to_move_chains( + vec![MemoryAddress(1)], + vec![MemoryAddress(5)], + ); + assert_eq!(chains, create_move_chains(vec![vec![(1, 5)]])); + assert!(!is_loop(&chains[0])); + } + + #[test] + fn test_solver_simple_loop() { + let movements = vec![(4, 5), (1, 2), (3, 4), (2, 3), (5, 1)]; + let (sources, destinations) = movements_to_source_and_destinations(movements); + let chains = + MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); + assert_eq!(chains, create_move_chains(vec![vec![(4, 5), (3, 4), (2, 3), (1, 2), (5, 1)]])); + assert!(is_loop(&chains[0])); + } + + #[test] + fn test_solver_simple_chain() { + let movements = vec![(2, 3), (3, 4), (1, 2), (4, 5)]; + let (sources, destinations) = movements_to_source_and_destinations(movements); + let chains = + MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); + assert_eq!(chains, create_move_chains(vec![vec![(4, 5), (3, 4), (2, 3), (1, 2),]])); + assert!(!is_loop(&chains[0])); + } + + #[test] + fn test_solver_chain_and_loop() { + let movements = vec![(2, 3), (3, 4), (1, 5), (4, 2)]; + let (sources, destinations) = movements_to_source_and_destinations(movements); + let chains = + MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); + assert_eq!(chains, create_move_chains(vec![vec![(1, 5)], vec![(4, 2), (3, 4), (2, 3)]])); + assert!(is_loop(&chains[1])); + } + + #[test] + fn test_no_op() { + let chains = MoveRegistersSolver::sources_destinations_to_move_chains( + vec![MemoryAddress(1)], + vec![MemoryAddress(1)], + ); + assert!(chains.is_empty()); + } + + #[test] + #[should_panic(expected = "Multiple moves from the same register found")] + fn test_multiple_destinations() { + let movements = vec![(1, 2), (1, 3)]; + let (sources, destinations) = movements_to_source_and_destinations(movements); + MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); + } + + #[test] + #[should_panic(expected = "Multiple moves to the same register found")] + fn test_multiple_sources() { + let movements = vec![(1, 2), (3, 2)]; + let (sources, destinations) = movements_to_source_and_destinations(movements); + MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); + } +} From de102abb9910744e386cedf8ad3dfdcae8dcd278 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 19:08:59 +0000 Subject: [PATCH 2/7] clippy --- .../src/brillig/brillig_ir/codegen_stack.rs | 2 +- .../brillig_ir/codegen_stack/mov_registers_solver.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 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 fc6bfe82c47..a09a8e0419a 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -16,7 +16,7 @@ impl BrilligContext< let chains = MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); for mut chain in chains { - assert!(chain.len() > 0, "Empty chain found"); + assert!(!chain.is_empty(), "Empty chain found"); // If the chain is a loop, we need a temporary register to break the loop if is_loop(&chain) { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs index d4365e976c2..717cd389c49 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs @@ -12,7 +12,7 @@ pub(crate) struct MoveRegistersSolver { // The source addresses that have been visited visited: HashSet, // The chains of dependencies that we have found - dependency_chain: Vec>, + dependency_chains: Vec>, } impl MoveRegistersSolver { @@ -28,7 +28,7 @@ impl MoveRegistersSolver { // Filter no-op movements let movements: Vec<_> = sources .into_iter() - .zip(destinations.into_iter()) + .zip(destinations) .filter(|(source, destination)| source != destination) .collect(); @@ -45,13 +45,13 @@ impl MoveRegistersSolver { movements, visited: HashSet::default(), destinations, - dependency_chain: Vec::new(), + dependency_chains: Vec::new(), }; solver.solve(sources); // Map the dependency chains to a chain of operations to perform solver - .dependency_chain + .dependency_chains .into_iter() .map(|chain| { chain @@ -102,10 +102,10 @@ impl MoveRegistersSolver { if !is_target_source || current_dependency_chain .get(0) - .and_then(|first: &MemoryAddress| Some(first == target)) + .map(|first: &MemoryAddress| first == target) .unwrap_or(false) { - self.dependency_chain.push(current_dependency_chain); + self.dependency_chains.push(current_dependency_chain); return; } From 470076ec3d8f5dbf278d14571c64d10e77064516 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 23:11:24 +0000 Subject: [PATCH 3/7] feat: add support for same source multiple destinations --- .../src/brillig/brillig_ir/codegen_stack.rs | 285 ++++++++++++++++-- .../codegen_stack/mov_registers_solver.rs | 216 ------------- 2 files changed, 260 insertions(+), 241 deletions(-) delete mode 100644 compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs 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 a09a8e0419a..ab5d1304b26 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -1,41 +1,276 @@ -mod mov_registers_solver; - use acvm::{acir::brillig::MemoryAddress, AcirField}; -use mov_registers_solver::{is_loop, MoveRegistersSolver}; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use super::{debug_show::DebugToString, registers::RegisterAllocator, BrilligContext}; impl BrilligContext { /// This function moves values from a set of registers to another set of registers. - /// It first moves all sources to new allocated registers to avoid overwriting. + /// The only requirement is that every destination needs to be written at most once. pub(crate) fn codegen_mov_registers_to_registers( &mut self, sources: Vec, destinations: Vec, ) { - let chains = - MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); - for mut chain in chains { - assert!(!chain.is_empty(), "Empty chain found"); - - // If the chain is a loop, we need a temporary register to break the loop - if is_loop(&chain) { - let temp_register = self.allocate_register(); - // Backup the first destination - self.mov_instruction(temp_register, chain[0].1); - // Do all operations but the last one - let last = chain.pop().unwrap(); - for (source, destination) in chain { - self.mov_instruction(destination, source); - } - // Move the backup to the last destination - self.mov_instruction(last.1, temp_register); - self.deallocate_register(temp_register); - } else { - for (source, destination) in chain { - self.mov_instruction(destination, source); + 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 = + movements.into_iter().fold(HashMap::default(), |mut map, (source, destination)| { + map.entry(source).or_insert_with(HashSet::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); + } + // 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); + } + + // Deallocate all temporaries + for temp in temporaries { + self.deallocate_register(temp); + } + } + + fn perform_movements( + &mut self, + movements: &HashMap>, + current_source: MemoryAddress, + ) { + 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()); + } + } + + fn find_loop_recursive( + &mut self, + source: MemoryAddress, + movements: &HashMap>, + mut previous_sources: im::OrdSet, + ) { + 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()); } } } } } + +#[cfg(test)] +mod tests { + use acvm::{ + acir::brillig::{MemoryAddress, Opcode}, + FieldElement, + }; + use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; + + use crate::{ + brillig::brillig_ir::{artifact::Label, registers::Stack, BrilligContext}, + 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(source)) + .or_insert_with(HashSet::default) + .insert(MemoryAddress(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( + movements: Vec<(usize, usize)>, + ) -> (Vec, Vec) { + let sources = movements.iter().map(|(source, _)| MemoryAddress::from(*source)).collect(); + let destinations = + movements.iter().map(|(_, destination)| MemoryAddress::from(*destination)).collect(); + (sources, destinations) + } + + pub(crate) fn create_context() -> BrilligContext { + let mut context = BrilligContext::new(true); + context.enter_context(Label::function(FunctionId::test_new(0))); + context + } + + #[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); + } + + #[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, + vec![ + Opcode::Mov { destination: MemoryAddress(14), source: MemoryAddress(13) }, + Opcode::Mov { destination: MemoryAddress(13), source: MemoryAddress(12) }, + Opcode::Mov { destination: MemoryAddress(12), source: MemoryAddress(11) }, + Opcode::Mov { destination: MemoryAddress(11), source: MemoryAddress(10) }, + ] + ); + } + + #[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, + vec![ + Opcode::Mov { destination: MemoryAddress(3), source: MemoryAddress(10) }, + Opcode::Mov { destination: MemoryAddress(10), source: MemoryAddress(13) }, + Opcode::Mov { destination: MemoryAddress(13), source: MemoryAddress(12) }, + Opcode::Mov { destination: MemoryAddress(12), source: MemoryAddress(11) }, + Opcode::Mov { destination: MemoryAddress(11), source: MemoryAddress(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); + let mut context = create_context(); + + context.codegen_mov_registers_to_registers(sources, destinations); + let opcodes = context.artifact().byte_code; + assert_eq!( + opcodes, + vec![ + Opcode::Mov { destination: MemoryAddress(3), source: MemoryAddress(10) }, // Temporary + Opcode::Mov { destination: MemoryAddress(14), source: MemoryAddress(13) }, // Branch + Opcode::Mov { destination: MemoryAddress(10), source: MemoryAddress(12) }, // Loop + Opcode::Mov { destination: MemoryAddress(12), source: MemoryAddress(11) }, // Loop + Opcode::Mov { destination: MemoryAddress(13), source: MemoryAddress(3) }, // Finish branch + Opcode::Mov { destination: MemoryAddress(11), source: MemoryAddress(3) } // Finish loop + ] + ); + } +} diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs deleted file mode 100644 index 717cd389c49..00000000000 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack/mov_registers_solver.rs +++ /dev/null @@ -1,216 +0,0 @@ -use acvm::acir::brillig::MemoryAddress; -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; -use im::Vector; - -type MoveChain = Vec<(MemoryAddress, MemoryAddress)>; - -pub(crate) struct MoveRegistersSolver { - // The source addresses and their corresponding destinations - movements: HashMap, - // The set of destination addresses - destinations: HashSet, - // The source addresses that have been visited - visited: HashSet, - // The chains of dependencies that we have found - dependency_chains: Vec>, -} - -impl MoveRegistersSolver { - pub(crate) fn sources_destinations_to_move_chains( - sources: Vec, - destinations: Vec, - ) -> Vec { - assert_eq!( - sources.len(), - destinations.len(), - "Different number of sources and destinations", - ); - // Filter no-op movements - let movements: Vec<_> = sources - .into_iter() - .zip(destinations) - .filter(|(source, destination)| source != destination) - .collect(); - - let total_movements = movements.len(); - let movements: HashMap<_, _> = movements.into_iter().collect(); - assert!(total_movements == movements.len(), "Multiple moves from the same register found"); - - // We collect the sources again after filtering no-ops - let sources = movements.keys().copied().collect(); - let destinations: HashSet<_> = movements.values().copied().collect(); - assert!(destinations.len() == total_movements, "Multiple moves to the same register found"); - - let mut solver = MoveRegistersSolver { - movements, - visited: HashSet::default(), - destinations, - dependency_chains: Vec::new(), - }; - - solver.solve(sources); - // Map the dependency chains to a chain of operations to perform - solver - .dependency_chains - .into_iter() - .map(|chain| { - chain - .into_iter() - .map(|source| { - let destination = solver.movements.get(&source).unwrap(); - (source, *destination) - }) - .rev() // We reverse the chain to express what needs to be applied first - .collect() - }) - .collect() - } - - fn solve(&mut self, sources: Vec) { - // First, we'll find all the non-cyclic chains of movements. - // All chains start with a source that is not written to - let chain_heads: Vec<_> = - sources.iter().filter(|source| !self.destinations.contains(source)).copied().collect(); - - for source in chain_heads { - self.explore(source, Vector::default()); - } - - // Then we handle the cycles - for &source in sources.iter() { - if self.visited.contains(&source) { - continue; - } - self.explore(source, Vector::new()); - } - } - - fn explore( - &mut self, - current_source: MemoryAddress, - mut current_dependency_chain: Vector, - ) { - // Record that we visited this source - self.visited.insert(current_source); - current_dependency_chain.push_back(current_source); - - // We need to check the target that this source is moving to - let target = self.movements.get(¤t_source).unwrap(); - let is_target_source = self.movements.contains_key(target); - - // Check if we are at the end of a chain or in a cycle - if !is_target_source - || current_dependency_chain - .get(0) - .map(|first: &MemoryAddress| first == target) - .unwrap_or(false) - { - self.dependency_chains.push(current_dependency_chain); - return; - } - - // Safety check that we are properly detecting cycles - assert!(!self.visited.contains(target), "Movement cycle went undetected"); - // If the target is also a non visited source, then we need to explore it - self.explore(*target, current_dependency_chain.clone()); - } -} - -/// We have a loop if the destination of the first movement needs to be the source for the last -pub(crate) fn is_loop(chain: &MoveChain) -> bool { - let first = chain.first().unwrap().1; - let last = chain.last().unwrap().0; - first == last -} - -#[cfg(test)] -mod tests { - use super::*; - - fn movements_to_source_and_destinations( - movements: Vec<(usize, usize)>, - ) -> (Vec, Vec) { - let sources = movements.iter().map(|(source, _)| MemoryAddress::from(*source)).collect(); - let destinations = - movements.iter().map(|(_, destination)| MemoryAddress::from(*destination)).collect(); - (sources, destinations) - } - - fn create_move_chains(operations: Vec>) -> Vec { - operations - .into_iter() - .map(|ops| { - ops.into_iter() - .map(|(source, destination)| { - (MemoryAddress::from(source), MemoryAddress::from(destination)) - }) - .collect() - }) - .collect() - } - - #[test] - fn test_solver_simple_op() { - let chains = MoveRegistersSolver::sources_destinations_to_move_chains( - vec![MemoryAddress(1)], - vec![MemoryAddress(5)], - ); - assert_eq!(chains, create_move_chains(vec![vec![(1, 5)]])); - assert!(!is_loop(&chains[0])); - } - - #[test] - fn test_solver_simple_loop() { - let movements = vec![(4, 5), (1, 2), (3, 4), (2, 3), (5, 1)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let chains = - MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); - assert_eq!(chains, create_move_chains(vec![vec![(4, 5), (3, 4), (2, 3), (1, 2), (5, 1)]])); - assert!(is_loop(&chains[0])); - } - - #[test] - fn test_solver_simple_chain() { - let movements = vec![(2, 3), (3, 4), (1, 2), (4, 5)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let chains = - MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); - assert_eq!(chains, create_move_chains(vec![vec![(4, 5), (3, 4), (2, 3), (1, 2),]])); - assert!(!is_loop(&chains[0])); - } - - #[test] - fn test_solver_chain_and_loop() { - let movements = vec![(2, 3), (3, 4), (1, 5), (4, 2)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - let chains = - MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); - assert_eq!(chains, create_move_chains(vec![vec![(1, 5)], vec![(4, 2), (3, 4), (2, 3)]])); - assert!(is_loop(&chains[1])); - } - - #[test] - fn test_no_op() { - let chains = MoveRegistersSolver::sources_destinations_to_move_chains( - vec![MemoryAddress(1)], - vec![MemoryAddress(1)], - ); - assert!(chains.is_empty()); - } - - #[test] - #[should_panic(expected = "Multiple moves from the same register found")] - fn test_multiple_destinations() { - let movements = vec![(1, 2), (1, 3)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); - } - - #[test] - #[should_panic(expected = "Multiple moves to the same register found")] - fn test_multiple_sources() { - let movements = vec![(1, 2), (3, 2)]; - let (sources, destinations) = movements_to_source_and_destinations(movements); - MoveRegistersSolver::sources_destinations_to_move_chains(sources, destinations); - } -} From 01e16da1f48f42cca9d52a08fb6c2482e3cd9a4e Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 23:13:16 +0000 Subject: [PATCH 4/7] moar tests --- .../src/brillig/brillig_ir/codegen_stack.rs | 16 ++++++++++++++++ 1 file changed, 16 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 ab5d1304b26..04896ddefeb 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -232,6 +232,22 @@ mod tests { ] ); } + #[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, + vec![ + Opcode::Mov { destination: MemoryAddress(12), source: MemoryAddress(11) }, + Opcode::Mov { destination: MemoryAddress(11), source: MemoryAddress(10) }, + ] + ); + } #[test] fn test_mov_registers_to_registers_loop() { From bae05375fda3573eb1391168fa28cb9df5a8a197 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 23:18:22 +0000 Subject: [PATCH 5/7] clippy --- .../noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 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 04896ddefeb..edf59153d99 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -21,9 +21,9 @@ 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 = + let mut movements_map: HashMap> = movements.into_iter().fold(HashMap::default(), |mut map, (source, destination)| { - map.entry(source).or_insert_with(HashSet::default).insert(destination); + map.entry(source).or_default().insert(destination); map }); From ae8689ca9962f5abecb235be5942616053dcce8e Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 23:20:13 +0000 Subject: [PATCH 6/7] clippy --- .../noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs | 4 +--- 1 file changed, 1 insertion(+), 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 edf59153d99..b7b25c6db49 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_stack.rs @@ -140,9 +140,7 @@ mod tests { movements: Vec<(usize, usize)>, ) -> HashMap> { movements.into_iter().fold(HashMap::default(), |mut map, (source, destination)| { - map.entry(MemoryAddress(source)) - .or_insert_with(HashSet::default) - .insert(MemoryAddress(destination)); + map.entry(MemoryAddress(source)).or_default().insert(MemoryAddress(destination)); map }) } From f7f38f50f43efcb1583fd0576740644e4c6ecdcc Mon Sep 17 00:00:00 2001 From: sirasistant Date: Wed, 18 Sep 2024 23:30:18 +0000 Subject: [PATCH 7/7] fmt --- test_programs/compile_success_empty/type_path/src/main.nr | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test_programs/compile_success_empty/type_path/src/main.nr b/test_programs/compile_success_empty/type_path/src/main.nr index 3b9c55b119d..96f3a29d96b 100644 --- a/test_programs/compile_success_empty/type_path/src/main.nr +++ b/test_programs/compile_success_empty/type_path/src/main.nr @@ -11,6 +11,5 @@ fn main() { struct Foo {} impl Foo { - fn static() { - } + fn static() {} }