From 3521ec59e0d34419033324ae622aed7dcd5f0b59 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 28 Jan 2025 22:48:30 +0000 Subject: [PATCH 01/16] initial work to hoist shared constants across function to the global brillig space. Done on the Brillig side --- .../src/brillig/brillig_gen.rs | 6 +- .../src/brillig/brillig_gen/brillig_block.rs | 55 ++++++++++++++++++- .../brillig_gen/brillig_block_variables.rs | 2 +- .../brillig/brillig_gen/brillig_globals.rs | 18 ++++-- .../brillig/brillig_gen/brillig_slice_ops.rs | 39 ++++++++++--- .../brillig_gen/constant_allocation.rs | 2 +- compiler/noirc_evaluator/src/brillig/mod.rs | 34 ++++++++++-- .../src/ssa/opt/constant_folding.rs | 1 + .../noirc_evaluator/src/ssa/opt/unrolling.rs | 10 +++- .../global_var_regression_simple/src/main.nr | 5 +- 10 files changed, 144 insertions(+), 28 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index a6117a8f2da..16e40a2e942 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -4,7 +4,7 @@ pub(crate) mod brillig_block_variables; pub(crate) mod brillig_fn; pub(crate) mod brillig_globals; pub(crate) mod brillig_slice_ops; -mod constant_allocation; +pub(crate) mod constant_allocation; mod variable_liveness; use acvm::FieldElement; @@ -20,7 +20,7 @@ use super::{ }; use crate::{ errors::InternalError, - ssa::ir::{call_stack::CallStack, function::Function}, + ssa::ir::{call_stack::CallStack, function::Function, types::NumericType}, }; /// Converting an SSA function into Brillig bytecode. @@ -28,6 +28,7 @@ pub(crate) fn convert_ssa_function( func: &Function, enable_debug_trace: bool, globals: &HashMap, + hoisted_global_constants: &HashMap<(FieldElement, NumericType), BrilligVariable>, ) -> BrilligArtifact { let mut brillig_context = BrilligContext::new(enable_debug_trace); @@ -44,6 +45,7 @@ pub(crate) fn convert_ssa_function( block, &func.dfg, globals, + hoisted_global_constants, ); } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 97de1aea8c7..b546b01ae73 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -27,7 +27,7 @@ use num_bigint::BigUint; use std::sync::Arc; use super::brillig_black_box::convert_black_box_call; -use super::brillig_block_variables::BlockVariables; +use super::brillig_block_variables::{allocate_value_with_type, BlockVariables}; use super::brillig_fn::FunctionContext; use super::constant_allocation::InstructionLocation; @@ -44,6 +44,8 @@ pub(crate) struct BrilligBlock<'block, Registers: RegisterAllocator> { pub(crate) last_uses: HashMap>, pub(crate) globals: &'block HashMap, + pub(crate) hoisted_global_constants: + &'block HashMap<(FieldElement, NumericType), BrilligVariable>, pub(crate) building_globals: bool, } @@ -56,11 +58,18 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { block_id: BasicBlockId, dfg: &DataFlowGraph, globals: &'block HashMap, + hoisted_global_constants: &'block HashMap<(FieldElement, NumericType), BrilligVariable>, ) { let live_in = function_context.liveness.get_live_in(&block_id); let mut live_in_no_globals = HashSet::default(); for value in live_in { + // TODO: Move this check into a helper function + if let Value::NumericConstant { constant, typ } = dfg[*value] { + if hoisted_global_constants.get(&(constant, typ)).is_some() { + continue; + } + } if !dfg.is_global(*value) { live_in_no_globals.insert(*value); } @@ -84,6 +93,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { variables, last_uses, globals, + hoisted_global_constants, building_globals: false, }; @@ -94,7 +104,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { &mut self, globals: &DataFlowGraph, used_globals: &HashSet, - ) { + hoisted_global_consts: &HashSet<(FieldElement, NumericType)>, + ) -> HashMap<(FieldElement, NumericType), BrilligVariable> { for (id, value) in globals.values_iter() { if !used_globals.contains(&id) { continue; @@ -113,6 +124,18 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } } + + let mut hoisted_global_constants = HashMap::default(); + for (constant, typ) in hoisted_global_consts { + let new_variable = allocate_value_with_type(self.brillig_context, Type::Numeric(*typ)); + + self.brillig_context.const_instruction(new_variable.extract_single_addr(), *constant); + + if hoisted_global_constants.insert((*constant, *typ), new_variable).is_some() { + unreachable!("ICE: ({constant:?}, {typ:?}) was already in cache"); + } + } + hoisted_global_constants } fn convert_block(&mut self, dfg: &DataFlowGraph) { @@ -903,6 +926,11 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .expect("Last uses for instruction should have been computed"); for dead_variable in dead_variables { + if let Value::NumericConstant { constant, typ } = &dfg[*dead_variable] { + if self.hoisted_global_constants.get(&(*constant, *typ)).is_some() { + continue; + } + } // Globals are reserved throughout the entirety of the program if !dfg.is_global(*dead_variable) { self.variables.remove_variable( @@ -1609,6 +1637,14 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { fn initialize_constants(&mut self, constants: &[ValueId], dfg: &DataFlowGraph) { for &constant_id in constants { + // if let Value::NumericConstant { constant, typ } = dfg[constant_id] { + // if let Some(variable) = self.hoisted_global_constants.get(&(constant, typ)) { + // if self.function_context.ssa_value_allocations.insert(constant_id, *variable).is_some() { + // unreachable!("ICE: ValueId {constant:?} was already in cache"); + // } + // self.variables.available_variables.insert(constant_id); + // } + // } self.convert_ssa_value(constant_id, dfg); } } @@ -1622,6 +1658,21 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let value_id = dfg.resolve(value_id); let value = &dfg[value_id]; + if !self.building_globals { + if let Value::NumericConstant { constant, typ } = value { + if let Some(variable) = self.hoisted_global_constants.get(&(*constant, *typ)) { + // if self.function_context.ssa_value_allocations.insert(value_id, *variable).is_some() { + // unreachable!("ICE: ValueId {constant:?} was already in cache"); + // } + // self.variables.available_variables.insert(value_id); + return *variable; + } + // else { + // panic!("ICE: should have variable already allocated"); + // } + } + } + match value { Value::Global(_) => { unreachable!("Expected global value to be resolve to its inner value"); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 4cf8e921483..10761caea9d 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -21,7 +21,7 @@ use super::brillig_fn::FunctionContext; #[derive(Debug, Default)] pub(crate) struct BlockVariables { - available_variables: HashSet, + pub(crate) available_variables: HashSet, } impl BlockVariables { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 9f9d271283d..319222d6dd2 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -4,14 +4,22 @@ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use super::{BrilligArtifact, BrilligBlock, BrilligVariable, FunctionContext, Label, ValueId}; use crate::{ brillig::{brillig_ir::BrilligContext, DataFlowGraph}, - ssa::ir::dfg::GlobalsGraph, + ssa::ir::{dfg::GlobalsGraph, types::NumericType}, }; +pub(crate) type BrilligGlobalsArtifact = ( + BrilligArtifact, + HashMap, + usize, + HashMap<(FieldElement, NumericType), BrilligVariable>, +); + pub(crate) fn convert_ssa_globals( enable_debug_trace: bool, globals: GlobalsGraph, used_globals: &HashSet, -) -> (BrilligArtifact, HashMap, usize) { + hoisted_global_consts: &HashSet<(FieldElement, NumericType)>, +) -> BrilligGlobalsArtifact { let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace); // The global space does not have globals itself let empty_globals = HashMap::default(); @@ -28,16 +36,18 @@ pub(crate) fn convert_ssa_globals( variables: Default::default(), last_uses: HashMap::default(), globals: &empty_globals, + hoisted_global_constants: &HashMap::default(), building_globals: true, }; let globals_dfg = DataFlowGraph::from(globals); - brillig_block.compile_globals(&globals_dfg, used_globals); + let hoisted_global_constants = + brillig_block.compile_globals(&globals_dfg, used_globals, hoisted_global_consts); let globals_size = brillig_block.brillig_context.global_space_size(); brillig_context.return_instruction(); let artifact = brillig_context.artifact(); - (artifact, function_context.ssa_value_allocations, globals_size) + (artifact, function_context.ssa_value_allocations, globals_size, hoisted_global_constants) } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index 1ec2d165b12..99645f84ed3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -179,6 +179,7 @@ mod tests { use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::function::RuntimeType; use crate::ssa::ir::map::Id; + use crate::ssa::ir::types::NumericType; use crate::ssa::ssa_gen::Ssa; fn create_test_environment() -> (Ssa, FunctionContext, BrilligContext) { @@ -197,6 +198,7 @@ mod tests { function_context: &'a mut FunctionContext, brillig_context: &'a mut BrilligContext, globals: &'a HashMap, + hoisted_global_constants: &'a HashMap<(FieldElement, NumericType), BrilligVariable>, ) -> BrilligBlock<'a, Stack> { let variables = BlockVariables::default(); BrilligBlock { @@ -206,6 +208,7 @@ mod tests { variables, last_uses: Default::default(), globals, + hoisted_global_constants, building_globals: false, } } @@ -249,8 +252,13 @@ mod tests { let target_vector = BrilligVector { pointer: context.allocate_register() }; let brillig_globals = HashMap::default(); - let mut block = - create_brillig_block(&mut function_context, &mut context, &brillig_globals); + let hoisted_globals = HashMap::default(); + let mut block = create_brillig_block( + &mut function_context, + &mut context, + &brillig_globals, + &hoisted_globals, + ); if push_back { block.slice_push_back_operation( @@ -367,8 +375,13 @@ mod tests { }; let brillig_globals = HashMap::default(); - let mut block = - create_brillig_block(&mut function_context, &mut context, &brillig_globals); + let hoisted_globals = HashMap::default(); + let mut block = create_brillig_block( + &mut function_context, + &mut context, + &brillig_globals, + &hoisted_globals, + ); if pop_back { block.slice_pop_back_operation( @@ -475,8 +488,13 @@ mod tests { let target_vector = BrilligVector { pointer: context.allocate_register() }; let brillig_globals = HashMap::default(); - let mut block = - create_brillig_block(&mut function_context, &mut context, &brillig_globals); + let hoisted_globals = HashMap::default(); + let mut block = create_brillig_block( + &mut function_context, + &mut context, + &brillig_globals, + &hoisted_globals, + ); block.slice_insert_operation( target_vector, @@ -617,8 +635,13 @@ mod tests { }; let brillig_globals = HashMap::default(); - let mut block = - create_brillig_block(&mut function_context, &mut context, &brillig_globals); + let hoisted_globals = HashMap::default(); + let mut block = create_brillig_block( + &mut function_context, + &mut context, + &brillig_globals, + &hoisted_globals, + ); block.slice_remove_operation( target_vector, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index 64741393dd7..3cddc61b7ff 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -24,7 +24,7 @@ pub(crate) enum InstructionLocation { #[derive(Default)] pub(crate) struct ConstantAllocation { - constant_usage: HashMap>>, + pub(crate) constant_usage: HashMap>>, allocation_points: HashMap>>, dominator_tree: DominatorTree, blocks_within_loops: HashSet, diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index b74c519f61a..c4f7a91b22b 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -2,7 +2,7 @@ pub(crate) mod brillig_gen; pub(crate) mod brillig_ir; use acvm::FieldElement; -use brillig_gen::brillig_globals::convert_ssa_globals; +use brillig_gen::{brillig_globals::convert_ssa_globals, constant_allocation::ConstantAllocation}; use brillig_ir::{artifact::LabelType, brillig_variable::BrilligVariable, registers::GlobalSpace}; use self::{ @@ -16,11 +16,12 @@ use crate::ssa::{ ir::{ dfg::DataFlowGraph, function::{Function, FunctionId}, + types::NumericType, value::ValueId, }, ssa_gen::Ssa, }; -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use std::{borrow::Cow, collections::BTreeSet}; pub use self::brillig_ir::procedures::ProcedureId; @@ -42,8 +43,9 @@ impl Brillig { func: &Function, enable_debug_trace: bool, globals: &HashMap, + hoisted_global_constants: &HashMap<(FieldElement, NumericType), BrilligVariable>, ) { - let obj = convert_ssa_function(func, enable_debug_trace, globals); + let obj = convert_ssa_function(func, enable_debug_trace, globals, hoisted_global_constants); self.ssa_function_to_brillig.insert(func.id(), obj); } @@ -89,17 +91,37 @@ impl Ssa { return brillig; } + // We can potentially have multiple local constants with the same value and type + let mut hoisted_global_consts: HashSet<(FieldElement, NumericType)> = HashSet::default(); + for brillig_function_id in brillig_reachable_function_ids.iter() { + let function = &self.functions[brillig_function_id]; + let constants = ConstantAllocation::from_function(function); + for (constant, _) in constants.constant_usage { + let value = function.dfg.get_numeric_constant(constant); + let value = value.unwrap(); + let typ = function.dfg.type_of_value(constant); + if !function.dfg.is_global(constant) { + hoisted_global_consts.insert((value, typ.unwrap_numeric())); + } + } + } + // Globals are computed once at compile time and shared across all functions, // thus we can just fetch globals from the main function. let globals = (*self.functions[&self.main_id].dfg.globals).clone(); - let (artifact, brillig_globals, globals_size) = - convert_ssa_globals(enable_debug_trace, globals, &self.used_global_values); + let (artifact, brillig_globals, globals_size, hoisted_global_constants) = + convert_ssa_globals( + enable_debug_trace, + globals, + &self.used_global_values, + &hoisted_global_consts, + ); brillig.globals = artifact; brillig.globals_memory_size = globals_size; for brillig_function_id in brillig_reachable_function_ids { let func = &self.functions[&brillig_function_id]; - brillig.compile(func, enable_debug_trace, &brillig_globals); + brillig.compile(func, enable_debug_trace, &brillig_globals, &hoisted_global_constants); } brillig diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index e8cae7da5b5..876ec9d5959 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -60,6 +60,7 @@ impl Ssa { /// See [`constant_folding`][self] module for more information. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants(mut self) -> Ssa { + // let mut constants = HashSet::default(); for function in self.functions.values_mut() { function.constant_fold(false, None); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index eb0bbd8c532..8f230d093af 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -82,8 +82,12 @@ impl Ssa { let globals = (*function.dfg.globals).clone(); // DIE is run at the end of our SSA optimizations, so we mark all globals as in use here. let used_globals = &globals.values_iter().map(|(id, _)| id).collect(); - let (_, brillig_globals, _) = - convert_ssa_globals(false, globals, used_globals); + let (_, brillig_globals, _, _) = convert_ssa_globals( + false, + globals, + used_globals, + &fxhash::FxHashSet::default(), + ); global_cache = Some(brillig_globals); } let brillig_globals = global_cache.as_ref().unwrap(); @@ -1037,7 +1041,7 @@ fn brillig_bytecode_size( // This is to try to prevent hitting ICE. temp.dead_instruction_elimination(false, true); - convert_ssa_function(&temp, false, globals).byte_code.len() + convert_ssa_function(&temp, false, globals, &HashMap::default()).byte_code.len() } /// Decide if the new bytecode size is acceptable, compared to the original. diff --git a/test_programs/execution_success/global_var_regression_simple/src/main.nr b/test_programs/execution_success/global_var_regression_simple/src/main.nr index b1bf753a73c..436aeb2652c 100644 --- a/test_programs/execution_success/global_var_regression_simple/src/main.nr +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -14,6 +14,10 @@ fn main(x: Field, y: pub Field) { } fn dummy_again(x: Field, y: Field) { + // Move this before as to change the value ids of the common constants + // between this function and main + assert(x != y); + let mut acc: Field = 0; for i in 0..2 { for j in 0..2 { @@ -21,5 +25,4 @@ fn dummy_again(x: Field, y: Field) { } } assert(!acc.lt(x)); - assert(x != y); } From 3c2e0e9e13a435492c660fe942e964071423a604 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 28 Jan 2025 22:57:25 +0000 Subject: [PATCH 02/16] cleanup --- .../src/brillig/brillig_gen/brillig_block.rs | 15 --------------- .../src/ssa/opt/constant_folding.rs | 1 - 2 files changed, 16 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index b546b01ae73..933a917b48b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1637,14 +1637,6 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { fn initialize_constants(&mut self, constants: &[ValueId], dfg: &DataFlowGraph) { for &constant_id in constants { - // if let Value::NumericConstant { constant, typ } = dfg[constant_id] { - // if let Some(variable) = self.hoisted_global_constants.get(&(constant, typ)) { - // if self.function_context.ssa_value_allocations.insert(constant_id, *variable).is_some() { - // unreachable!("ICE: ValueId {constant:?} was already in cache"); - // } - // self.variables.available_variables.insert(constant_id); - // } - // } self.convert_ssa_value(constant_id, dfg); } } @@ -1661,15 +1653,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { if !self.building_globals { if let Value::NumericConstant { constant, typ } = value { if let Some(variable) = self.hoisted_global_constants.get(&(*constant, *typ)) { - // if self.function_context.ssa_value_allocations.insert(value_id, *variable).is_some() { - // unreachable!("ICE: ValueId {constant:?} was already in cache"); - // } - // self.variables.available_variables.insert(value_id); return *variable; } - // else { - // panic!("ICE: should have variable already allocated"); - // } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 876ec9d5959..e8cae7da5b5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -60,7 +60,6 @@ impl Ssa { /// See [`constant_folding`][self] module for more information. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants(mut self) -> Ssa { - // let mut constants = HashSet::default(); for function in self.functions.values_mut() { function.constant_fold(false, None); } From da763fabcaca5b0c074c0547f9500b9d3f24f72d Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 28 Jan 2025 23:11:47 +0000 Subject: [PATCH 03/16] selectively hoist based off num occurrences --- compiler/noirc_evaluator/src/brillig/mod.rs | 22 +++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index c4f7a91b22b..584032e02f2 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -92,7 +92,8 @@ impl Ssa { } // We can potentially have multiple local constants with the same value and type - let mut hoisted_global_consts: HashSet<(FieldElement, NumericType)> = HashSet::default(); + let mut hoisted_global_consts: HashMap<(FieldElement, NumericType), usize> = + HashMap::default(); for brillig_function_id in brillig_reachable_function_ids.iter() { let function = &self.functions[brillig_function_id]; let constants = ConstantAllocation::from_function(function); @@ -101,11 +102,28 @@ impl Ssa { let value = value.unwrap(); let typ = function.dfg.type_of_value(constant); if !function.dfg.is_global(constant) { - hoisted_global_consts.insert((value, typ.unwrap_numeric())); + hoisted_global_consts + .entry((value, typ.unwrap_numeric())) + .and_modify(|counter| *counter += 1) + .or_insert(1); } } } + // We want to hoist only if there are repeat occurrences of a constant. + let hoisted_global_consts = hoisted_global_consts + .into_iter() + .filter_map( + |(value, num_occurrences)| { + if num_occurrences > 1 { + Some(value) + } else { + None + } + }, + ) + .collect::>(); + // Globals are computed once at compile time and shared across all functions, // thus we can just fetch globals from the main function. let globals = (*self.functions[&self.main_id].dfg.globals).clone(); From c2c1370d75af3c249c3a52289701f595ae64c5de Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 29 Jan 2025 14:59:47 +0000 Subject: [PATCH 04/16] use constants not consts --- compiler/noirc_evaluator/src/brillig/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 584032e02f2..e7a4cd4a148 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -92,7 +92,7 @@ impl Ssa { } // We can potentially have multiple local constants with the same value and type - let mut hoisted_global_consts: HashMap<(FieldElement, NumericType), usize> = + let mut hoisted_global_constants: HashMap<(FieldElement, NumericType), usize> = HashMap::default(); for brillig_function_id in brillig_reachable_function_ids.iter() { let function = &self.functions[brillig_function_id]; @@ -102,7 +102,7 @@ impl Ssa { let value = value.unwrap(); let typ = function.dfg.type_of_value(constant); if !function.dfg.is_global(constant) { - hoisted_global_consts + hoisted_global_constants .entry((value, typ.unwrap_numeric())) .and_modify(|counter| *counter += 1) .or_insert(1); @@ -111,7 +111,7 @@ impl Ssa { } // We want to hoist only if there are repeat occurrences of a constant. - let hoisted_global_consts = hoisted_global_consts + let hoisted_global_constants = hoisted_global_constants .into_iter() .filter_map( |(value, num_occurrences)| { @@ -132,7 +132,7 @@ impl Ssa { enable_debug_trace, globals, &self.used_global_values, - &hoisted_global_consts, + &hoisted_global_constants, ); brillig.globals = artifact; brillig.globals_memory_size = globals_size; From a6d631cd3ebaa4ac7612879c4e82f0e1a443b84c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 29 Jan 2025 20:52:51 +0000 Subject: [PATCH 05/16] updated hoisting to work per entry point --- .../src/brillig/brillig_gen/brillig_block.rs | 20 ++- .../brillig/brillig_gen/brillig_globals.rs | 140 ++++++++++++++++-- compiler/noirc_evaluator/src/brillig/mod.rs | 48 ++---- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- 4 files changed, 150 insertions(+), 60 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 933a917b48b..9c1e06faca3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -60,7 +60,10 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { globals: &'block HashMap, hoisted_global_constants: &'block HashMap<(FieldElement, NumericType), BrilligVariable>, ) { - let live_in = function_context.liveness.get_live_in(&block_id); + let live_in: &std::collections::HashSet< + crate::ssa::ir::map::Id, + std::hash::BuildHasherDefault, + > = function_context.liveness.get_live_in(&block_id); let mut live_in_no_globals = HashSet::default(); for value in live_in { @@ -106,6 +109,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { used_globals: &HashSet, hoisted_global_consts: &HashSet<(FieldElement, NumericType)>, ) -> HashMap<(FieldElement, NumericType), BrilligVariable> { + let mut new_hoisted_constants = HashMap::default(); + for (id, value) in globals.values_iter() { if !used_globals.contains(&id) { continue; @@ -125,17 +130,16 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } - let mut hoisted_global_constants = HashMap::default(); for (constant, typ) in hoisted_global_consts { - let new_variable = allocate_value_with_type(self.brillig_context, Type::Numeric(*typ)); - - self.brillig_context.const_instruction(new_variable.extract_single_addr(), *constant); - - if hoisted_global_constants.insert((*constant, *typ), new_variable).is_some() { + let constant = *constant; + let typ = *typ; + let new_variable = allocate_value_with_type(self.brillig_context, Type::Numeric(typ)); + self.brillig_context.const_instruction(new_variable.extract_single_addr(), constant); + if new_hoisted_constants.insert((constant, typ), new_variable).is_some() { unreachable!("ICE: ({constant:?}, {typ:?}) was already in cache"); } } - hoisted_global_constants + new_hoisted_constants } fn convert_block(&mut self, dfg: &DataFlowGraph) { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 24cce7fbb47..6c019849887 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -3,14 +3,13 @@ use std::collections::BTreeMap; use acvm::FieldElement; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; -use super::{BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId}; -use crate::{ - brillig::{brillig_ir::BrilligContext, DataFlowGraph}, - ssa::ir::{dfg::GlobalsGraph, types::NumericType}, +use super::{ + BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId, }; -use crate::brillig::{ - called_functions_vec, Brillig, FunctionId, - Instruction, Value, +use crate::brillig::{called_functions_vec, Brillig, FunctionId, Instruction, Value}; +use crate::{ + brillig::{brillig_ir::BrilligContext, ConstantAllocation, DataFlowGraph}, + ssa::ir::types::NumericType, }; /// Context structure for generating Brillig globals @@ -28,17 +27,27 @@ pub(crate) struct BrilligGlobals { /// any Brillig function with the appropriate global allocations. brillig_entry_points: HashMap>, + /// Maps a Brillig entry point to constants shared across the entry point and its nested calls. + final_hoisted_global_constants: HashMap>, + /// Maps an inner call to its Brillig entry point /// This is simply used to simplify fetching global allocations when compiling /// individual Brillig functions. inner_call_to_entry_point: HashMap>, /// Final map that associated an entry point with its Brillig global allocations entry_point_globals_map: HashMap, + // TODO: combine this map with the one above + entry_point_hoisted_globals_map: HashMap, } /// Mapping of SSA value ids to their Brillig allocations pub(crate) type SsaToBrilligGlobals = HashMap; +pub(crate) type HoistedConstantsToBrilligGlobals = + HashMap<(FieldElement, NumericType), BrilligVariable>; +/// Mapping of a constant value and the number of functions in which it occurs +pub(crate) type ConstantCounterMap = HashMap<(FieldElement, NumericType), usize>; + impl BrilligGlobals { pub(crate) fn new( functions: &BTreeMap, @@ -46,6 +55,9 @@ impl BrilligGlobals { main_id: FunctionId, ) -> Self { let mut brillig_entry_points = HashMap::default(); + let mut hoisted_global_constants: HashMap = + HashMap::default(); + let acir_functions = functions.iter().filter(|(_, func)| func.runtime().is_acir()); for (_, function) in acir_functions { for block_id in function.reachable_blocks() { @@ -74,7 +86,23 @@ impl BrilligGlobals { &mut used_globals, &mut brillig_entry_points, im::HashSet::new(), + &mut hoisted_global_constants, ); + let function = &functions[func_id]; + let constants = ConstantAllocation::from_function(function); + for (constant, _) in constants.constant_usage { + let value = function.dfg.get_numeric_constant(constant); + let value = value.unwrap(); + let typ = function.dfg.type_of_value(constant); + if !function.dfg.is_global(constant) { + hoisted_global_constants + .entry(*func_id) + .or_default() + .entry((value, typ.unwrap_numeric())) + .and_modify(|counter| *counter += 1) + .or_insert(1); + } + } } } } @@ -91,10 +119,39 @@ impl BrilligGlobals { &mut used_globals, &mut brillig_entry_points, im::HashSet::new(), + &mut hoisted_global_constants, ); } - Self { used_globals, brillig_entry_points, ..Default::default() } + let final_hoisted_global_constants: HashMap< + FunctionId, + HashSet<(FieldElement, NumericType)>, + > = hoisted_global_constants + .iter() + .map(|(entry_point, constants_for_entry_point)| { + let hoisted_global_constants: HashSet<_> = constants_for_entry_point + .iter() + .filter_map( + |(&value, &num_occurrences)| { + if num_occurrences > 1 { + Some(value) + } else { + None + } + }, + ) + .collect(); + + (*entry_point, hoisted_global_constants) + }) + .collect(); + + Self { + used_globals, + brillig_entry_points, + final_hoisted_global_constants, + ..Default::default() + } } /// Recursively mark any functions called in an entry point as well as @@ -108,11 +165,28 @@ impl BrilligGlobals { used_globals: &mut HashMap>, brillig_entry_points: &mut HashMap>, mut explored_functions: im::HashSet, + hoisted_global_constants: &mut HashMap, ) { if explored_functions.insert(called_function.id()).is_some() { return; } + // We can potentially have multiple local constants with the same value and type + let constants = ConstantAllocation::from_function(called_function); + for (constant, _) in constants.constant_usage { + let value = called_function.dfg.get_numeric_constant(constant); + let value = value.unwrap(); + let typ = called_function.dfg.type_of_value(constant); + if !called_function.dfg.is_global(constant) { + hoisted_global_constants + .entry(entry_point) + .or_default() + .entry((value, typ.unwrap_numeric())) + .and_modify(|counter| *counter += 1) + .or_insert(1); + } + } + let inner_calls = called_functions_vec(called_function).into_iter().collect::>(); for inner_call in inner_calls { @@ -136,6 +210,7 @@ impl BrilligGlobals { used_globals, brillig_entry_points, explored_functions.clone(), + hoisted_global_constants, ); } } @@ -149,20 +224,46 @@ impl BrilligGlobals { // Map for fetching the correct entry point globals when compiling any function let mut inner_call_to_entry_point: HashMap> = HashMap::default(); - let mut entry_point_globals_map = HashMap::default(); // We only need to generate globals for entry points for (entry_point, entry_point_inner_calls) in self.brillig_entry_points.iter() { let entry_point = *entry_point; + // dbg!(entry_point_inner_calls.clone()); for inner_call in entry_point_inner_calls { inner_call_to_entry_point.entry(*inner_call).or_default().push(entry_point); } + } + + let mut entry_point_globals_map = HashMap::default(); + let mut entry_point_hoisted_globals_map = HashMap::default(); + + let mut all_hoisted_allocations = HashMap::default(); + // We only need to generate globals for entry points + for (entry_point, _entry_point_inner_calls) in self.brillig_entry_points.iter() { + let entry_point = *entry_point; let used_globals = self.used_globals.remove(&entry_point).unwrap_or_default(); + // let hoisted_global_constants = if exempt_hoisting.contains(&entry_point) { + // // HashSet::default() + // self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default() + // } else { + // self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default() + // }; + let hoisted_global_constants = + self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default(); let (artifact, brillig_globals, globals_size, hoisted_global_constants) = - convert_ssa_globals(enable_debug_trace, globals_dfg, &used_globals, &HashSet::default(), entry_point); + convert_ssa_globals( + enable_debug_trace, + globals_dfg, + &used_globals, + &hoisted_global_constants, + entry_point, + ); + + all_hoisted_allocations.extend(hoisted_global_constants.clone()); entry_point_globals_map.insert(entry_point, brillig_globals); + entry_point_hoisted_globals_map.insert(entry_point, hoisted_global_constants); brillig.globals.insert(entry_point, artifact); brillig.globals_memory_size.insert(entry_point, globals_size); @@ -170,6 +271,7 @@ impl BrilligGlobals { self.inner_call_to_entry_point = inner_call_to_entry_point; self.entry_point_globals_map = entry_point_globals_map; + self.entry_point_hoisted_globals_map = entry_point_hoisted_globals_map; } /// Fetch the global allocations that can possibly be accessed @@ -183,10 +285,11 @@ impl BrilligGlobals { pub(crate) fn get_brillig_globals( &self, brillig_function_id: FunctionId, - ) -> SsaToBrilligGlobals { + ) -> (SsaToBrilligGlobals, HoistedConstantsToBrilligGlobals) { let entry_points = self.inner_call_to_entry_point.get(&brillig_function_id); let mut globals_allocations = HashMap::default(); + let mut hoisted_constants_allocations = HashMap::default(); if let Some(entry_points) = entry_points { // A Brillig function is used by multiple entry points. Fetch both globals allocations // in case one is used by the internal call. @@ -197,17 +300,26 @@ impl BrilligGlobals { for map in entry_point_allocations { globals_allocations.extend(map); } + + // The global memory space is not shared across entry points. + // Thus, if a call can be used by two entry points we should not use the hoisted constant allocations. + if entry_points.len() == 1 { + let hoisted_allocations = self.entry_point_hoisted_globals_map.get(&entry_points[0]).unwrap_or_else(|| panic!("ICE: Expected hoisted constant allocations to be set for function {brillig_function_id}")); + hoisted_constants_allocations.extend(hoisted_allocations); + } } else if let Some(globals) = self.entry_point_globals_map.get(&brillig_function_id) { // If there is no mapping from an inner call to an entry point, that means `brillig_function_id` // is itself an entry point and we can fetch the global allocations directly from `self.entry_point_globals_map`. - // vec![globals] globals_allocations.extend(globals); + + let hoisted_allocations = self.entry_point_hoisted_globals_map.get(&brillig_function_id).unwrap_or_else(|| panic!("ICE: Expected hoisted constant allocations to be set for function {brillig_function_id}")); + hoisted_constants_allocations.extend(hoisted_allocations); } else { unreachable!( "ICE: Expected global allocation to be set for function {brillig_function_id}" ); } - globals_allocations + (globals_allocations, hoisted_constants_allocations) } } @@ -246,7 +358,7 @@ pub(crate) fn convert_ssa_globals( }; let hoisted_global_constants = - brillig_block.compile_globals(&globals_dfg, used_globals, hoisted_global_consts); + brillig_block.compile_globals(globals_dfg, used_globals, hoisted_global_consts); let globals_size = brillig_context.global_space_size(); diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 9fb778eb9aa..d652cabfb2a 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -2,8 +2,8 @@ pub(crate) mod brillig_gen; pub(crate) mod brillig_ir; use acvm::FieldElement; -use brillig_gen::{brillig_globals::convert_ssa_globals, constant_allocation::ConstantAllocation}; use brillig_gen::brillig_globals::BrilligGlobals; +use brillig_gen::constant_allocation::ConstantAllocation; use brillig_ir::{artifact::LabelType, brillig_variable::BrilligVariable, registers::GlobalSpace}; use self::{ @@ -18,8 +18,8 @@ use crate::ssa::{ ir::{ dfg::DataFlowGraph, function::{Function, FunctionId}, - types::NumericType, instruction::Instruction, + types::NumericType, value::{Value, ValueId}, }, opt::inlining::called_functions_vec, @@ -104,42 +104,10 @@ impl Ssa { return brillig; } - // We can potentially have multiple local constants with the same value and type - let mut hoisted_global_constants: HashMap<(FieldElement, NumericType), usize> = - HashMap::default(); - for brillig_function_id in brillig_reachable_function_ids.iter() { - let function = &self.functions[brillig_function_id]; - let constants = ConstantAllocation::from_function(function); - for (constant, _) in constants.constant_usage { - let value = function.dfg.get_numeric_constant(constant); - let value = value.unwrap(); - let typ = function.dfg.type_of_value(constant); - if !function.dfg.is_global(constant) { - hoisted_global_constants - .entry((value, typ.unwrap_numeric())) - .and_modify(|counter| *counter += 1) - .or_insert(1); - } - } - } - - // We want to hoist only if there are repeat occurrences of a constant. - let hoisted_global_constants = hoisted_global_constants - .into_iter() - .filter_map( - |(value, num_occurrences)| { - if num_occurrences > 1 { - Some(value) - } else { - None - } - }, - ) - .collect::>(); - let mut brillig_globals = BrilligGlobals::new(&self.functions, used_globals_map, self.main_id); + // brillig_globals.fetch_global_constants_to_hoist(&self.functions); // SSA Globals are computed once at compile time and shared across all functions, // thus we can just fetch globals from the main function. // This same globals graph will then be used to declare Brillig globals for the respective entry points. @@ -148,10 +116,16 @@ impl Ssa { brillig_globals.declare_globals(&globals_dfg, &mut brillig, enable_debug_trace); for brillig_function_id in brillig_reachable_function_ids { - let globals_allocations = brillig_globals.get_brillig_globals(brillig_function_id); + let (globals_allocations, hoisted_constant_allocations) = + brillig_globals.get_brillig_globals(brillig_function_id); let func = &self.functions[&brillig_function_id]; - brillig.compile(func, enable_debug_trace, &globals_allocations, &HashMap::default()); + brillig.compile( + func, + enable_debug_trace, + &globals_allocations, + &hoisted_constant_allocations, + ); } brillig diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 96fcb7a2fc6..79fece8a76d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -88,7 +88,7 @@ impl Ssa { &globals_dfg, used_globals, &fxhash::FxHashSet::default(), - function.id() + function.id(), ); global_cache = Some(brillig_globals); } From 637b1577e63d6ec3170d6e7e9a928ee2db0f68e4 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 00:13:08 +0000 Subject: [PATCH 06/16] fix brillig globals unit tests --- .../brillig/brillig_gen/brillig_globals.rs | 21 ------------------- 1 file changed, 21 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 6c019849887..584f03cd748 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -88,21 +88,6 @@ impl BrilligGlobals { im::HashSet::new(), &mut hoisted_global_constants, ); - let function = &functions[func_id]; - let constants = ConstantAllocation::from_function(function); - for (constant, _) in constants.constant_usage { - let value = function.dfg.get_numeric_constant(constant); - let value = value.unwrap(); - let typ = function.dfg.type_of_value(constant); - if !function.dfg.is_global(constant) { - hoisted_global_constants - .entry(*func_id) - .or_default() - .entry((value, typ.unwrap_numeric())) - .and_modify(|counter| *counter += 1) - .or_insert(1); - } - } } } } @@ -243,12 +228,6 @@ impl BrilligGlobals { let entry_point = *entry_point; let used_globals = self.used_globals.remove(&entry_point).unwrap_or_default(); - // let hoisted_global_constants = if exempt_hoisting.contains(&entry_point) { - // // HashSet::default() - // self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default() - // } else { - // self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default() - // }; let hoisted_global_constants = self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default(); let (artifact, brillig_globals, globals_size, hoisted_global_constants) = From 09935116b13ff3a8bc9715a587f9ca9a1a800ffb Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 02:43:34 +0000 Subject: [PATCH 07/16] fixup brillig context structure --- .../brillig/brillig_gen/brillig_globals.rs | 130 ++++++++---------- .../brillig_gen/constant_allocation.rs | 6 +- 2 files changed, 63 insertions(+), 73 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 584f03cd748..3afff613aba 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -26,17 +26,19 @@ pub(crate) struct BrilligGlobals { /// This includes any nested calls as well, as we want to be able to associate /// any Brillig function with the appropriate global allocations. brillig_entry_points: HashMap>, - /// Maps a Brillig entry point to constants shared across the entry point and its nested calls. - final_hoisted_global_constants: HashMap>, + hoisted_global_constants: HashMap, /// Maps an inner call to its Brillig entry point /// This is simply used to simplify fetching global allocations when compiling /// individual Brillig functions. inner_call_to_entry_point: HashMap>, - /// Final map that associated an entry point with its Brillig global allocations + /// Final map that associates an entry point with its Brillig global allocations entry_point_globals_map: HashMap, - // TODO: combine this map with the one above + /// Final map that associates an entry point with any local function constants + /// that are shared and were hoisted to the global space. + /// This map is kept separate from `entry_point_globals_map` to clearly distinguish + /// the two types of globals. entry_point_hoisted_globals_map: HashMap, } @@ -51,13 +53,27 @@ pub(crate) type ConstantCounterMap = HashMap<(FieldElement, NumericType), usize> impl BrilligGlobals { pub(crate) fn new( functions: &BTreeMap, - mut used_globals: HashMap>, + used_globals: HashMap>, main_id: FunctionId, ) -> Self { - let mut brillig_entry_points = HashMap::default(); - let mut hoisted_global_constants: HashMap = - HashMap::default(); + let mut instance = Self { used_globals, ..Default::default() }; + instance.build_entry_point_info_from_call_graph(functions, main_id); + + instance + } + + /// This methods collects information from the call graph of a Brillig entry point. + /// - Update the used globals for a given entry point based upon its call graph. + /// - Determine constants which are shared across multiple functions for possible hoisting + /// into the global memory space. + /// - Build a simplified call graph of an entry point to all functions + /// possibly called from that entry point. + fn build_entry_point_info_from_call_graph( + &mut self, + functions: &BTreeMap, + main_id: FunctionId, + ) { let acir_functions = functions.iter().filter(|(_, func)| func.runtime().is_acir()); for (_, function) in acir_functions { for block_id in function.reachable_blocks() { @@ -78,15 +94,12 @@ impl BrilligGlobals { // We have now found a Brillig entry point. // Let's recursively build a call graph to determine any functions // whose parent is this entry point and any globals used in those internal calls. - brillig_entry_points.insert(*func_id, HashSet::default()); - Self::mark_entry_points_calls_recursive( + self.brillig_entry_points.insert(*func_id, HashSet::default()); + self.mark_entry_points_calls_recursive( functions, *func_id, called_function, - &mut used_globals, - &mut brillig_entry_points, im::HashSet::new(), - &mut hoisted_global_constants, ); } } @@ -96,47 +109,14 @@ impl BrilligGlobals { // Run the same analysis from above on main. let main_func = &functions[&main_id]; if main_func.runtime().is_brillig() { - brillig_entry_points.insert(main_id, HashSet::default()); - Self::mark_entry_points_calls_recursive( + self.brillig_entry_points.insert(main_id, HashSet::default()); + self.mark_entry_points_calls_recursive( functions, main_id, main_func, - &mut used_globals, - &mut brillig_entry_points, im::HashSet::new(), - &mut hoisted_global_constants, ); } - - let final_hoisted_global_constants: HashMap< - FunctionId, - HashSet<(FieldElement, NumericType)>, - > = hoisted_global_constants - .iter() - .map(|(entry_point, constants_for_entry_point)| { - let hoisted_global_constants: HashSet<_> = constants_for_entry_point - .iter() - .filter_map( - |(&value, &num_occurrences)| { - if num_occurrences > 1 { - Some(value) - } else { - None - } - }, - ) - .collect(); - - (*entry_point, hoisted_global_constants) - }) - .collect(); - - Self { - used_globals, - brillig_entry_points, - final_hoisted_global_constants, - ..Default::default() - } } /// Recursively mark any functions called in an entry point as well as @@ -144,13 +124,11 @@ impl BrilligGlobals { /// Using the information collected we can determine which globals /// an entry point must initialize. fn mark_entry_points_calls_recursive( + &mut self, functions: &BTreeMap, entry_point: FunctionId, called_function: &Function, - used_globals: &mut HashMap>, - brillig_entry_points: &mut HashMap>, mut explored_functions: im::HashSet, - hoisted_global_constants: &mut HashMap, ) { if explored_functions.insert(called_function.id()).is_some() { return; @@ -158,12 +136,12 @@ impl BrilligGlobals { // We can potentially have multiple local constants with the same value and type let constants = ConstantAllocation::from_function(called_function); - for (constant, _) in constants.constant_usage { + for constant in constants.get_constants() { let value = called_function.dfg.get_numeric_constant(constant); let value = value.unwrap(); let typ = called_function.dfg.type_of_value(constant); if !called_function.dfg.is_global(constant) { - hoisted_global_constants + self.hoisted_global_constants .entry(entry_point) .or_default() .entry((value, typ.unwrap_numeric())) @@ -175,27 +153,25 @@ impl BrilligGlobals { let inner_calls = called_functions_vec(called_function).into_iter().collect::>(); for inner_call in inner_calls { - let inner_globals = used_globals + let inner_globals = self + .used_globals .get(&inner_call) .expect("Should have a slot for each function") .clone(); - used_globals + self.used_globals .get_mut(&entry_point) .expect("ICE: should have func") .extend(inner_globals); - if let Some(inner_calls) = brillig_entry_points.get_mut(&entry_point) { + if let Some(inner_calls) = self.brillig_entry_points.get_mut(&entry_point) { inner_calls.insert(inner_call); } - Self::mark_entry_points_calls_recursive( + self.mark_entry_points_calls_recursive( functions, entry_point, &functions[&inner_call], - used_globals, - brillig_entry_points, explored_functions.clone(), - hoisted_global_constants, ); } } @@ -209,27 +185,37 @@ impl BrilligGlobals { // Map for fetching the correct entry point globals when compiling any function let mut inner_call_to_entry_point: HashMap> = HashMap::default(); - // We only need to generate globals for entry points - for (entry_point, entry_point_inner_calls) in self.brillig_entry_points.iter() { - let entry_point = *entry_point; - - // dbg!(entry_point_inner_calls.clone()); - for inner_call in entry_point_inner_calls { - inner_call_to_entry_point.entry(*inner_call).or_default().push(entry_point); - } - } let mut entry_point_globals_map = HashMap::default(); let mut entry_point_hoisted_globals_map = HashMap::default(); let mut all_hoisted_allocations = HashMap::default(); // We only need to generate globals for entry points - for (entry_point, _entry_point_inner_calls) in self.brillig_entry_points.iter() { + for (entry_point, entry_point_inner_calls) in self.brillig_entry_points.iter() { let entry_point = *entry_point; + for inner_call in entry_point_inner_calls { + inner_call_to_entry_point.entry(*inner_call).or_default().push(entry_point); + } + let used_globals = self.used_globals.remove(&entry_point).unwrap_or_default(); - let hoisted_global_constants = - self.final_hoisted_global_constants.remove(&entry_point).unwrap_or_default(); + // Select set of constants which can be hoisted from function's to the global memory space + // for a given entry point. + let hoisted_global_constants = self + .hoisted_global_constants + .remove(&entry_point) + .unwrap_or_default() + .iter() + .filter_map( + |(&value, &num_occurrences)| { + if num_occurrences > 1 { + Some(value) + } else { + None + } + }, + ) + .collect(); let (artifact, brillig_globals, globals_size, hoisted_global_constants) = convert_ssa_globals( enable_debug_trace, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index 3cddc61b7ff..d49876ef1d9 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -24,7 +24,7 @@ pub(crate) enum InstructionLocation { #[derive(Default)] pub(crate) struct ConstantAllocation { - pub(crate) constant_usage: HashMap>>, + constant_usage: HashMap>>, allocation_points: HashMap>>, dominator_tree: DominatorTree, blocks_within_loops: HashSet, @@ -163,6 +163,10 @@ impl ConstantAllocation { } current_block } + + pub(crate) fn get_constants(&self) -> HashSet { + self.constant_usage.keys().copied().collect() + } } pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool { From bd0473e67c81e59d454ebe3566cd61eb7b5041c1 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 03:04:10 +0000 Subject: [PATCH 08/16] helper for checking if we have hoisted global when compiling a block --- .../src/brillig/brillig_gen/brillig_block.rs | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 9c1e06faca3..45d7c93eca9 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -29,6 +29,7 @@ use std::sync::Arc; use super::brillig_black_box::convert_black_box_call; use super::brillig_block_variables::{allocate_value_with_type, BlockVariables}; use super::brillig_fn::FunctionContext; +use super::brillig_globals::HoistedConstantsToBrilligGlobals; use super::constant_allocation::InstructionLocation; /// Generate the compilation artifacts for compiling a function into brillig bytecode. @@ -44,8 +45,7 @@ pub(crate) struct BrilligBlock<'block, Registers: RegisterAllocator> { pub(crate) last_uses: HashMap>, pub(crate) globals: &'block HashMap, - pub(crate) hoisted_global_constants: - &'block HashMap<(FieldElement, NumericType), BrilligVariable>, + pub(crate) hoisted_global_constants: &'block HoistedConstantsToBrilligGlobals, pub(crate) building_globals: bool, } @@ -58,16 +58,12 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { block_id: BasicBlockId, dfg: &DataFlowGraph, globals: &'block HashMap, - hoisted_global_constants: &'block HashMap<(FieldElement, NumericType), BrilligVariable>, + hoisted_global_constants: &'block HoistedConstantsToBrilligGlobals, ) { - let live_in: &std::collections::HashSet< - crate::ssa::ir::map::Id, - std::hash::BuildHasherDefault, - > = function_context.liveness.get_live_in(&block_id); + let live_in = function_context.liveness.get_live_in(&block_id); let mut live_in_no_globals = HashSet::default(); for value in live_in { - // TODO: Move this check into a helper function if let Value::NumericConstant { constant, typ } = dfg[*value] { if hoisted_global_constants.get(&(constant, typ)).is_some() { continue; @@ -930,13 +926,9 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { .expect("Last uses for instruction should have been computed"); for dead_variable in dead_variables { - if let Value::NumericConstant { constant, typ } = &dfg[*dead_variable] { - if self.hoisted_global_constants.get(&(*constant, *typ)).is_some() { - continue; - } - } // Globals are reserved throughout the entirety of the program - if !dfg.is_global(*dead_variable) { + let not_hoisted_global = self.get_hoisted_global(dfg, *dead_variable).is_none(); + if !dfg.is_global(*dead_variable) && not_hoisted_global { self.variables.remove_variable( dead_variable, self.function_context, @@ -1654,12 +1646,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let value_id = dfg.resolve(value_id); let value = &dfg[value_id]; - if !self.building_globals { - if let Value::NumericConstant { constant, typ } = value { - if let Some(variable) = self.hoisted_global_constants.get(&(*constant, *typ)) { - return *variable; - } - } + if let Some(variable) = self.get_hoisted_global(dfg, value_id) { + return variable; } match value { @@ -2002,6 +1990,19 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } } + + fn get_hoisted_global( + &self, + dfg: &DataFlowGraph, + value_id: ValueId, + ) -> Option { + if let Value::NumericConstant { constant, typ } = &dfg[value_id] { + if let Some(variable) = self.hoisted_global_constants.get(&(*constant, *typ)) { + return Some(*variable); + } + } + None + } } /// Returns the type of the operation considering the types of the operands From 6fa2d058c50fd4c6b92b9a96212b4b6626f1790c Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 29 Jan 2025 22:08:20 -0500 Subject: [PATCH 09/16] Apply suggestions from code review --- compiler/noirc_evaluator/src/brillig/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index d652cabfb2a..914d00f7995 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -107,7 +107,6 @@ impl Ssa { let mut brillig_globals = BrilligGlobals::new(&self.functions, used_globals_map, self.main_id); - // brillig_globals.fetch_global_constants_to_hoist(&self.functions); // SSA Globals are computed once at compile time and shared across all functions, // thus we can just fetch globals from the main function. // This same globals graph will then be used to declare Brillig globals for the respective entry points. From e0ccaa7538b7d59d69d669ffc517f2d46af6440a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 08:05:44 -0500 Subject: [PATCH 10/16] Apply suggestions from code review --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 45d7c93eca9..c7c20d823cd 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -103,7 +103,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { &mut self, globals: &DataFlowGraph, used_globals: &HashSet, - hoisted_global_consts: &HashSet<(FieldElement, NumericType)>, + hoisted_global_constants: &HashSet<(FieldElement, NumericType)>, ) -> HashMap<(FieldElement, NumericType), BrilligVariable> { let mut new_hoisted_constants = HashMap::default(); @@ -126,7 +126,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } - for (constant, typ) in hoisted_global_consts { + for (constant, typ) in hoisted_global_constants { let constant = *constant; let typ = *typ; let new_variable = allocate_value_with_type(self.brillig_context, Type::Numeric(typ)); From 0f7940c82bf79ef206316c80761525ee5f7170eb Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 08:06:04 -0500 Subject: [PATCH 11/16] Update compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs --- .../src/brillig/brillig_gen/brillig_block_variables.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 10761caea9d..4cf8e921483 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -21,7 +21,7 @@ use super::brillig_fn::FunctionContext; #[derive(Debug, Default)] pub(crate) struct BlockVariables { - pub(crate) available_variables: HashSet, + available_variables: HashSet, } impl BlockVariables { From eb3b3c184e7a03092ed2ad4bc2f7d2095a8053e8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 08:16:33 -0500 Subject: [PATCH 12/16] Apply suggestions from code review --- .../src/brillig/brillig_gen/brillig_globals.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 3afff613aba..d9e707a4c74 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -299,7 +299,7 @@ pub(crate) fn convert_ssa_globals( enable_debug_trace: bool, globals_dfg: &DataFlowGraph, used_globals: &HashSet, - hoisted_global_consts: &HashSet<(FieldElement, NumericType)>, + hoisted_global_constants: &HashSet<(FieldElement, NumericType)>, entry_point: FunctionId, ) -> BrilligGlobalsArtifact { let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace, entry_point); @@ -323,7 +323,7 @@ pub(crate) fn convert_ssa_globals( }; let hoisted_global_constants = - brillig_block.compile_globals(globals_dfg, used_globals, hoisted_global_consts); + brillig_block.compile_globals(globals_dfg, used_globals, hoisted_global_constants); let globals_size = brillig_context.global_space_size(); From 631a6556709914b3d5088eeda5828168c4f9fb76 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 18:03:27 +0000 Subject: [PATCH 13/16] unit tests for shared constant hoisting --- .../src/brillig/brillig_gen/brillig_block.rs | 3 +- .../brillig/brillig_gen/brillig_globals.rs | 160 +++++++++++++++++- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 2 +- .../global_var_regression_simple/src/main.nr | 5 +- 4 files changed, 161 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index c7c20d823cd..fc9fd559754 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -24,6 +24,7 @@ use acvm::{acir::AcirField, FieldElement}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use iter_extended::vecmap; use num_bigint::BigUint; +use std::collections::BTreeSet; use std::sync::Arc; use super::brillig_black_box::convert_black_box_call; @@ -103,7 +104,7 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { &mut self, globals: &DataFlowGraph, used_globals: &HashSet, - hoisted_global_constants: &HashSet<(FieldElement, NumericType)>, + hoisted_global_constants: &BTreeSet<(FieldElement, NumericType)>, ) -> HashMap<(FieldElement, NumericType), BrilligVariable> { let mut new_hoisted_constants = HashMap::default(); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index d9e707a4c74..9fae6e46c53 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -1,4 +1,4 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use acvm::FieldElement; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; @@ -299,7 +299,7 @@ pub(crate) fn convert_ssa_globals( enable_debug_trace: bool, globals_dfg: &DataFlowGraph, used_globals: &HashSet, - hoisted_global_constants: &HashSet<(FieldElement, NumericType)>, + hoisted_global_constants: &BTreeSet<(FieldElement, NumericType)>, entry_point: FunctionId, ) -> BrilligGlobalsArtifact { let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace, entry_point); @@ -335,13 +335,17 @@ pub(crate) fn convert_ssa_globals( #[cfg(test)] mod tests { + use std::collections::BTreeSet; + use acvm::{ - acir::brillig::{BitSize, Opcode}, + acir::brillig::{BitSize, IntegerBitSize, Opcode}, FieldElement, }; use crate::brillig::{brillig_ir::registers::RegisterAllocator, GlobalSpace, LabelType, Ssa}; + use super::ConstantAllocation; + #[test] fn entry_points_different_globals() { let src = " @@ -539,4 +543,154 @@ mod tests { } } } + + #[test] + fn hoist_shared_constants() { + let src = " + acir(inline) predicate_pure fn main f0 { + b0(v0: Field, v1: Field): + call f1(v0, v1) + return + } + brillig(inline) predicate_pure fn entry_point f1 { + b0(v0: Field, v1: Field): + v2 = add v0, v1 + v4 = add v2, Field 1 + v6 = eq v4, Field 5 + constrain v6 == u1 0 + call f2(v0, v1) + return + } + brillig(inline) predicate_pure fn inner_func f2 { + b0(v0: Field, v1: Field): + v3 = eq v0, Field 20 + constrain v3 == u1 0 + v5 = add v0, v1 + v7 = add v5, Field 10 + v9 = add v7, Field 1 + v11 = eq v9, Field 20 + constrain v11 == u1 0 + return + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + // Need to run DIE to generate the used globals map, which is necessary for Brillig globals generation. + let mut ssa = ssa.dead_instruction_elimination(); + + // Show that the constants in each function have different SSA value IDs + for (func_id, function) in &ssa.functions { + let constant_allocation = ConstantAllocation::from_function(function); + let mut constants = constant_allocation.get_constants().into_iter().collect::>(); + // We want to order the constants by ID + constants.sort(); + if func_id.to_u32() == 1 { + assert_eq!(constants.len(), 3); + let one = function.dfg.get_numeric_constant(constants[0]).unwrap(); + assert_eq!(one, FieldElement::from(1u128)); + let five = function.dfg.get_numeric_constant(constants[1]).unwrap(); + assert_eq!(five, FieldElement::from(5u128)); + let zero = function.dfg.get_numeric_constant(constants[2]).unwrap(); + assert_eq!(zero, FieldElement::from(0u128)); + } else if func_id.to_u32() == 2 { + assert_eq!(constants.len(), 4); + let twenty = function.dfg.get_numeric_constant(constants[0]).unwrap(); + assert_eq!(twenty, FieldElement::from(20u128)); + let zero = function.dfg.get_numeric_constant(constants[1]).unwrap(); + assert_eq!(zero, FieldElement::from(0u128)); + let ten = function.dfg.get_numeric_constant(constants[2]).unwrap(); + assert_eq!(ten, FieldElement::from(10u128)); + let one = function.dfg.get_numeric_constant(constants[3]).unwrap(); + assert_eq!(one, FieldElement::from(1u128)); + } + } + + let used_globals_map = std::mem::take(&mut ssa.used_globals); + let brillig = ssa.to_brillig_with_globals(false, used_globals_map); + + assert_eq!(brillig.globals.len(), 1, "Should have a single entry point"); + for (func_id, artifact) in brillig.globals { + assert_eq!(func_id.to_u32(), 1); + assert_eq!( + artifact.byte_code.len(), + 3, + "Expected enough opcodes to initialize the hoisted constants" + ); + let Opcode::Const { destination, bit_size, value } = &artifact.byte_code[0] else { + panic!("First opcode is expected to be `Const`"); + }; + assert_eq!(destination.unwrap_direct(), GlobalSpace::start()); + assert!(matches!(bit_size, BitSize::Integer(IntegerBitSize::U1))); + assert_eq!(*value, FieldElement::from(0u128)); + + let Opcode::Const { destination, bit_size, value } = &artifact.byte_code[1] else { + panic!("First opcode is expected to be `Const`"); + }; + assert_eq!(destination.unwrap_direct(), GlobalSpace::start() + 1); + assert!(matches!(bit_size, BitSize::Field)); + assert_eq!(*value, FieldElement::from(1u128)); + + assert!(matches!(&artifact.byte_code[2], Opcode::Return)); + } + } + + #[test] + fn do_not_hoist_shared_constants_different_entry_points() { + let src = " + acir(inline) predicate_pure fn main f0 { + b0(v0: Field, v1: Field): + call f1(v0, v1) + call f2(v0, v1) + return + } + brillig(inline) predicate_pure fn entry_point f1 { + b0(v0: Field, v1: Field): + v2 = add v0, v1 + v4 = add v2, Field 1 + v6 = eq v4, Field 5 + constrain v6 == u1 0 + return + } + brillig(inline) predicate_pure fn entry_point_two f2 { + b0(v0: Field, v1: Field): + v3 = eq v0, Field 20 + constrain v3 == u1 0 + v5 = add v0, v1 + v7 = add v5, Field 10 + v9 = add v7, Field 1 + v10 = eq v9, Field 20 + constrain v10 == u1 0 + return + } + "; + + let ssa = Ssa::from_str(src).unwrap(); + // Need to run DIE to generate the used globals map, which is necessary for Brillig globals generation. + let mut ssa = ssa.dead_instruction_elimination(); + + let used_globals_map = std::mem::take(&mut ssa.used_globals); + let brillig = ssa.to_brillig_with_globals(false, used_globals_map); + + assert_eq!( + brillig.globals.len(), + 2, + "Should have a globals artifact associated with each entry point" + ); + for (func_id, mut artifact) in brillig.globals { + let labels = artifact.take_labels(); + // When entering a context two labels are created. + // One is a context label and another is a section label. + assert_eq!(labels.len(), 2); + for (label, position) in labels { + assert_eq!(label.label_type, LabelType::GlobalInit(func_id)); + assert_eq!(position, 0); + } + assert_eq!( + artifact.byte_code.len(), + 1, + "Expected enough opcodes to initialize the hoisted constants" + ); + assert!(matches!(&artifact.byte_code[0], Opcode::Return)); + } + } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 79fece8a76d..33b7462907d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -87,7 +87,7 @@ impl Ssa { false, &globals_dfg, used_globals, - &fxhash::FxHashSet::default(), + &BTreeSet::default(), function.id(), ); global_cache = Some(brillig_globals); diff --git a/test_programs/execution_success/global_var_regression_simple/src/main.nr b/test_programs/execution_success/global_var_regression_simple/src/main.nr index 436aeb2652c..8af1879b042 100644 --- a/test_programs/execution_success/global_var_regression_simple/src/main.nr +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -14,15 +14,12 @@ fn main(x: Field, y: pub Field) { } fn dummy_again(x: Field, y: Field) { - // Move this before as to change the value ids of the common constants - // between this function and main - assert(x != y); - let mut acc: Field = 0; for i in 0..2 { for j in 0..2 { acc += EXPONENTIATE[i][j]; } } + assert(x != y); assert(!acc.lt(x)); } From 1d68b7dd1204d59d21505915f5f0508823ae2359 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 18:11:38 +0000 Subject: [PATCH 14/16] cmments on globals artifact type --- .../src/brillig/brillig_gen/brillig_globals.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 9fae6e46c53..92d6123f5a6 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -288,10 +288,17 @@ impl BrilligGlobals { } } +/// A globals artifact containing all information necessary for utilizing +/// globals from SSA during Brillig code generation. pub(crate) type BrilligGlobalsArtifact = ( + // The actual bytecode declaring globals and any metadata needing for linking BrilligArtifact, + // The SSA value -> Brillig global allocations + // This will be used for fetching global values when compiling functions to Brillig. HashMap, + // The size of the global memory usize, + // Duplicate SSA constants local to a function -> Brillig global allocations HashMap<(FieldElement, NumericType), BrilligVariable>, ); @@ -335,8 +342,6 @@ pub(crate) fn convert_ssa_globals( #[cfg(test)] mod tests { - use std::collections::BTreeSet; - use acvm::{ acir::brillig::{BitSize, IntegerBitSize, Opcode}, FieldElement, From 40c3b5b6e7339836660811fb8b61556d171c987b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 18:14:58 +0000 Subject: [PATCH 15/16] split up long line --- .../src/brillig/brillig_gen/brillig_globals.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 92d6123f5a6..f899f22d467 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -269,7 +269,9 @@ impl BrilligGlobals { // The global memory space is not shared across entry points. // Thus, if a call can be used by two entry points we should not use the hoisted constant allocations. if entry_points.len() == 1 { - let hoisted_allocations = self.entry_point_hoisted_globals_map.get(&entry_points[0]).unwrap_or_else(|| panic!("ICE: Expected hoisted constant allocations to be set for function {brillig_function_id}")); + let hoisted_allocations = + self.entry_point_hoisted_globals_map.get(&brillig_function_id); + let hoisted_allocations = hoisted_allocations.unwrap_or_else(|| panic!("ICE: Expected hoisted allocations to be set for function {brillig_function_id}")); hoisted_constants_allocations.extend(hoisted_allocations); } } else if let Some(globals) = self.entry_point_globals_map.get(&brillig_function_id) { @@ -277,7 +279,9 @@ impl BrilligGlobals { // is itself an entry point and we can fetch the global allocations directly from `self.entry_point_globals_map`. globals_allocations.extend(globals); - let hoisted_allocations = self.entry_point_hoisted_globals_map.get(&brillig_function_id).unwrap_or_else(|| panic!("ICE: Expected hoisted constant allocations to be set for function {brillig_function_id}")); + let hoisted_allocations = + self.entry_point_hoisted_globals_map.get(&brillig_function_id); + let hoisted_allocations = hoisted_allocations.unwrap_or_else(|| panic!("ICE: Expected hoisted allocations to be set for function {brillig_function_id}")); hoisted_constants_allocations.extend(hoisted_allocations); } else { unreachable!( From 2e5e5d71fb95c9c1ca1f4e3ead1df8570552c922 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 30 Jan 2025 18:22:07 +0000 Subject: [PATCH 16/16] use entry_points[0] not brillig_function_id --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index f899f22d467..f522dcecc31 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -270,7 +270,7 @@ impl BrilligGlobals { // Thus, if a call can be used by two entry points we should not use the hoisted constant allocations. if entry_points.len() == 1 { let hoisted_allocations = - self.entry_point_hoisted_globals_map.get(&brillig_function_id); + self.entry_point_hoisted_globals_map.get(&entry_points[0]); let hoisted_allocations = hoisted_allocations.unwrap_or_else(|| panic!("ICE: Expected hoisted allocations to be set for function {brillig_function_id}")); hoisted_constants_allocations.extend(hoisted_allocations); }