diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index 0b39c59d270..9d81daad1b1 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -500,7 +500,7 @@ impl<'a> Context<'a> { let mut warnings = Vec::new(); // Disable the side effects if the binary instruction does not require them let one = self.acir_context.add_constant(FieldElement::one()); - let predicate = if instruction.requires_acir_gen_predicate(dfg) { + let predicate = if instruction.requires_acir_gen_predicate(dfg, &ssa.function_purities) { self.current_side_effects_enabled_var } else { one diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 4ddd5106817..40cfd7526ea 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -26,7 +26,6 @@ use super::{ instruction::{ArrayOffset, ConstrainError, InstructionId, Intrinsic}, types::NumericType, }, - opt::pure::FunctionPurities, ssa_gen::Ssa, }; @@ -49,7 +48,6 @@ pub struct FunctionBuilder { pub simplify: bool, globals: Arc, - purities: Arc, } impl FunctionBuilder { @@ -67,7 +65,6 @@ impl FunctionBuilder { error_types: BTreeMap::default(), simplify: true, globals: Default::default(), - purities: Default::default(), } } @@ -76,9 +73,7 @@ impl FunctionBuilder { pub fn from_existing(function: &Function, function_id: FunctionId) -> Self { let mut this = Self::new(function.name().to_owned(), function_id); this.set_globals(function.dfg.globals.clone()); - this.purities = function.dfg.function_purities.clone(); this.current_function.set_runtime(function.runtime()); - this.current_function.dfg.set_function_purities(this.purities.clone()); this } @@ -107,11 +102,6 @@ impl FunctionBuilder { self.current_function.set_globals(self.globals.clone()); } - pub fn set_purities(&mut self, purities: Arc) { - self.purities = purities.clone(); - self.current_function.dfg.set_function_purities(purities); - } - /// Finish the current function and create a new function. /// /// A FunctionBuilder can always only work on one function at a time, so care @@ -134,7 +124,6 @@ impl FunctionBuilder { self.current_function.dfg.call_stack_data.get_or_insert_locations(&call_stack); self.finished_functions.push(old_function); - self.current_function.dfg.set_function_purities(self.purities.clone()); self.apply_globals(); } diff --git a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs index 5cc3776be51..6a266bd296f 100644 --- a/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/interpreter/mod.rs @@ -467,8 +467,9 @@ impl<'ssa, W: Write> Interpreter<'ssa, W> { match current_function.runtime() { RuntimeType::Acir(_) => { + let purities = &self.ssa.function_purities; self.side_effects_enabled - || !instruction.requires_acir_gen_predicate(¤t_function.dfg) + || !instruction.requires_acir_gen_predicate(¤t_function.dfg, purities) } RuntimeType::Brillig(_) => true, } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index a0af3979271..807630fa51d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -1,9 +1,6 @@ use std::{borrow::Cow, sync::Arc}; -use crate::ssa::{ - function_builder::data_bus::DataBus, - opt::pure::{FunctionPurities, Purity}, -}; +use crate::ssa::function_builder::data_bus::DataBus; use super::{ basic_block::{BasicBlock, BasicBlockId}, @@ -104,9 +101,6 @@ pub(crate) struct DataFlowGraph { pub(crate) data_bus: DataBus, pub(crate) globals: Arc, - - #[serde(skip)] - pub(crate) function_purities: Arc, } /// The GlobalsGraph contains the actual global data. @@ -768,14 +762,6 @@ impl DataFlowGraph { _ => None, } } - - pub(crate) fn set_function_purities(&mut self, purities: Arc) { - self.function_purities = purities; - } - - pub(crate) fn purity_of(&self, function: FunctionId) -> Option { - self.function_purities.get(&function).copied() - } } impl std::ops::Index for DataFlowGraph { diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index 4ed890da3a9..69acb6229fc 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -115,7 +115,6 @@ impl Function { let mut new_function = Function::new(another.name.clone(), id); new_function.set_runtime(another.runtime()); new_function.set_globals(another.dfg.globals.clone()); - new_function.dfg.set_function_purities(another.dfg.function_purities.clone()); new_function } diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 4dc2022cf82..0c6c4f28ec1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -10,7 +10,7 @@ use fxhash::FxHasher64; use iter_extended::vecmap; use noirc_frontend::hir_def::types::Type as HirType; -use crate::ssa::opt::pure::Purity; +use crate::ssa::opt::pure::{FunctionPurities, Purity}; use super::{ basic_block::BasicBlockId, @@ -377,7 +377,11 @@ impl Instruction { } /// If true the instruction will depend on `enable_side_effects` context during acir-gen. - pub(crate) fn requires_acir_gen_predicate(&self, dfg: &DataFlowGraph) -> bool { + pub(crate) fn requires_acir_gen_predicate( + &self, + dfg: &DataFlowGraph, + purities: &FunctionPurities, + ) -> bool { match self { Instruction::Binary(binary) => binary.requires_acir_gen_predicate(dfg), @@ -389,7 +393,7 @@ impl Instruction { Instruction::EnableSideEffectsIf { .. } | Instruction::ArraySet { .. } => true, Instruction::Call { func, .. } => match dfg[*func] { - Value::Function(id) => !matches!(dfg.purity_of(id), Some(Purity::Pure)), + Value::Function(id) => !matches!(purities.get(&id), Some(Purity::Pure)), Value::Intrinsic(intrinsic) => { // These utilize `noirc_evaluator::acir::Context::get_flattened_index` internally // which uses the side effects predicate. @@ -415,7 +419,11 @@ impl Instruction { } /// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory. - pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool { + pub(crate) fn has_side_effects( + &self, + dfg: &DataFlowGraph, + purities: &FunctionPurities, + ) -> bool { use Instruction::*; match self { @@ -431,7 +439,7 @@ impl Instruction { Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(), // Functions known to be pure have no side effects. // `PureWithPredicates` functions may still have side effects. - Value::Function(function) => dfg.purity_of(function) != Some(Purity::Pure), + Value::Function(function) => purities.get(&function).copied() != Some(Purity::Pure), _ => true, // Be conservative and assume other functions can have side effects. }, diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 5573a0e576d..943b18b6e78 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -12,6 +12,7 @@ use crate::ssa::{ instruction::ArrayOffset, types::{NumericType, Type}, }, + opt::pure::FunctionPurities, }; use super::{ @@ -68,7 +69,7 @@ impl Display for Printer<'_> { } for function in self.ssa.functions.values() { - display_function(function, self.fm, f)?; + display_function(function, self.fm, &self.ssa.function_purities, f)?; writeln!(f)?; } Ok(()) @@ -77,7 +78,7 @@ impl Display for Printer<'_> { impl Display for Function { fn fmt(&self, f: &mut Formatter<'_>) -> Result { - display_function(self, None, f) + display_function(self, None, &FunctionPurities::default(), f) } } @@ -85,9 +86,10 @@ impl Display for Function { fn display_function( function: &Function, files: Option<&fm::FileManager>, + purities: &FunctionPurities, f: &mut Formatter, ) -> Result { - if let Some(purity) = function.dfg.purity_of(function.id()) { + if let Some(purity) = purities.get(&function.id()).copied() { writeln!(f, "{} {purity} fn {} {} {{", function.runtime(), function.name(), function.id())?; } else { writeln!(f, "{} fn {} {} {{", function.runtime(), function.name(), function.id())?; diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index bdf45eb19c1..73bd636fd95 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -41,7 +41,7 @@ use crate::{ types::{NumericType, Type}, value::{Value, ValueId, ValueMapping}, }, - opt::pure::Purity, + opt::pure::{FunctionPurities, Purity}, ssa_gen::Ssa, }, }; @@ -57,7 +57,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants(mut self) -> Ssa { for function in self.functions.values_mut() { - function.constant_fold(false, None); + function.constant_fold(false, None, &self.function_purities); } self } @@ -70,7 +70,7 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn fold_constants_using_constraints(mut self) -> Ssa { for function in self.functions.values_mut() { - function.constant_fold(true, None); + function.constant_fold(true, None, &self.function_purities); } self } @@ -96,7 +96,7 @@ impl Ssa { if function.dfg.runtime().is_brillig() { continue; } - function.constant_fold(false, brillig_info); + function.constant_fold(false, brillig_info, &self.function_purities); } self @@ -106,12 +106,13 @@ impl Ssa { impl Function { /// The structure of this pass is simple: /// Go through each block and re-insert all instructions. - pub(crate) fn constant_fold( + fn constant_fold( &mut self, use_constraint_info: bool, brillig_info: Option, + purities: &FunctionPurities, ) { - let mut context = Context::new(use_constraint_info, brillig_info); + let mut context = Context::new(use_constraint_info, brillig_info, purities); let mut dom = DominatorTree::with_function(self); context.block_queue.push_back(self.entry_block()); @@ -127,6 +128,7 @@ impl Function { } struct Context<'a> { + purities: &'a FunctionPurities, use_constraint_info: bool, brillig_info: Option>, /// Maps pre-folded ValueIds to the new ValueIds obtained by re-inserting the instruction. @@ -216,9 +218,14 @@ struct ResultCache { result: Option<(BasicBlockId, Vec)>, } -impl<'brillig> Context<'brillig> { - fn new(use_constraint_info: bool, brillig_info: Option>) -> Self { +impl<'a> Context<'a> { + fn new( + use_constraint_info: bool, + brillig_info: Option>, + purities: &'a FunctionPurities, + ) -> Self { Self { + purities, use_constraint_info, brillig_info, visited_blocks: Default::default(), @@ -508,13 +515,13 @@ impl<'brillig> Context<'brillig> { // we cache the results so we can reuse them if the same instruction appears again later in the block. // Others have side effects representing failure, which are implicit in the ACIR code and can also be deduplicated. let can_be_deduplicated = - can_be_deduplicated(&instruction, function, self.use_constraint_info); + can_be_deduplicated(&instruction, function, self.use_constraint_info, self.purities); // We also allow deduplicating MakeArray instructions that we have tracked which haven't // been mutated. if can_be_deduplicated || matches!(instruction, Instruction::MakeArray { .. }) { - let use_predicate = - self.use_constraint_info && instruction.requires_acir_gen_predicate(&function.dfg); + let use_predicate = self.use_constraint_info + && instruction.requires_acir_gen_predicate(&function.dfg, self.purities); let predicate = use_predicate.then_some(side_effects_enabled_var); // If we see this make_array again, we can reuse the current result. @@ -553,10 +560,12 @@ impl<'brillig> Context<'brillig> { block: BasicBlockId, ) -> Option { let results_for_instruction = self.cached_instruction_results.get(instruction)?; - let predicate = self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg); + let predicate = + self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg, self.purities); let predicate = predicate.then_some(side_effects_enabled_var); - results_for_instruction.get(&predicate)?.get(block, dom, instruction.has_side_effects(dfg)) + let results = results_for_instruction.get(&predicate)?; + results.get(block, dom, instruction.has_side_effects(dfg, self.purities)) } /// Checks if the given instruction is a call to a brillig function with all constant arguments. @@ -778,7 +787,7 @@ impl<'brillig> Context<'brillig> { Call { arguments, func } if function.runtime().is_brillig() => { // If we pass a value to a function, it might get modified, making it unsafe for reuse after the call. let Value::Function(func_id) = &function.dfg[*func] else { return }; - if matches!(function.dfg.purity_of(*func_id), None | Some(Purity::Impure)) { + if matches!(self.purities.get(func_id).copied(), None | Some(Purity::Impure)) { // Arrays passed to functions might be mutated by them if there are no `inc_rc` instructions // placed *before* the call to protect them. Currently we don't track the ref count in this // context, so be conservative and do not reuse any array shared with a callee. @@ -909,6 +918,7 @@ pub(crate) fn can_be_deduplicated( instruction: &Instruction, function: &Function, deduplicate_with_predicate: bool, + purities: &FunctionPurities, ) -> bool { use Instruction::*; @@ -924,7 +934,7 @@ pub(crate) fn can_be_deduplicated( Call { func, .. } => { let purity = match function.dfg[*func] { Value::Intrinsic(intrinsic) => Some(intrinsic.purity()), - Value::Function(id) => function.dfg.purity_of(id), + Value::Function(id) => purities.get(&id).copied(), _ => None, }; match purity { @@ -956,7 +966,8 @@ pub(crate) fn can_be_deduplicated( // with one that was disabled. See // https://github.com/noir-lang/noir/pull/4716#issuecomment-2047846328. Binary(_) | ArrayGet { .. } | ArraySet { .. } => { - deduplicate_with_predicate || !instruction.requires_acir_gen_predicate(&function.dfg) + deduplicate_with_predicate + || !instruction.requires_acir_gen_predicate(&function.dfg, purities) } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs index dabc3a0a5af..69823f18a68 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -98,7 +98,7 @@ impl Ssa { let variants = find_variants(&self); // Generate the apply functions for the provided variants - let (apply_functions, purities) = create_apply_functions(&mut self, variants); + let apply_functions = create_apply_functions(&mut self, variants); // Setup the pass context let context = DefunctionalizationContext { apply_functions }; @@ -106,11 +106,6 @@ impl Ssa { // Run defunctionalization over all functions in the SSA context.defunctionalize_all(&mut self); - let purities = Arc::new(purities); - for function in self.functions.values_mut() { - function.dfg.set_function_purities(purities.clone()); - } - // Check that we have established the properties expected from this pass. #[cfg(debug_assertions)] self.functions.values().for_each(defunctionalize_post_check); @@ -421,21 +416,11 @@ fn find_dynamic_dispatches(func: &Function) -> BTreeSet { /// - `variants_map`: [Variants] /// /// # Returns -/// - [ApplyFunctions] keyed by each function's signature _before_ functions are changed -/// into field types. The inner apply function itself will have its defunctionalized type, -/// with function values represented as field values. -/// - [HashMap] with purities that must be set to all functions in the SSA, -/// as this function might have created dummy pure functions. -fn create_apply_functions( - ssa: &mut Ssa, - variants_map: Variants, -) -> (ApplyFunctions, HashMap) { +/// [ApplyFunctions] keyed by each function's signature _before_ functions are changed +/// into field types. The inner apply function itself will have its defunctionalized type, +/// with function values represented as field values. +fn create_apply_functions(ssa: &mut Ssa, variants_map: Variants) -> ApplyFunctions { let mut apply_functions = HashMap::default(); - let mut purities = if ssa.functions.is_empty() { - HashMap::default() - } else { - (*ssa.functions.iter().next().unwrap().1.dfg.function_purities).clone() - }; for ((signature, runtime), variants) in variants_map.into_iter() { let dispatches_to_multiple_functions = variants.len() > 1; @@ -467,13 +452,13 @@ fn create_apply_functions( // If no variants exist for a dynamic call we leave removing those dead calls and parameters to DIE. // However, we have to construct a dummy function for these dead calls as to keep a well formed SSA // and to not break the semantics of other SSA passes before DIE is reached. - create_dummy_function(ssa, defunctionalized_signature, runtime, &mut purities) + create_dummy_function(ssa, defunctionalized_signature, runtime) }; apply_functions .insert((signature, runtime), ApplyFunction { id, dispatches_to_multiple_functions }); } - (apply_functions, purities) + apply_functions } /// Transforms a [FunctionId] into a [FieldElement] @@ -651,9 +636,8 @@ fn create_dummy_function( ssa: &mut Ssa, signature: Signature, caller_runtime: RuntimeType, - purities: &mut HashMap, ) -> FunctionId { - ssa.add_fn(|id| { + let id = ssa.add_fn(|id| { let mut function_builder = FunctionBuilder::new("apply_dummy".to_string(), id); // Set the runtime of the dummy function. The dummy function is expect to always be simplified out @@ -669,19 +653,21 @@ fn create_dummy_function( // was set to be inlined before the call to it was removed by DIE. vecmap(signature.params, |typ| function_builder.add_parameter(typ)); - // We can mark the dummy function pure as all it does is return. - // As the dummy function is just meant to be a placeholder for any calls to - // higher-order functions without variants, we want the function to be marked pure - // so that dead instruction elimination can remove any calls to it. - purities.insert(id, Purity::Pure); - let results = vecmap(signature.returns, |typ| make_dummy_return_data(&mut function_builder, &typ)); function_builder.terminate_with_return(results); function_builder.current_function - }) + }); + + // We can mark the dummy function pure as all it does is return. + // As the dummy function is just meant to be a placeholder for any calls to + // higher-order functions without variants, we want the function to be marked pure + // so that dead instruction elimination can remove any calls to it. + ssa.function_purities.insert(id, Purity::Pure); + + id } /// Construct a dummy value to be returned from the placeholder function for calls to invalid lambda references. @@ -1652,7 +1638,7 @@ mod tests { let variants = find_variants(&ssa); assert_eq!(variants.len(), 2); - let (apply_functions, _purities) = create_apply_functions(&mut ssa, variants); + let apply_functions = create_apply_functions(&mut ssa, variants); // This was 1 before this bug was fixed. assert_eq!(apply_functions.len(), 2); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index fe4694e4034..06b8b8f0a23 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -18,7 +18,7 @@ use crate::ssa::{ types::{NumericType, Type}, value::{Value, ValueId}, }, - opt::pure::Purity, + opt::pure::{FunctionPurities, Purity}, ssa_gen::Ssa, }; @@ -93,8 +93,12 @@ impl Ssa { .functions .par_iter_mut() .map(|(id, func)| { - let unused_params = - func.dead_instruction_elimination(true, flattened, skip_brillig); + let unused_params = func.dead_instruction_elimination( + true, + flattened, + skip_brillig, + &self.function_purities, + ); let mut result = DIEResult::default(); result.unused_parameters.insert(*id, unused_params); @@ -146,12 +150,13 @@ impl Function { insert_out_of_bounds_checks: bool, flattened: bool, skip_brillig: bool, + purities: &FunctionPurities, ) -> HashMap> { if skip_brillig && self.dfg.runtime().is_brillig() { return HashMap::default(); } - let mut context = Context { flattened, ..Default::default() }; + let mut context = Context::new(flattened, purities); context.mark_function_parameter_arrays_as_used(self); @@ -190,7 +195,7 @@ impl Function { // instructions (we don't want to remove those checks, or instructions that are // dependencies of those checks) if inserted_out_of_bounds_checks { - return self.dead_instruction_elimination(false, flattened, skip_brillig); + return self.dead_instruction_elimination(false, flattened, skip_brillig, purities); } context.remove_rc_instructions(&mut self.dfg); @@ -204,8 +209,8 @@ struct DIEResult { unused_parameters: HashMap>>, } /// Per function context for tracking unused values and which instructions to remove. -#[derive(Default)] -struct Context { +struct Context<'purities> { + purities: &'purities FunctionPurities, used_values: HashSet, instructions_to_remove: HashSet, @@ -234,7 +239,19 @@ struct Context { parameter_keep_list: HashMap>, } -impl Context { +impl<'purities> Context<'purities> { + fn new(flattened: bool, purities: &'purities FunctionPurities) -> Self { + Context { + purities, + flattened, + used_values: Default::default(), + instructions_to_remove: Default::default(), + rc_instructions: Default::default(), + rc_tracker: Default::default(), + parameter_keep_list: Default::default(), + } + } + /// Steps backwards through the instruction of the given block, amassing a set of used values /// as it goes, and at the same time marking instructions for removal if they haven't appeared /// in the set thus far. @@ -345,7 +362,7 @@ impl Context { fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { let instruction = &function.dfg[instruction_id]; - if can_be_eliminated_if_unused(instruction, function, self.flattened) { + if can_be_eliminated_if_unused(instruction, function, self.flattened, self.purities) { let results = function.dfg.instruction_results(instruction_id); results.iter().all(|result| !self.used_values.contains(result)) } else if let Instruction::Call { func, arguments } = instruction { @@ -586,6 +603,7 @@ fn can_be_eliminated_if_unused( instruction: &Instruction, function: &Function, flattened: bool, + purities: &FunctionPurities, ) -> bool { use Instruction::*; match instruction { @@ -598,7 +616,7 @@ fn can_be_eliminated_if_unused( } } else { // Checked binary operations can have different behavior depending on the predicate. - !instruction.requires_acir_gen_predicate(&function.dfg) + !instruction.requires_acir_gen_predicate(&function.dfg, purities) } } @@ -641,7 +659,7 @@ fn can_be_eliminated_if_unused( // We use purity to determine whether functions contain side effects. // If we have an impure function, we cannot remove it even if it is unused. - Value::Function(function_id) => match function.dfg.purity_of(function_id) { + Value::Function(function_id) => match purities.get(&function_id).copied() { Some(Purity::Pure) => true, Some(Purity::PureWithPredicate) => false, Some(Purity::Impure) => false, diff --git a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs index d482d73458f..601834d3d54 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/loop_invariant.rs @@ -62,7 +62,7 @@ use crate::ssa::{ types::{NumericType, Type}, value::{Value, ValueId}, }, - opt::pure::Purity, + opt::pure::{FunctionPurities, Purity}, }; use acvm::{FieldElement, acir::AcirField}; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; @@ -74,22 +74,21 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn loop_invariant_code_motion(mut self) -> Ssa { for function in self.functions.values_mut() { - function.loop_invariant_code_motion(); + function.loop_invariant_code_motion(&self.function_purities); } - self } } impl Function { - pub(super) fn loop_invariant_code_motion(&mut self) { - Loops::find_all(self).hoist_loop_invariants(self); + pub(super) fn loop_invariant_code_motion(&mut self, purities: &FunctionPurities) { + Loops::find_all(self).hoist_loop_invariants(self, purities); } } impl Loops { - fn hoist_loop_invariants(mut self, function: &mut Function) { - let mut context = LoopInvariantContext::new(function); + fn hoist_loop_invariants(mut self, function: &mut Function, purities: &FunctionPurities) { + let mut context = LoopInvariantContext::new(function, purities); // Insert all loop bounds up front, so we can inspect both outer and nested loops. for loop_ in &self.yet_to_unroll { @@ -151,7 +150,7 @@ impl Loop { } } -struct LoopInvariantContext<'f> { +struct LoopInvariantContext<'f, 'purities> { inserter: FunctionInserter<'f>, defined_in_loop: HashSet, loop_invariants: HashSet, @@ -201,10 +200,12 @@ struct LoopInvariantContext<'f> { // Helper constants true_value: ValueId, false_value: ValueId, + + purities: &'purities FunctionPurities, } -impl<'f> LoopInvariantContext<'f> { - fn new(function: &'f mut Function) -> Self { +impl<'f, 'purities> LoopInvariantContext<'f, 'purities> { + fn new(function: &'f mut Function, purities: &'purities FunctionPurities) -> Self { let cfg = ControlFlowGraph::with_function(function); let reversed_cfg = ControlFlowGraph::extended_reverse(function); let post_order = PostOrder::with_cfg(&reversed_cfg); @@ -231,6 +232,7 @@ impl<'f> LoopInvariantContext<'f> { true_value, false_value, no_break: false, + purities, } } @@ -282,7 +284,8 @@ impl<'f> LoopInvariantContext<'f> { // If the block has already been labelled as impure, we do need to check the current // instruction's side effects. if !self.current_block_impure { - self.current_block_impure = dfg[instruction_id].has_side_effects(dfg); + self.current_block_impure = + dfg[instruction_id].has_side_effects(dfg, self.purities); } self.inserter.push_instruction(instruction_id, *block); } @@ -324,7 +327,7 @@ impl<'f> LoopInvariantContext<'f> { dfg[*block] .instructions() .iter() - .any(|instruction| dfg[*instruction].has_side_effects(dfg)) + .any(|instruction| dfg[*instruction].has_side_effects(dfg, self.purities)) }); // Reset the current block control dependent flag, the check will set it to true if needed. @@ -521,7 +524,7 @@ impl<'f> LoopInvariantContext<'f> { } matches!(instruction, MakeArray { .. }) - || can_be_hoisted(&instruction, self.inserter.function, false) + || can_be_hoisted(&instruction, self.inserter.function, false, self.purities) || self.can_be_hoisted_with_control_dependence(&instruction, self.inserter.function) || self.can_be_hoisted_from_loop_bounds(&instruction) } @@ -533,7 +536,7 @@ impl<'f> LoopInvariantContext<'f> { instruction: &Instruction, function: &Function, ) -> bool { - can_be_hoisted(instruction, function, true) + can_be_hoisted(instruction, function, true, self.purities) && self.can_hoist_control_dependent_instruction() } @@ -610,7 +613,7 @@ impl<'f> LoopInvariantContext<'f> { Call { func, .. } => { let purity = match self.inserter.function.dfg[*func] { Value::Intrinsic(intrinsic) => Some(intrinsic.purity()), - Value::Function(id) => self.inserter.function.dfg.purity_of(id), + Value::Function(id) => self.purities.get(&id).copied(), _ => None, }; matches!(purity, Some(Purity::PureWithPredicate)) @@ -1054,6 +1057,7 @@ fn can_be_hoisted( instruction: &Instruction, function: &Function, hoist_with_predicate: bool, + purities: &FunctionPurities, ) -> bool { use Instruction::*; @@ -1069,7 +1073,7 @@ fn can_be_hoisted( Call { func, .. } => { let purity = match function.dfg[*func] { Value::Intrinsic(intrinsic) => Some(intrinsic.purity()), - Value::Function(id) => function.dfg.purity_of(id), + Value::Function(id) => purities.get(&id).copied(), _ => None, }; match purity { @@ -1100,7 +1104,8 @@ fn can_be_hoisted( // These can have different behavior depending on the predicate. Binary(_) | ArrayGet { .. } | ArraySet { .. } => { - hoist_with_predicate || !instruction.requires_acir_gen_predicate(&function.dfg) + hoist_with_predicate + || !instruction.requires_acir_gen_predicate(&function.dfg, purities) } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs index 953795a9649..92a9426ffc5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeMap, sync::Arc}; +use std::collections::BTreeMap; use crate::ssa::{ ir::{ @@ -8,6 +8,7 @@ use crate::ssa::{ post_order::PostOrder, value::{Value, ValueId}, }, + opt::pure::FunctionPurities, ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; @@ -22,20 +23,26 @@ impl Ssa { /// may increase the ID counter so that later passes start at different offsets, /// even if they contain the same SSA code. pub fn normalize_ids(&mut self) { - let mut context = Context::default(); + let mut context = Context { + old_purities: std::mem::take(&mut self.function_purities), + ..Context::default() + }; + context.populate_functions(&self.functions); for function in self.functions.values_mut() { context.normalize_ids(function); } self.functions = context.functions.into_btree(); + self.function_purities = context.new_purities; } } #[derive(Default)] struct Context { functions: SparseMap, - new_ids: IdMaps, + old_purities: FunctionPurities, + new_purities: FunctionPurities, } /// Maps from old ids to new ones. @@ -57,28 +64,17 @@ struct IdMaps { impl Context { fn populate_functions(&mut self, functions: &BTreeMap) { - let Some(old_purities) = &functions.iter().next().map(|f| &f.1.dfg.function_purities) - else { - return; - }; - let mut new_purities = HashMap::default(); - for (id, function) in functions { self.functions.insert_with_id(|new_id| { self.new_ids.function_ids.insert(*id, new_id); - if let Some(purity) = old_purities.get(id) { - new_purities.insert(new_id, *purity); + if let Some(purity) = self.old_purities.get(id) { + self.new_purities.insert(new_id, *purity); } Function::clone_signature(new_id, function) }); } - - let new_purities = Arc::new(new_purities); - for new_id in self.new_ids.function_ids.values() { - self.functions[*new_id].dfg.set_function_purities(new_purities.clone()); - } } fn normalize_ids(&mut self, old_function: &mut Function) { diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index 1ebcf787598..3b5aa8aa29c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -57,7 +57,7 @@ impl Ssa { // Help unrolling determine bounds. function.as_slice_optimization(); // Prepare for unrolling - function.loop_invariant_code_motion(); + function.loop_invariant_code_motion(&self.function_purities); // We might not be able to unroll all loops without fully inlining them, so ignore errors. let _ = function.unroll_loops_iteratively(); // Reduce the number of redundant stores/loads after unrolling diff --git a/compiler/noirc_evaluator/src/ssa/opt/pure.rs b/compiler/noirc_evaluator/src/ssa/opt/pure.rs index 16ef3832e72..242b7e2ae95 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/pure.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/pure.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use fxhash::FxHashMap as HashMap; use petgraph::visit::DfsPostOrder; @@ -29,19 +27,18 @@ impl Ssa { pub(crate) fn purity_analysis(mut self) -> Ssa { // First look through each function to get a baseline on its purity and collect // the functions it calls to build a call graph. - let purities: HashMap<_, _> = - self.functions.values().map(|function| (function.id(), function.is_pure())).collect(); + let functions = self.functions.values(); + let purities: FunctionPurities = functions + .map(|function| (function.id(), function.is_pure(&self.function_purities))) + .collect(); // Then transitively 'infect' any functions which call impure functions as also // impure. let call_graph = CallGraph::from_ssa(&self); let purities = analyze_call_graph(call_graph, purities); - let purities = Arc::new(purities); - // We're done, now store purities somewhere every dfg can find it. - for function in self.functions.values_mut() { - function.dfg.set_function_purities(purities.clone()); - } + // We're done, now store purities + self.function_purities = purities; #[cfg(debug_assertions)] purity_analysis_post_check(&self); @@ -58,9 +55,7 @@ impl Ssa { /// Otherwise panics. #[cfg(debug_assertions)] fn purity_analysis_post_check(ssa: &Ssa) { - if let Some((id, _)) = - ssa.functions.iter().find(|(id, function)| function.dfg.purity_of(**id).is_none()) - { + if let Some(id) = ssa.functions.keys().find(|id| !ssa.function_purities.contains_key(*id)) { panic!("Function {id} does not have a purity status") } } @@ -111,7 +106,7 @@ impl std::fmt::Display for Purity { } impl Function { - fn is_pure(&self) -> Purity { + fn is_pure(&self, purities: &FunctionPurities) -> Purity { let contains_reference = |value_id: &ValueId| { let typ = self.dfg.type_of_value(*value_id); typ.contains_reference() @@ -150,7 +145,7 @@ impl Function { ins @ (Instruction::Binary(_) | Instruction::ArrayGet { .. } | Instruction::ArraySet { .. }) => { - if ins.requires_acir_gen_predicate(&self.dfg) { + if ins.requires_acir_gen_predicate(&self.dfg, purities) { result = Purity::PureWithPredicate; } } @@ -347,7 +342,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); let ssa = ssa.purity_analysis(); - let purities = &ssa.main().dfg.function_purities; + let purities = &ssa.function_purities; assert_eq!(purities[&FunctionId::test_new(0)], Purity::Impure); assert_eq!(purities[&FunctionId::test_new(1)], Purity::Impure); assert_eq!(purities[&FunctionId::test_new(2)], Purity::Impure); @@ -452,7 +447,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); let ssa = ssa.purity_analysis(); - let purities = &ssa.main().dfg.function_purities; + let purities = &ssa.function_purities; assert_eq!(purities[&FunctionId::test_new(0)], Purity::Impure); assert_eq!(purities[&FunctionId::test_new(1)], Purity::Impure); assert_eq!(purities[&FunctionId::test_new(2)], Purity::Impure); @@ -475,7 +470,7 @@ mod test { let ssa = Ssa::from_str(src).unwrap(); let ssa = ssa.purity_analysis(); - let purities = &ssa.main().dfg.function_purities; + let purities = &ssa.function_purities; assert_eq!(purities[&FunctionId::test_new(0)], Purity::PureWithPredicate); assert_eq!(purities[&FunctionId::test_new(1)], Purity::PureWithPredicate); } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index 25f063fcd94..77712ab9a12 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -18,6 +18,7 @@ use crate::ssa::{ instruction::Instruction, types::NumericType, }, + opt::pure::FunctionPurities, ssa_gen::Ssa, }; @@ -26,14 +27,14 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn remove_enable_side_effects(mut self) -> Ssa { for function in self.functions.values_mut() { - function.remove_enable_side_effects(); + function.remove_enable_side_effects(&self.function_purities); } self } } impl Function { - pub(crate) fn remove_enable_side_effects(&mut self) { + pub(crate) fn remove_enable_side_effects(&mut self, purities: &FunctionPurities) { if matches!(self.runtime(), RuntimeType::Brillig(_)) { // Brillig functions do not make use of the `EnableSideEffects` instruction so are unaffected by this pass. return; @@ -88,7 +89,7 @@ impl Function { // If we hit an instruction which is affected by the side effects var then we must insert the // `Instruction::EnableSideEffectsIf` before we insert this new instruction. - if instruction.requires_acir_gen_predicate(context.dfg) { + if instruction.requires_acir_gen_predicate(context.dfg, purities) { if let Some(enable_side_effects_instruction_id) = last_side_effects_enabled_instruction.take() { diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs index 18e22b43466..d6a0523f23e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable_instructions.rs @@ -23,14 +23,14 @@ use crate::ssa::{ types::{NumericType, Type}, value::ValueId, }, - opt::simple_optimization::SimpleOptimizationContext, + opt::{pure::FunctionPurities, simple_optimization::SimpleOptimizationContext}, ssa_gen::Ssa, }; impl Ssa { pub(crate) fn remove_unreachable_instructions(mut self) -> Ssa { for function in self.functions.values_mut() { - function.remove_unreachable_instructions(); + function.remove_unreachable_instructions(&self.function_purities); } self } @@ -48,7 +48,7 @@ enum Reachability { } impl Function { - fn remove_unreachable_instructions(&mut self) { + fn remove_unreachable_instructions(&mut self, purities: &FunctionPurities) { let func_id = self.id(); // The current block we are currently processing let mut current_block_id = None; @@ -86,7 +86,7 @@ impl Function { if current_block_reachability == Reachability::UnreachableUnderPredicate { // Instructions that don't interact with the predicate should be left alone, // because the `remove_enable_side_effects` pass might have moved the boundaries around them. - if !instruction.requires_acir_gen_predicate(context.dfg) { + if !instruction.requires_acir_gen_predicate(context.dfg, purities) { return; } // Remove the current instruction and insert defaults for the results. diff --git a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index bfe81b6feef..6623bd27df8 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -58,7 +58,7 @@ struct Translator { globals_graph: Arc, error_selector_counter: u64, - purities: Arc, + purities: FunctionPurities, } impl Translator { @@ -119,9 +119,6 @@ impl Translator { // Does not matter what ID we use here. let globals = Function::new("globals".to_owned(), main_id); - let purities = Arc::new(purities); - builder.set_purities(purities.clone()); - let mut translator = Self { builder, functions, @@ -159,7 +156,6 @@ impl Translator { } } - self.builder.set_purities(self.purities.clone()); self.translate_function_body(function) } @@ -574,6 +570,7 @@ impl Translator { fn finish(self) -> Ssa { let mut ssa = self.builder.finish(); + ssa.function_purities = self.purities; // Normalize the IDs so we have a better chance of matching the SSA we parsed // after the step-by-step reconstruction done during translation. This assumes diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 3c041743c42..9c269729ac1 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -6,10 +6,13 @@ use iter_extended::btree_map; use serde::{Deserialize, Serialize}; use serde_with::serde_as; -use crate::ssa::ir::{ - function::{Function, FunctionId}, - map::AtomicCounter, - value::Value, +use crate::ssa::{ + ir::{ + function::{Function, FunctionId}, + map::AtomicCounter, + value::Value, + }, + opt::pure::FunctionPurities, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -34,6 +37,8 @@ pub struct Ssa { // ABI not the actual SSA IR. #[serde(skip)] pub error_selector_to_type: BTreeMap, + #[serde(skip)] + pub function_purities: FunctionPurities, } impl Ssa { @@ -54,6 +59,7 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, + function_purities: FunctionPurities::default(), } }