diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index f23e64aec52..6f653f2b85d 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..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,11 +24,13 @@ 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; -use super::brillig_block_variables::BlockVariables; +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,6 +46,7 @@ pub(crate) struct BrilligBlock<'block, Registers: RegisterAllocator> { pub(crate) last_uses: HashMap>, pub(crate) globals: &'block HashMap, + pub(crate) hoisted_global_constants: &'block HoistedConstantsToBrilligGlobals, pub(crate) building_globals: bool, } @@ -56,11 +59,17 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { block_id: BasicBlockId, dfg: &DataFlowGraph, globals: &'block HashMap, + hoisted_global_constants: &'block HoistedConstantsToBrilligGlobals, ) { let live_in = function_context.liveness.get_live_in(&block_id); let mut live_in_no_globals = HashSet::default(); for value in live_in { + 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,10 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { &mut self, globals: &DataFlowGraph, used_globals: &HashSet, - ) { + hoisted_global_constants: &BTreeSet<(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; @@ -113,6 +126,17 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { } } } + + 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)); + 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"); + } + } + new_hoisted_constants } fn convert_block(&mut self, dfg: &DataFlowGraph) { @@ -904,7 +928,8 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { for dead_variable in dead_variables { // 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, @@ -1622,6 +1647,10 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> { let value_id = dfg.resolve(value_id); let value = &dfg[value_id]; + if let Some(variable) = self.get_hoisted_global(dfg, value_id) { + return variable; + } + match value { Value::Global(_) => { unreachable!("Expected global value to be resolve to its inner value"); @@ -1962,6 +1991,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 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 6f5645485a2..f522dcecc31 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}; @@ -6,9 +6,10 @@ use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use super::{ BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId, }; -use crate::brillig::{ - brillig_ir::BrilligContext, called_functions_vec, Brillig, DataFlowGraph, 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 @@ -25,25 +26,54 @@ 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. + 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, + /// 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, } /// 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, - mut used_globals: HashMap>, + used_globals: HashMap>, main_id: FunctionId, ) -> Self { - let mut brillig_entry_points = 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() { @@ -64,13 +94,11 @@ 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(), ); } @@ -81,18 +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(), ); } - - Self { used_globals, brillig_entry_points, ..Default::default() } } /// Recursively mark any functions called in an entry point as well as @@ -100,39 +124,53 @@ 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, ) { 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.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) { + self.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 { - 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(), ); } @@ -147,7 +185,11 @@ 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(); + 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; @@ -157,10 +199,36 @@ impl BrilligGlobals { } let used_globals = self.used_globals.remove(&entry_point).unwrap_or_default(); - let (artifact, brillig_globals, globals_size) = - convert_ssa_globals(enable_debug_trace, globals_dfg, &used_globals, entry_point); + // 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, + 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); @@ -168,6 +236,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 @@ -181,10 +250,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. @@ -195,26 +265,54 @@ 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]); + 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) { // 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); + 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!( "ICE: Expected global allocation to be set for function {brillig_function_id}" ); } - globals_allocations + (globals_allocations, hoisted_constants_allocations) } } +/// 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>, +); + pub(crate) fn convert_ssa_globals( enable_debug_trace: bool, globals_dfg: &DataFlowGraph, used_globals: &HashSet, + hoisted_global_constants: &BTreeSet<(FieldElement, NumericType)>, entry_point: FunctionId, -) -> (BrilligArtifact, HashMap, usize) { +) -> BrilligGlobalsArtifact { let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace, entry_point); // The global space does not have globals itself let empty_globals = HashMap::default(); @@ -231,28 +329,32 @@ pub(crate) fn convert_ssa_globals( variables: Default::default(), last_uses: HashMap::default(), globals: &empty_globals, + hoisted_global_constants: &HashMap::default(), building_globals: true, }; - brillig_block.compile_globals(globals_dfg, used_globals); + let hoisted_global_constants = + brillig_block.compile_globals(globals_dfg, used_globals, hoisted_global_constants); let globals_size = 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) } #[cfg(test)] mod tests { 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 = " @@ -450,4 +552,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/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..d49876ef1d9 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -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 { diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 791f6a466cf..914d00f7995 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -3,6 +3,7 @@ pub(crate) mod brillig_ir; use acvm::FieldElement; 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,6 +19,7 @@ use crate::ssa::{ dfg::DataFlowGraph, function::{Function, FunctionId}, instruction::Instruction, + types::NumericType, value::{Value, ValueId}, }, opt::inlining::called_functions_vec, @@ -45,8 +47,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); } @@ -112,10 +115,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); + 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 f6dda107d9c..33b7462907d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -80,11 +80,16 @@ impl Ssa { if let Some(max_incr_pct) = max_bytecode_increase_percent { if global_cache.is_none() { 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 globals_dfg = DataFlowGraph::from(globals); - // DIE is run at the end of our SSA optimizations, so we mark all globals as in use here. - let (_, brillig_globals, _) = - convert_ssa_globals(false, &globals_dfg, used_globals, function.id()); + let (_, brillig_globals, _, _) = convert_ssa_globals( + false, + &globals_dfg, + used_globals, + &BTreeSet::default(), + function.id(), + ); global_cache = Some(brillig_globals); } let brillig_globals = global_cache.as_ref().unwrap(); @@ -1038,7 +1043,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..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 @@ -20,6 +20,6 @@ fn dummy_again(x: Field, y: Field) { acc += EXPONENTIATE[i][j]; } } - assert(!acc.lt(x)); assert(x != y); + assert(!acc.lt(x)); }