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 90b23f7cbb7..130b7450de1 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -324,11 +324,7 @@ mod tests { "; 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(&BrilligOptions::default(), used_globals_map); + let brillig = ssa.to_brillig(&BrilligOptions::default()); assert_eq!( brillig.globals.len(), @@ -443,11 +439,8 @@ mod tests { let ssa = Ssa::from_str(src).unwrap(); // Need to run SSA pass that sets up Brillig array gets let ssa = ssa.brillig_array_get_and_set(); - // 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(&BrilligOptions::default(), used_globals_map); + let brillig = ssa.to_brillig(&BrilligOptions::default()); assert_eq!( brillig.globals.len(), @@ -527,8 +520,6 @@ mod tests { "; 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 { @@ -557,8 +548,7 @@ mod tests { } } - let used_globals_map = std::mem::take(&mut ssa.used_globals); - let brillig = ssa.to_brillig_with_globals(&BrilligOptions::default(), used_globals_map); + let brillig = ssa.to_brillig(&BrilligOptions::default()); assert_eq!(brillig.globals.len(), 1, "Should have a single entry point"); for (func_id, artifact) in brillig.globals { @@ -617,11 +607,8 @@ mod tests { "; 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(&BrilligOptions::default(), used_globals_map); + let brillig = ssa.to_brillig(&BrilligOptions::default()); assert_eq!( brillig.globals.len(), diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 9b9bfb648df..a1df3e4093d 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -23,7 +23,7 @@ use crate::ssa::{ }, ssa_gen::Ssa, }; -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use fxhash::FxHashMap as HashMap; use std::{borrow::Cow, collections::BTreeSet}; pub use self::brillig_ir::procedures::ProcedureId; @@ -129,18 +129,11 @@ impl std::ops::Index for Brillig { } impl Ssa { + /// Compile Brillig functions and ACIR functions reachable from them #[tracing::instrument(level = "trace", skip_all)] pub fn to_brillig(&self, options: &BrilligOptions) -> Brillig { - self.to_brillig_with_globals(options, HashMap::default()) - } + let used_globals_map = self.used_globals_in_brillig_functions(); - /// Compile Brillig functions and ACIR functions reachable from them - #[tracing::instrument(level = "trace", skip_all)] - pub(crate) fn to_brillig_with_globals( - &self, - options: &BrilligOptions, - used_globals_map: HashMap>, - ) -> Brillig { // Collect all the function ids that are reachable from brillig // That means all the functions marked as brillig and ACIR functions called by them let brillig_reachable_function_ids = self diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index edaa6c5602a..850741e861d 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -230,8 +230,6 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec { .and_then(Ssa::remove_unreachable_functions), SsaPass::new(Ssa::dead_instruction_elimination, "Dead Instruction Elimination"), SsaPass::new(Ssa::array_set_optimization, "Array Set Optimizations"), - // The Brillig globals pass expected that we have the used globals map set for each function. - // The used globals map is determined during DIE, so we should duplicate entry points before a DIE pass run. SsaPass::new(Ssa::brillig_entry_point_analysis, "Brillig Entry Point Analysis") // Remove any potentially unnecessary duplication from the Brillig entry point analysis. .and_then(Ssa::remove_unreachable_functions), @@ -239,13 +237,7 @@ pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec { // This pass makes transformations specific to Brillig generation. // It must be the last pass to either alter or add new instructions before Brillig generation, // as other semantics in the compiler can potentially break (e.g. inserting instructions). - // We can safely place the pass before DIE as that pass only removes instructions. - // We also need DIE's tracking of used globals in case the array get transformations - // end up using an existing constant from the globals space. - // This pass might result in otherwise unused global constant becoming used, - // because the creation of shifted index constants can reuse their IDs. SsaPass::new(Ssa::brillig_array_get_and_set, "Brillig Array Get and Set Optimizations"), - // Perform another DIE pass to update the used globals after offsetting Brillig indexes. SsaPass::new(Ssa::dead_instruction_elimination, "Dead Instruction Elimination") // A function can be potentially unreachable post-DIE if all calls to that function were removed. .and_then(Ssa::remove_unreachable_functions), @@ -291,8 +283,6 @@ pub fn minimal_passes() -> Vec> { // This can change which globals are used, because constant creation might result // in the (re)use of otherwise unused global values. SsaPass::new(Ssa::brillig_array_get_and_set, "Brillig Array Get and Set Optimizations"), - // We need a DIE pass to populate `used_globals`, otherwise it will panic later. - SsaPass::new(Ssa::dead_instruction_elimination, "Dead Instruction Elimination"), ] } @@ -323,14 +313,13 @@ where let mut builder = builder.with_skip_passes(options.skip_passes.clone()).run_passes(primary)?; let passed = std::mem::take(&mut builder.passed); let files = builder.files; - let mut ssa = builder.finish(); + let ssa = builder.finish(); let mut ssa_level_warnings = vec![]; drop(ssa_gen_span_guard); - let used_globals_map = std::mem::take(&mut ssa.used_globals); let brillig = time("SSA to Brillig", options.print_codegen_timings, || { - ssa.to_brillig_with_globals(&options.brillig_options, used_globals_map) + ssa.to_brillig(&options.brillig_options) }); let ssa_gen_span = span!(Level::TRACE, "ssa_generation"); diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index f42b2e1d568..27217866a6d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1537,9 +1537,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - 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(&BrilligOptions::default(), used_globals_map); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let ssa = ssa.fold_constants_with_brillig(&brillig); let ssa = ssa.remove_unreachable_functions(); @@ -1577,9 +1575,7 @@ mod test { } "; let ssa = Ssa::from_str(src).unwrap(); - 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(&BrilligOptions::default(), used_globals_map); + let brillig = ssa.to_brillig(&BrilligOptions::default()); let ssa = ssa.fold_constants_with_brillig(&brillig); let ssa = ssa.remove_unreachable_functions(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 3147e2e81c9..68d8bb9028d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -96,45 +96,22 @@ impl Ssa { flattened: bool, skip_brillig: bool, ) -> (Ssa, DIEResult) { - let mut result = self + let result = self .functions .par_iter_mut() .map(|(id, func)| { - let (used_globals, unused_params) = - func.dead_instruction_elimination(flattened, skip_brillig); + let unused_params = func.dead_instruction_elimination(flattened, skip_brillig); let mut result = DIEResult::default(); - if func.runtime().is_brillig() { - result.used_globals.insert(*id, used_globals); - } result.unused_parameters.insert(*id, unused_params); result }) .reduce(DIEResult::default, |mut a, b| { - a.used_globals.extend(b.used_globals); a.unused_parameters.extend(b.unused_parameters); a }); - let globals = &self.functions[&self.main_id].dfg.globals; - for used_global_values in result.used_globals.values_mut() { - // DIE only tracks used instruction results, however, globals include constants. - // Back track globals for internal values which may be in use. - for (id, value) in globals.values_iter().rev() { - if used_global_values.contains(&id) { - if let Value::Instruction { instruction, .. } = &value { - let instruction = &globals[*instruction]; - instruction.for_each_value(|value_id| { - used_global_values.insert(value_id); - }); - } - } - } - } - - self.used_globals = std::mem::take(&mut result.used_globals); - (self, result) } } @@ -154,7 +131,6 @@ impl Function { /// of its instructions are needed elsewhere. /// /// # Returns - /// - The set of globals that were used in this function. /// After processing all functions, the union of these sets enables determining the unused globals. /// - A mapping of (block id -> unused parameters) for the given function. /// This can be used by follow-up passes to prune unused parameters from blocks. @@ -162,9 +138,9 @@ impl Function { &mut self, flattened: bool, skip_brillig: bool, - ) -> (HashSet, HashMap>) { + ) -> HashMap> { if skip_brillig && self.dfg.runtime().is_brillig() { - return (HashSet::default(), HashMap::default()); + return HashMap::default(); } let mut context = Context { flattened, ..Default::default() }; @@ -198,16 +174,12 @@ impl Function { context.remove_rc_instructions(&mut self.dfg); - ( - context.used_values.into_iter().filter(|value| self.dfg.is_global(*value)).collect(), - unused_params_per_block, - ) + unused_params_per_block } } #[derive(Default)] struct DIEResult { - used_globals: HashMap>, unused_parameters: HashMap>>, } /// Per function context for tracking unused values and which instructions to remove. diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 0cd8737880e..3c041743c42 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -9,6 +9,7 @@ use serde_with::serde_as; use crate::ssa::ir::{ function::{Function, FunctionId}, map::AtomicCounter, + value::Value, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -20,7 +21,6 @@ use super::ValueId; pub struct Ssa { #[serde_as(as = "Vec<(_, _)>")] pub functions: BTreeMap, - pub used_globals: HashMap>, pub main_id: FunctionId, #[serde(skip)] pub next_id: AtomicCounter, @@ -54,9 +54,6 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, - // This field is set only after running DIE and is utilized - // for optimizing implementation of globals post-SSA. - used_globals: HashMap::default(), } } @@ -101,6 +98,77 @@ impl Ssa { pub(crate) fn is_entry_point(&self, function: FunctionId) -> bool { function == self.main_id || self.functions[&function].runtime().is_entry_point() } + + pub(crate) fn used_globals_in_brillig_functions( + &self, + ) -> HashMap> { + fn add_value_to_globals_if_global( + function: &Function, + value_id: ValueId, + used_globals: &mut HashSet, + ) { + if !function.dfg.is_global(value_id) { + return; + } + + if !used_globals.insert(value_id) { + return; + } + + // If we found a new global, its value could be an instruction that points to other globals. + let globals = &function.dfg.globals; + if let Value::Instruction { instruction, .. } = globals[value_id] { + let instruction = &globals[instruction]; + instruction.for_each_value(|value_id| { + add_value_to_globals_if_global(function, value_id, used_globals); + }); + } + } + + let mut used_globals = HashMap::default(); + + for (function_id, function) in &self.functions { + if !function.runtime().is_brillig() { + continue; + } + + let mut used_globals_in_function = HashSet::default(); + + for call_data in &function.dfg.data_bus.call_data { + add_value_to_globals_if_global( + function, + call_data.array_id, + &mut used_globals_in_function, + ); + } + + for block_id in function.reachable_blocks() { + let block = &function.dfg[block_id]; + for instruction_id in block.instructions() { + let instruction = &function.dfg[*instruction_id]; + instruction.for_each_value(|value_id| { + add_value_to_globals_if_global( + function, + value_id, + &mut used_globals_in_function, + ); + }); + } + + block.unwrap_terminator().for_each_value(|value_id| { + add_value_to_globals_if_global( + function, + value_id, + &mut used_globals_in_function, + ); + }); + } + + used_globals.insert(*function_id, used_globals_in_function); + } + + used_globals + } } #[cfg(test)]