From 767a20934cf51a6a1c88c43fc94b32659f8c612b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 22:29:20 +0000 Subject: [PATCH 1/7] check if result of array set is used in value of another array set --- .../src/brillig/brillig_ir/entry_point.rs | 2 +- .../noirc_evaluator/src/ssa/opt/array_set.rs | 83 +++++++++++-------- 2 files changed, 51 insertions(+), 34 deletions(-) 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 ff9b5ea67eb..c28e9c66011 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -9,7 +9,7 @@ use super::{ }; use acvm::acir::{brillig::MemoryAddress, AcirField}; -pub(crate) const MAX_STACK_SIZE: usize = 2048; +pub(crate) const MAX_STACK_SIZE: usize = 32768; pub(crate) const MAX_SCRATCH_SPACE: usize = 64; impl BrilligContext { diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 267a2c105f2..795a84c13f4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -70,12 +70,16 @@ fn analyze_last_uses( instructions_that_can_be_made_mutable.remove(&existing); } } - Instruction::ArraySet { array, .. } => { + Instruction::ArraySet { array, value, .. } => { let array = dfg.resolve(*array); + let value = dfg.resolve(*value); if let Some(existing) = array_to_last_use.insert(array, *instruction_id) { instructions_that_can_be_made_mutable.remove(&existing); } + if let Some(existing) = array_to_last_use.insert(value, *instruction_id) { + instructions_that_can_be_made_mutable.remove(&existing); + } // If the array we are setting does not come from a load we can safely mark it mutable. // If the array comes from a load we may potentially being mutating an array at a reference // that is loaded from by other values. @@ -92,6 +96,9 @@ fn analyze_last_uses( if (!arrays_from_load.contains(&array) || is_return_block) && !array_in_terminator { instructions_that_can_be_made_mutable.insert(*instruction_id); } + + let result = dfg.instruction_results(*instruction_id)[0]; + array_to_last_use.insert(result, *instruction_id); } Instruction::Call { arguments, .. } => { for argument in arguments { @@ -169,29 +176,31 @@ mod tests { // from and cloned in a loop. If the array is inadvertently marked mutable, and is cloned in a previous iteration // of the loop, its clone will also be altered. // - // acir(inline) fn main f0 { + // brillig fn main f0 { // b0(): - // v2 = allocate - // store [Field 0, Field 0, Field 0, Field 0, Field 0] at v2 // v3 = allocate - // store [Field 0, Field 0, Field 0, Field 0, Field 0] at v3 + // store [[Field 0, Field 0, Field 0, Field 0, Field 0], [Field 0, Field 0, Field 0, Field 0, Field 0]] at v3 + // v4 = allocate + // store [[Field 0, Field 0, Field 0, Field 0, Field 0], [Field 0, Field 0, Field 0, Field 0, Field 0]] at v4 // jmp b1(u32 0) - // b1(v5: u32): - // v7 = lt v5, u32 5 - // jmpif v7 then: b3, else: b2 + // b1(v6: u32): + // v8 = lt v6, u32 5 + // jmpif v8 then: b3, else: b2 // b3(): - // v8 = eq v5, u32 5 - // jmpif v8 then: b4, else: b5 + // v9 = eq v6, u32 5 + // jmpif v9 then: b4, else: b5 // b4(): - // v9 = load v2 - // store v9 at v3 + // v10 = load v3 + // store v10 at v4 // jmp b5() // b5(): - // v10 = load v2 - // v12 = array_set v10, index v5, value Field 20 - // store v12 at v2 - // v14 = add v5, u32 1 - // jmp b1(v14) + // v11 = load v3 + // v13 = array_get v11, index Field 0 + // v14 = array_set v13, index v6, value Field 20 + // v15 = array_set v11, index v6, value v14 + // store v15 at v3 + // v17 = add v6, u32 1 + // jmp b1(v17) // b2(): // return // } @@ -203,13 +212,16 @@ mod tests { let zero = builder.field_constant(0u128); let array_constant = builder.array_constant(vector![zero, zero, zero, zero, zero], array_type.clone()); + let nested_array_type = Type::Array(Arc::new(vec![array_type.clone()]), 2); + let nested_array_constant = builder + .array_constant(vector![array_constant, array_constant], nested_array_type.clone()); - let v2 = builder.insert_allocate(array_type.clone()); + let v3 = builder.insert_allocate(array_type.clone()); - builder.insert_store(v2, array_constant); + builder.insert_store(v3, nested_array_constant); - let v3 = builder.insert_allocate(array_type.clone()); - builder.insert_store(v3, array_constant); + let v4 = builder.insert_allocate(array_type.clone()); + builder.insert_store(v4, nested_array_constant); let b1 = builder.insert_block(); let zero_u32 = builder.numeric_constant(0u128, Type::unsigned(32)); @@ -219,42 +231,47 @@ mod tests { builder.switch_to_block(b1); let v5 = builder.add_block_parameter(b1, Type::unsigned(32)); let five = builder.numeric_constant(5u128, Type::unsigned(32)); - let v7 = builder.insert_binary(v5, BinaryOp::Lt, five); + let v8 = builder.insert_binary(v5, BinaryOp::Lt, five); let b2 = builder.insert_block(); let b3 = builder.insert_block(); let b4 = builder.insert_block(); let b5 = builder.insert_block(); - builder.terminate_with_jmpif(v7, b3, b2); + builder.terminate_with_jmpif(v8, b3, b2); // Loop body // b3 is the if statement conditional builder.switch_to_block(b3); let two = builder.numeric_constant(5u128, Type::unsigned(32)); - let v8 = builder.insert_binary(v5, BinaryOp::Eq, two); - builder.terminate_with_jmpif(v8, b4, b5); + let v9 = builder.insert_binary(v5, BinaryOp::Eq, two); + builder.terminate_with_jmpif(v9, b4, b5); // b4 is the rest of the loop after the if statement builder.switch_to_block(b4); - let v9 = builder.insert_load(v2, array_type.clone()); - builder.insert_store(v3, v9); + let v10 = builder.insert_load(v3, nested_array_type.clone()); + builder.insert_store(v4, v10); builder.terminate_with_jmp(b5, vec![]); builder.switch_to_block(b5); - let v10 = builder.insert_load(v2, array_type.clone()); + let v11 = builder.insert_load(v3, nested_array_type.clone()); let twenty = builder.field_constant(20u128); - let v12 = builder.insert_array_set(v10, v5, twenty); - builder.insert_store(v2, v12); + let v13 = builder.insert_array_get(v11, zero, array_type.clone()); + let v14 = builder.insert_array_set(v13, v5, twenty); + let v15 = builder.insert_array_set(v11, v5, v14); + + builder.insert_store(v3, v15); let one = builder.numeric_constant(1u128, Type::unsigned(32)); - let v14 = builder.insert_binary(v5, BinaryOp::Add, one); - builder.terminate_with_jmp(b1, vec![v14]); + let v17 = builder.insert_binary(v5, BinaryOp::Add, one); + builder.terminate_with_jmp(b1, vec![v17]); builder.switch_to_block(b2); builder.terminate_with_return(vec![]); let ssa = builder.finish(); + println!("{}", ssa); // We expect the same result as above let ssa = ssa.array_set_optimization(); + println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 6); @@ -265,7 +282,7 @@ mod tests { .filter(|instruction| matches!(&main.dfg[**instruction], Instruction::ArraySet { .. })) .collect::>(); - assert_eq!(array_set_instructions.len(), 1); + assert_eq!(array_set_instructions.len(), 2); if let Instruction::ArraySet { mutable, .. } = &main.dfg[*array_set_instructions[0]] { // The single array set should not be marked mutable assert!(!mutable); From 3827460d5a1379a698c6ce72421672b6ff8c0e07 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 22:47:35 +0000 Subject: [PATCH 2/7] gate nested arrays check to brillig --- .../noirc_evaluator/src/ssa/opt/array_set.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 795a84c13f4..5367335c20f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -2,7 +2,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, - function::Function, + function::{Function, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, types::Type::{Array, Slice}, value::ValueId, @@ -39,6 +39,7 @@ impl Function { analyze_last_uses( &self.dfg, *block, + matches!(self.runtime(), RuntimeType::Brillig), &mut array_to_last_use, &mut instructions_to_update, &mut arrays_from_load, @@ -55,6 +56,7 @@ impl Function { fn analyze_last_uses( dfg: &DataFlowGraph, block_id: BasicBlockId, + is_brillig_func: bool, array_to_last_use: &mut HashMap, instructions_that_can_be_made_mutable: &mut HashSet, arrays_from_load: &mut HashSet, @@ -72,14 +74,20 @@ fn analyze_last_uses( } Instruction::ArraySet { array, value, .. } => { let array = dfg.resolve(*array); - let value = dfg.resolve(*value); if let Some(existing) = array_to_last_use.insert(array, *instruction_id) { instructions_that_can_be_made_mutable.remove(&existing); } - if let Some(existing) = array_to_last_use.insert(value, *instruction_id) { - instructions_that_can_be_made_mutable.remove(&existing); + if is_brillig_func { + let value = dfg.resolve(*value); + + if let Some(existing) = array_to_last_use.insert(value, *instruction_id) { + instructions_that_can_be_made_mutable.remove(&existing); + } + let result = dfg.instruction_results(*instruction_id)[0]; + array_to_last_use.insert(result, *instruction_id); } + // If the array we are setting does not come from a load we can safely mark it mutable. // If the array comes from a load we may potentially being mutating an array at a reference // that is loaded from by other values. @@ -96,9 +104,6 @@ fn analyze_last_uses( if (!arrays_from_load.contains(&array) || is_return_block) && !array_in_terminator { instructions_that_can_be_made_mutable.insert(*instruction_id); } - - let result = dfg.instruction_results(*instruction_id)[0]; - array_to_last_use.insert(result, *instruction_id); } Instruction::Call { arguments, .. } => { for argument in arguments { From ef80140b2643397db6dddaf2b5ca6bd8204c8b41 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 23:22:23 +0000 Subject: [PATCH 3/7] reduce blow-up for non nestested arrays --- .../noirc_evaluator/src/ssa/opt/array_set.rs | 63 ++++++++++++++++++- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 5367335c20f..12a5f743a94 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -1,9 +1,12 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, + cfg::ControlFlowGraph, dfg::DataFlowGraph, + dom::DominatorTree, function::{Function, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, + post_order::PostOrder, types::Type::{Array, Slice}, value::ValueId, }, @@ -34,6 +37,7 @@ impl Function { let mut array_to_last_use = HashMap::default(); let mut instructions_to_update = HashSet::default(); let mut arrays_from_load = HashSet::default(); + let mut inner_nested_arrays = HashMap::default(); for block in reachable_blocks.iter() { analyze_last_uses( @@ -43,6 +47,7 @@ impl Function { &mut array_to_last_use, &mut instructions_to_update, &mut arrays_from_load, + &mut inner_nested_arrays, ); } for block in reachable_blocks { @@ -60,6 +65,7 @@ fn analyze_last_uses( array_to_last_use: &mut HashMap, instructions_that_can_be_made_mutable: &mut HashSet, arrays_from_load: &mut HashSet, + inner_nested_arrays: &mut HashMap, ) { let block = &dfg[block_id]; @@ -81,11 +87,11 @@ fn analyze_last_uses( if is_brillig_func { let value = dfg.resolve(*value); - if let Some(existing) = array_to_last_use.insert(value, *instruction_id) { - instructions_that_can_be_made_mutable.remove(&existing); + if let Some(existing) = inner_nested_arrays.get(&value) { + instructions_that_can_be_made_mutable.remove(existing); } let result = dfg.instruction_results(*instruction_id)[0]; - array_to_last_use.insert(result, *instruction_id); + inner_nested_arrays.insert(result, *instruction_id); } // If the array we are setting does not come from a load we can safely mark it mutable. @@ -159,6 +165,57 @@ fn make_mutable( *dfg[block_id].instructions_mut() = instructions; } +/// For a given function, finds all the blocks that are within loops +fn find_all_blocks_within_loops( + func: &Function, + cfg: &ControlFlowGraph, + dominator_tree: &mut DominatorTree, +) -> HashSet { + let mut blocks_in_loops = HashSet::default(); + for block_id in func.reachable_blocks() { + let block = &func.dfg[block_id]; + let successors = block.successors(); + for successor_id in successors { + if dominator_tree.dominates(successor_id, block_id) { + blocks_in_loops.extend(find_blocks_in_loop(successor_id, block_id, cfg)); + } + } + } + + blocks_in_loops +} + +/// Return each block that is in a loop starting in the given header block. +/// Expects back_edge_start -> header to be the back edge of the loop. +fn find_blocks_in_loop( + header: BasicBlockId, + back_edge_start: BasicBlockId, + cfg: &ControlFlowGraph, +) -> HashSet { + let mut blocks = HashSet::default(); + blocks.insert(header); + + let mut insert = |block, stack: &mut Vec| { + if !blocks.contains(&block) { + blocks.insert(block); + stack.push(block); + } + }; + + // Starting from the back edge of the loop, each predecessor of this block until + // the header is within the loop. + let mut stack = vec![]; + insert(back_edge_start, &mut stack); + + while let Some(block) = stack.pop() { + for predecessor in cfg.predecessors(block) { + insert(predecessor, &mut stack); + } + } + + blocks +} + #[cfg(test)] mod tests { use std::sync::Arc; From 7cb4d55ca636a59eccfc202937bf6d030e9e33cf Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 23:24:03 +0000 Subject: [PATCH 4/7] cleanup --- .../noirc_evaluator/src/ssa/opt/array_set.rs | 54 ------------------- 1 file changed, 54 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 12a5f743a94..ff1d29591c3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -1,12 +1,9 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, - cfg::ControlFlowGraph, dfg::DataFlowGraph, - dom::DominatorTree, function::{Function, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, - post_order::PostOrder, types::Type::{Array, Slice}, value::ValueId, }, @@ -165,57 +162,6 @@ fn make_mutable( *dfg[block_id].instructions_mut() = instructions; } -/// For a given function, finds all the blocks that are within loops -fn find_all_blocks_within_loops( - func: &Function, - cfg: &ControlFlowGraph, - dominator_tree: &mut DominatorTree, -) -> HashSet { - let mut blocks_in_loops = HashSet::default(); - for block_id in func.reachable_blocks() { - let block = &func.dfg[block_id]; - let successors = block.successors(); - for successor_id in successors { - if dominator_tree.dominates(successor_id, block_id) { - blocks_in_loops.extend(find_blocks_in_loop(successor_id, block_id, cfg)); - } - } - } - - blocks_in_loops -} - -/// Return each block that is in a loop starting in the given header block. -/// Expects back_edge_start -> header to be the back edge of the loop. -fn find_blocks_in_loop( - header: BasicBlockId, - back_edge_start: BasicBlockId, - cfg: &ControlFlowGraph, -) -> HashSet { - let mut blocks = HashSet::default(); - blocks.insert(header); - - let mut insert = |block, stack: &mut Vec| { - if !blocks.contains(&block) { - blocks.insert(block); - stack.push(block); - } - }; - - // Starting from the back edge of the loop, each predecessor of this block until - // the header is within the loop. - let mut stack = vec![]; - insert(back_edge_start, &mut stack); - - while let Some(block) = stack.pop() { - for predecessor in cfg.predecessors(block) { - insert(predecessor, &mut stack); - } - } - - blocks -} - #[cfg(test)] mod tests { use std::sync::Arc; From 2ae9d43f9eeba2533ceebada296b822012a690e7 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 19:47:38 -0400 Subject: [PATCH 5/7] Update compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs --- compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c28e9c66011..ff9b5ea67eb 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -9,7 +9,7 @@ use super::{ }; use acvm::acir::{brillig::MemoryAddress, AcirField}; -pub(crate) const MAX_STACK_SIZE: usize = 32768; +pub(crate) const MAX_STACK_SIZE: usize = 2048; pub(crate) const MAX_SCRATCH_SPACE: usize = 64; impl BrilligContext { From 64b51561dadc12353327dea06a5743ed25a0b223 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 19:47:55 -0400 Subject: [PATCH 6/7] Update compiler/noirc_evaluator/src/ssa/opt/array_set.rs --- compiler/noirc_evaluator/src/ssa/opt/array_set.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index ff1d29591c3..d327d8aac41 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -276,7 +276,6 @@ mod tests { builder.terminate_with_return(vec![]); let ssa = builder.finish(); - println!("{}", ssa); // We expect the same result as above let ssa = ssa.array_set_optimization(); println!("{}", ssa); From 25060d3024beda4c2d7167f6293b2e2e17c5a234 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 1 Oct 2024 19:48:07 -0400 Subject: [PATCH 7/7] Update compiler/noirc_evaluator/src/ssa/opt/array_set.rs --- compiler/noirc_evaluator/src/ssa/opt/array_set.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index d327d8aac41..e7d765949b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -278,7 +278,6 @@ mod tests { let ssa = builder.finish(); // We expect the same result as above let ssa = ssa.array_set_optimization(); - println!("{}", ssa); let main = ssa.main(); assert_eq!(main.reachable_blocks().len(), 6);