From 08732ed5367140040c94e099d8897e00936244f8 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 09:32:39 -0600 Subject: [PATCH 01/16] Separate brillig fns during monomorphization --- Cargo.lock | 1 + compiler/noirc_evaluator/src/ssa.rs | 1 - compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 - .../src/ssa/opt/runtime_separation.rs | 351 ------------------ compiler/noirc_frontend/Cargo.toml | 1 + .../src/monomorphization/mod.rs | 82 ++-- 6 files changed, 63 insertions(+), 374 deletions(-) delete mode 100644 compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs diff --git a/Cargo.lock b/Cargo.lock index f08467e0172..f71e0ceabbb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3200,6 +3200,7 @@ dependencies = [ "bn254_blackbox_solver", "cfg-if 1.0.0", "fm", + "fxhash", "im", "iter-extended", "noirc_arena", diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 9377cadb260..78724261f0d 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -157,7 +157,6 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result Self { - RuntimeSeparatorContext::separate_runtime(&mut self); - self - } -} - -#[derive(Debug, Default)] -struct RuntimeSeparatorContext { - // Original functions to clone to brillig - acir_functions_called_from_brillig: BTreeSet, - // Tracks the original => cloned version - mapped_functions: HashMap, -} - -impl RuntimeSeparatorContext { - pub(crate) fn separate_runtime(ssa: &mut Ssa) { - let mut runtime_separator = RuntimeSeparatorContext::default(); - - // We first collect all the acir functions called from a brillig context by exploring the SSA recursively - let mut processed_functions = HashSet::default(); - runtime_separator.collect_acir_functions_called_from_brillig( - ssa, - ssa.main_id, - false, - &mut processed_functions, - ); - - // Now we clone the relevant acir functions and change their runtime to brillig - runtime_separator.convert_acir_functions_called_from_brillig_to_brillig(ssa); - - // Now we update any calls within a brillig context to the mapped functions - runtime_separator.replace_calls_to_mapped_functions(ssa); - - // Some functions might be unreachable now (for example an acir function only called from brillig) - prune_unreachable_functions(ssa); - } - - fn collect_acir_functions_called_from_brillig( - &mut self, - ssa: &Ssa, - current_func_id: FunctionId, - mut within_brillig: bool, - processed_functions: &mut HashSet<(/* within_brillig */ bool, FunctionId)>, - ) { - // Processed functions needs the within brillig flag, since it is possible to call the same function from both brillig and acir - if processed_functions.contains(&(within_brillig, current_func_id)) { - return; - } - processed_functions.insert((within_brillig, current_func_id)); - - let func = &ssa.functions[¤t_func_id]; - if matches!(func.runtime(), RuntimeType::Brillig(_)) { - within_brillig = true; - } - - let called_functions = called_functions(func); - - if within_brillig { - for called_func_id in called_functions.iter() { - let called_func = &ssa.functions[called_func_id]; - if matches!(called_func.runtime(), RuntimeType::Acir(_)) { - self.acir_functions_called_from_brillig.insert(*called_func_id); - } - } - } - - for called_func_id in called_functions.into_iter() { - self.collect_acir_functions_called_from_brillig( - ssa, - called_func_id, - within_brillig, - processed_functions, - ); - } - } - - fn convert_acir_functions_called_from_brillig_to_brillig(&mut self, ssa: &mut Ssa) { - for acir_func_id in self.acir_functions_called_from_brillig.iter() { - let RuntimeType::Acir(inline_type) = ssa.functions[acir_func_id].runtime() else { - unreachable!("Function to transform to brillig should be ACIR") - }; - let cloned_id = ssa.clone_fn(*acir_func_id); - let new_func = - ssa.functions.get_mut(&cloned_id).expect("Cloned function should exist in SSA"); - new_func.set_runtime(RuntimeType::Brillig(inline_type)); - self.mapped_functions.insert(*acir_func_id, cloned_id); - } - } - - fn replace_calls_to_mapped_functions(&self, ssa: &mut Ssa) { - for (_function_id, func) in ssa.functions.iter_mut() { - if matches!(func.runtime(), RuntimeType::Brillig(_)) { - for called_func_value_id in called_functions_values(func).iter() { - let Value::Function(called_func_id) = &func.dfg[*called_func_value_id] else { - unreachable!("Value should be a function") - }; - if let Some(mapped_func_id) = self.mapped_functions.get(called_func_id) { - let mapped_value_id = func.dfg.import_function(*mapped_func_id); - func.dfg.set_value_from_id(*called_func_value_id, mapped_value_id); - } - } - } - } - } -} - -// We only consider direct calls to functions since functions as values should have been resolved -fn called_functions_values(func: &Function) -> BTreeSet { - let mut called_function_ids = BTreeSet::default(); - for block_id in func.reachable_blocks() { - for instruction_id in func.dfg[block_id].instructions() { - let Instruction::Call { func: called_value_id, .. } = &func.dfg[*instruction_id] else { - continue; - }; - - if let Value::Function(_) = func.dfg[*called_value_id] { - called_function_ids.insert(*called_value_id); - } - } - } - - called_function_ids -} - -fn called_functions(func: &Function) -> BTreeSet { - called_functions_values(func) - .into_iter() - .map(|value_id| { - let Value::Function(func_id) = func.dfg[value_id] else { - unreachable!("Value should be a function") - }; - func_id - }) - .collect() -} - -fn collect_reachable_functions( - ssa: &Ssa, - current_func_id: FunctionId, - reachable_functions: &mut HashSet, -) { - if reachable_functions.contains(¤t_func_id) { - return; - } - reachable_functions.insert(current_func_id); - - let func = &ssa.functions[¤t_func_id]; - let called_functions = called_functions(func); - - for called_func_id in called_functions.iter() { - collect_reachable_functions(ssa, *called_func_id, reachable_functions); - } -} - -fn prune_unreachable_functions(ssa: &mut Ssa) { - let mut reachable_functions = HashSet::default(); - collect_reachable_functions(ssa, ssa.main_id, &mut reachable_functions); - - ssa.functions.retain(|id, _value| reachable_functions.contains(id)); -} - -#[cfg(test)] -mod test { - use std::collections::BTreeSet; - - use noirc_frontend::monomorphization::ast::InlineType; - - use crate::ssa::{ - function_builder::FunctionBuilder, - ir::{ - function::{Function, FunctionId, RuntimeType}, - map::Id, - types::Type, - }, - opt::runtime_separation::called_functions, - ssa_gen::Ssa, - }; - - #[test] - fn basic_runtime_separation() { - // brillig fn foo { - // b0(): - // v0 = call bar() - // return v0 - // } - // acir fn bar { - // b0(): - // return 72 - // } - let foo_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("foo".into(), foo_id); - builder.current_function.set_runtime(RuntimeType::Brillig(InlineType::default())); - - let bar_id = Id::test_new(1); - let bar = builder.import_function(bar_id); - let results = builder.insert_call(bar, Vec::new(), vec![Type::field()]).to_vec(); - builder.terminate_with_return(results); - - builder.new_function("bar".into(), bar_id, InlineType::default()); - let expected_return = 72u128; - let seventy_two = builder.field_constant(expected_return); - builder.terminate_with_return(vec![seventy_two]); - - let ssa = builder.finish(); - assert_eq!(ssa.functions.len(), 2); - - // Expected result - // brillig fn foo { - // b0(): - // v0 = call bar() - // return v0 - // } - // brillig fn bar { - // b0(): - // return 72 - // } - let separated = ssa.separate_runtime(); - - // The original bar function must have been pruned - assert_eq!(separated.functions.len(), 2); - - // All functions should be brillig now - for func in separated.functions.values() { - assert_eq!(func.runtime(), RuntimeType::Brillig(InlineType::default())); - } - } - - fn find_func_by_name<'ssa>( - ssa: &'ssa Ssa, - funcs: &BTreeSet, - name: &str, - ) -> &'ssa Function { - funcs - .iter() - .find_map(|id| { - let func = ssa.functions.get(id).unwrap(); - if func.name() == name { - Some(func) - } else { - None - } - }) - .unwrap() - } - - #[test] - fn same_function_shared_acir_brillig() { - // acir fn foo { - // b0(): - // v0 = call bar() - // v1 = call baz() - // return v0, v1 - // } - // brillig fn bar { - // b0(): - // v0 = call baz() - // return v0 - // } - // acir fn baz { - // b0(): - // return 72 - // } - let foo_id = Id::test_new(0); - let mut builder = FunctionBuilder::new("foo".into(), foo_id); - - let bar_id = Id::test_new(1); - let baz_id = Id::test_new(2); - let bar = builder.import_function(bar_id); - let baz = builder.import_function(baz_id); - let v0 = builder.insert_call(bar, Vec::new(), vec![Type::field()]).to_vec(); - let v1 = builder.insert_call(baz, Vec::new(), vec![Type::field()]).to_vec(); - builder.terminate_with_return(vec![v0[0], v1[0]]); - - builder.new_brillig_function("bar".into(), bar_id, InlineType::default()); - let baz = builder.import_function(baz_id); - let v0 = builder.insert_call(baz, Vec::new(), vec![Type::field()]).to_vec(); - builder.terminate_with_return(v0); - - builder.new_function("baz".into(), baz_id, InlineType::default()); - let expected_return = 72u128; - let seventy_two = builder.field_constant(expected_return); - builder.terminate_with_return(vec![seventy_two]); - - let ssa = builder.finish(); - assert_eq!(ssa.functions.len(), 3); - - // Expected result - // acir fn foo { - // b0(): - // v0 = call bar() - // v1 = call baz() <- baz_acir - // return v0, v1 - // } - // brillig fn bar { - // b0(): - // v0 = call baz() <- baz_brillig - // return v0 - // } - // acir fn baz { - // b0(): - // return 72 - // } - // brillig fn baz { - // b0(): - // return 72 - // } - let separated = ssa.separate_runtime(); - - // The original baz function must have been duplicated - assert_eq!(separated.functions.len(), 4); - - let main_function = separated.functions.get(&separated.main_id).unwrap(); - assert_eq!(main_function.runtime(), RuntimeType::Acir(InlineType::Inline)); - - let main_calls = called_functions(main_function); - assert_eq!(main_calls.len(), 2); - - let bar = find_func_by_name(&separated, &main_calls, "bar"); - let baz_acir = find_func_by_name(&separated, &main_calls, "baz"); - - assert_eq!(baz_acir.runtime(), RuntimeType::Acir(InlineType::Inline)); - assert_eq!(bar.runtime(), RuntimeType::Brillig(InlineType::default())); - - let bar_calls = called_functions(bar); - assert_eq!(bar_calls.len(), 1); - - let baz_brillig = find_func_by_name(&separated, &bar_calls, "baz"); - assert_eq!(baz_brillig.runtime(), RuntimeType::Brillig(InlineType::default())); - } -} diff --git a/compiler/noirc_frontend/Cargo.toml b/compiler/noirc_frontend/Cargo.toml index 5f8f02689c8..041c1b1e015 100644 --- a/compiler/noirc_frontend/Cargo.toml +++ b/compiler/noirc_frontend/Cargo.toml @@ -31,6 +31,7 @@ petgraph = "0.6" rangemap = "1.4.0" strum.workspace = true strum_macros.workspace = true +fxhash.workspace = true [dev-dependencies] diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 8c07d71de21..d09257b0634 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -25,11 +25,12 @@ use crate::{ Kind, Type, TypeBinding, TypeBindings, }; use acvm::{acir::AcirField, FieldElement}; +use fxhash::FxHashMap as HashMap; use iter_extended::{btree_map, try_vecmap, vecmap}; use noirc_errors::Location; use noirc_printable_type::PrintableType; use std::{ - collections::{BTreeMap, HashMap, VecDeque}, + collections::{BTreeMap, VecDeque}, unreachable, }; @@ -56,14 +57,12 @@ struct LambdaContext { /// This struct holds the FIFO queue of functions to monomorphize, which is added to /// whenever a new (function, type) combination is encountered. struct Monomorphizer<'interner> { - /// Functions are keyed by their unique ID and expected type so that we can monomorphize - /// a new version of the function for each type. - /// We also key by any turbofish generics that are specified. - /// This is necessary for a case where we may have a trait generic that can be instantiated - /// outside of a function parameter or return value. + /// Functions are keyed by their unique ID, whether they're unconstrained, their expected type, + /// and any generics they have so that we can monomorphize a new version of the function for each type. /// - /// Using nested HashMaps here lets us avoid cloning HirTypes when calling .get() - functions: HashMap), FuncId>>, + /// Keying by any turbofish generics that are specified is necessary for a case where we may have a + /// trait generic that can be instantiated outside of a function parameter or return value. + functions: Functions, /// Unlike functions, locals are only keyed by their unique ID because they are never /// duplicated during monomorphization. Doing so would allow them to be used polymorphically @@ -72,8 +71,15 @@ struct Monomorphizer<'interner> { locals: HashMap, /// Queue of functions to monomorphize next each item in the queue is a tuple of: - /// (old_id, new_monomorphized_id, any type bindings to apply, the trait method if old_id is from a trait impl) - queue: VecDeque<(node_interner::FuncId, FuncId, TypeBindings, Option, Location)>, + /// (old_id, new_monomorphized_id, any type bindings to apply, the trait method if old_id is from a trait impl, is_unconstrained, location) + queue: VecDeque<( + node_interner::FuncId, + FuncId, + TypeBindings, + Option, + bool, + Location, + )>, /// When a function finishes being monomorphized, the monomorphized ast::Function is /// stored here along with its FuncId. @@ -92,8 +98,16 @@ struct Monomorphizer<'interner> { return_location: Option, debug_type_tracker: DebugTypeTracker, + + in_unconstrained_function: bool, } +/// Using nested HashMaps here lets us avoid cloning HirTypes when calling .get() +type Functions = HashMap< + (node_interner::FuncId, /*is_unconstrained:*/ bool), + HashMap, FuncId>>, +>; + type HirType = crate::Type; /// Starting from the given `main` function, monomorphize the entire program, @@ -125,10 +139,12 @@ pub fn monomorphize_debug( let function_sig = monomorphizer.compile_main(main)?; while !monomorphizer.queue.is_empty() { - let (next_fn_id, new_id, bindings, trait_method, location) = + let (next_fn_id, new_id, bindings, trait_method, is_unconstrained, location) = monomorphizer.queue.pop_front().unwrap(); monomorphizer.locals.clear(); + monomorphizer.in_unconstrained_function = is_unconstrained; + perform_instantiation_bindings(&bindings); let interner = &monomorphizer.interner; let impl_bindings = perform_impl_bindings(interner, trait_method, next_fn_id, location) @@ -172,8 +188,8 @@ pub fn monomorphize_debug( impl<'interner> Monomorphizer<'interner> { fn new(interner: &'interner mut NodeInterner, debug_type_tracker: DebugTypeTracker) -> Self { Monomorphizer { - functions: HashMap::new(), - locals: HashMap::new(), + functions: HashMap::default(), + locals: HashMap::default(), queue: VecDeque::new(), finished_functions: BTreeMap::new(), next_local_id: 0, @@ -183,6 +199,7 @@ impl<'interner> Monomorphizer<'interner> { is_range_loop: false, return_location: None, debug_type_tracker, + in_unconstrained_function: false, } } @@ -207,14 +224,18 @@ impl<'interner> Monomorphizer<'interner> { id: node_interner::FuncId, expr_id: node_interner::ExprId, typ: &HirType, - turbofish_generics: Vec, + turbofish_generics: &[HirType], trait_method: Option, ) -> Definition { let typ = typ.follow_bindings(); + let turbofish_generics = vecmap(turbofish_generics, |typ| typ.follow_bindings()); + let is_unconstrained = self.is_unconstrained(id); + match self .functions - .get(&id) - .and_then(|inner_map| inner_map.get(&(typ.clone(), turbofish_generics.clone()))) + .get(&(id, is_unconstrained)) + .and_then(|inner_map| inner_map.get(&typ)) + .and_then(|inner_map| inner_map.get(&turbofish_generics)) { Some(id) => Definition::Function(*id), None => { @@ -257,14 +278,21 @@ impl<'interner> Monomorphizer<'interner> { } /// Prerequisite: typ = typ.follow_bindings() + /// and: turbofish_generics = vecmap(turbofish_generics, Type::follow_bindings) fn define_function( &mut self, id: node_interner::FuncId, typ: HirType, turbofish_generics: Vec, + is_unconstrained: bool, new_id: FuncId, ) { - self.functions.entry(id).or_default().insert((typ, turbofish_generics), new_id); + self.functions + .entry((id, is_unconstrained)) + .or_default() + .entry(typ) + .or_default() + .insert(turbofish_generics, new_id); } fn compile_main( @@ -874,7 +902,7 @@ impl<'interner> Monomorphizer<'interner> { *func_id, expr_id, &typ, - generics.unwrap_or_default(), + &generics.unwrap_or_default(), None, ); let typ = Self::convert_type(&typ, ident.location)?; @@ -1281,7 +1309,7 @@ impl<'interner> Monomorphizer<'interner> { .map_err(MonomorphizationError::InterpreterError)?; let func_id = - match self.lookup_function(func_id, expr_id, &function_type, vec![], Some(method)) { + match self.lookup_function(func_id, expr_id, &function_type, &[], Some(method)) { Definition::Function(func_id) => func_id, _ => unreachable!(), }; @@ -1546,12 +1574,19 @@ impl<'interner> Monomorphizer<'interner> { trait_method: Option, ) -> FuncId { let new_id = self.next_function_id(); - self.define_function(id, function_type.clone(), turbofish_generics, new_id); + let is_unconstrained = self.is_unconstrained(id); + self.define_function( + id, + function_type.clone(), + turbofish_generics, + is_unconstrained, + new_id, + ); let location = self.interner.expr_location(&expr_id); let bindings = self.interner.get_instantiation_bindings(expr_id); let bindings = self.follow_bindings(bindings); - self.queue.push_back((id, new_id, bindings, trait_method, location)); + self.queue.push_back((id, new_id, bindings, trait_method, is_unconstrained, location)); new_id } @@ -2007,6 +2042,11 @@ impl<'interner> Monomorphizer<'interner> { Ok(ast::Expression::Call(ast::Call { func, arguments, return_type, location })) } + + fn is_unconstrained(&self, func_id: node_interner::FuncId) -> bool { + self.in_unconstrained_function + || self.interner.function_modifiers(&func_id).is_unconstrained + } } fn unwrap_tuple_type(typ: &HirType) -> Vec { From 094a8315b9790f0cfd72d34a64815990d480f453 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 09:44:53 -0600 Subject: [PATCH 02/16] Use new flag to set function unconstrained --- compiler/noirc_frontend/src/monomorphization/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index d09257b0634..9f37fbb38a8 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -303,6 +303,7 @@ impl<'interner> Monomorphizer<'interner> { assert_eq!(new_main_id, Program::main_id()); let location = self.interner.function_meta(&main_id).location; + self.in_unconstrained_function = self.is_unconstrained(main_id); self.function(main_id, new_main_id, location)?; self.return_location = @@ -352,7 +353,7 @@ impl<'interner> Monomorphizer<'interner> { }; let return_type = Self::convert_type(return_type, meta.location)?; - let unconstrained = modifiers.is_unconstrained; + let unconstrained = self.in_unconstrained_function; let attributes = self.interner.function_attributes(&f); let inline_type = InlineType::from(attributes); From 459b3003abba1fc8a3710cc685a2cc4e518679b8 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 09:56:50 -0600 Subject: [PATCH 03/16] Fix force-brillig flag --- compiler/noirc_driver/src/lib.rs | 12 ++++++--- compiler/noirc_evaluator/src/ssa.rs | 26 +++++-------------- .../src/ssa/ssa_gen/context.rs | 9 ++----- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 24 +++++++---------- .../src/monomorphization/mod.rs | 5 +++- 5 files changed, 31 insertions(+), 45 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 1b311504b5c..6b852354824 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -590,10 +590,17 @@ pub fn compile_no_check( cached_program: Option, force_compile: bool, ) -> Result { + let force_unconstrained = options.force_brillig; + let program = if options.instrument_debug { - monomorphize_debug(main_function, &mut context.def_interner, &context.debug_instrumenter)? + monomorphize_debug( + main_function, + &mut context.def_interner, + &context.debug_instrumenter, + force_unconstrained, + )? } else { - monomorphize(main_function, &mut context.def_interner)? + monomorphize(main_function, &mut context.def_interner, force_unconstrained)? }; if options.show_monomorphized { @@ -632,7 +639,6 @@ pub fn compile_no_check( } }, enable_brillig_logging: options.show_brillig, - force_brillig_output: options.force_brillig, print_codegen_timings: options.benchmark_codegen, expression_width: if options.bounded_codegen { options.expression_width.unwrap_or(DEFAULT_EXPRESSION_WIDTH) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 78724261f0d..70a77b37aac 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -57,9 +57,6 @@ pub struct SsaEvaluatorOptions { pub enable_brillig_logging: bool, - /// Force Brillig output (for step debugging) - pub force_brillig_output: bool, - /// Pretty print benchmark times of each code generation pass pub print_codegen_timings: bool, @@ -100,7 +97,6 @@ pub(crate) fn optimize_into_acir( let builder = SsaBuilder::new( program, options.ssa_logging.clone(), - options.force_brillig_output, options.print_codegen_timings, &options.emit_ssa, )?; @@ -275,19 +271,12 @@ pub fn create_program( (generated_acirs, generated_brillig, brillig_function_names, error_types), ssa_level_warnings, ) = optimize_into_acir(program, options)?; - if options.force_brillig_output { - assert_eq!( - generated_acirs.len(), - 1, - "Only the main ACIR is expected when forcing Brillig output" - ); - } else { - assert_eq!( - generated_acirs.len(), - func_sigs.len(), - "The generated ACIRs should match the supplied function signatures" - ); - } + + assert_eq!( + generated_acirs.len(), + func_sigs.len(), + "The generated ACIRs should match the supplied function signatures" + ); let error_types = error_types .into_iter() @@ -450,11 +439,10 @@ impl SsaBuilder { fn new( program: Program, ssa_logging: SsaLogging, - force_brillig_runtime: bool, print_codegen_timings: bool, emit_ssa: &Option, ) -> Result { - let ssa = ssa_gen::generate_ssa(program, force_brillig_runtime)?; + let ssa = ssa_gen::generate_ssa(program)?; if let Some(emit_ssa) = emit_ssa { let mut emit_ssa_dir = emit_ssa.clone(); // We expect the full package artifact path to be passed in here, diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 7807658dabb..7d7d84e25b5 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -119,14 +119,9 @@ impl<'a> FunctionContext<'a> { /// /// Note that the previous function cannot be resumed after calling this. Developers should /// avoid calling new_function until the previous function is completely finished with ssa-gen. - pub(super) fn new_function( - &mut self, - id: IrFunctionId, - func: &ast::Function, - force_brillig_runtime: bool, - ) { + pub(super) fn new_function(&mut self, id: IrFunctionId, func: &ast::Function) { self.definitions.clear(); - if func.unconstrained || (force_brillig_runtime && func.inline_type != InlineType::Inline) { + if func.unconstrained || func.inline_type != InlineType::Inline { self.builder.new_brillig_function(func.name.clone(), id, func.inline_type); } else { self.builder.new_function(func.name.clone(), id, func.inline_type); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index d3821158b80..b3a8ecf1314 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -39,10 +39,7 @@ pub(crate) const SSA_WORD_SIZE: u32 = 32; /// Generates SSA for the given monomorphized program. /// /// This function will generate the SSA but does not perform any optimizations on it. -pub(crate) fn generate_ssa( - program: Program, - force_brillig_runtime: bool, -) -> Result { +pub(crate) fn generate_ssa(program: Program) -> Result { // see which parameter has call_data/return_data attribute let is_databus = DataBusBuilder::is_databus(&program.main_function_signature); @@ -56,16 +53,13 @@ pub(crate) fn generate_ssa( // Queue the main function for compilation context.get_or_queue_function(main_id); - let mut function_context = FunctionContext::new( - main.name.clone(), - &main.parameters, - if force_brillig_runtime || main.unconstrained { - RuntimeType::Brillig(main.inline_type) - } else { - RuntimeType::Acir(main.inline_type) - }, - &context, - ); + let main_runtime = if main.unconstrained { + RuntimeType::Brillig(main.inline_type) + } else { + RuntimeType::Acir(main.inline_type) + }; + let mut function_context = + FunctionContext::new(main.name.clone(), &main.parameters, main_runtime, &context); // Generate the call_data bus from the relevant parameters. We create it *before* processing the function body let call_data = function_context.builder.call_data_bus(is_databus); @@ -122,7 +116,7 @@ pub(crate) fn generate_ssa( // to generate SSA for each function used within the program. while let Some((src_function_id, dest_id)) = context.pop_next_function_in_queue() { let function = &context.program[src_function_id]; - function_context.new_function(dest_id, function, force_brillig_runtime); + function_context.new_function(dest_id, function); function_context.codegen_function_body(&function.body)?; } diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 9f37fbb38a8..398be2731ab 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -125,17 +125,20 @@ type HirType = crate::Type; pub fn monomorphize( main: node_interner::FuncId, interner: &mut NodeInterner, + force_unconstrained: bool, ) -> Result { - monomorphize_debug(main, interner, &DebugInstrumenter::default()) + monomorphize_debug(main, interner, &DebugInstrumenter::default(), force_unconstrained) } pub fn monomorphize_debug( main: node_interner::FuncId, interner: &mut NodeInterner, debug_instrumenter: &DebugInstrumenter, + force_unconstrained: bool, ) -> Result { let debug_type_tracker = DebugTypeTracker::build_from_debug_instrumenter(debug_instrumenter); let mut monomorphizer = Monomorphizer::new(interner, debug_type_tracker); + monomorphizer.in_unconstrained_function = force_unconstrained; let function_sig = monomorphizer.compile_main(main)?; while !monomorphizer.queue.is_empty() { From a597f6aafad72439cdc98e4dc643c9441d0a92f9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 10:24:03 -0600 Subject: [PATCH 04/16] Remove case --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- compiler/noirc_frontend/src/monomorphization/mod.rs | 12 ++++++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 7d7d84e25b5..43ce89811d4 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -121,7 +121,7 @@ impl<'a> FunctionContext<'a> { /// avoid calling new_function until the previous function is completely finished with ssa-gen. pub(super) fn new_function(&mut self, id: IrFunctionId, func: &ast::Function) { self.definitions.clear(); - if func.unconstrained || func.inline_type != InlineType::Inline { + if func.unconstrained { self.builder.new_brillig_function(func.name.clone(), id, func.inline_type); } else { self.builder.new_function(func.name.clone(), id, func.inline_type); diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 398be2731ab..be8c457f795 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -146,6 +146,8 @@ pub fn monomorphize_debug( monomorphizer.queue.pop_front().unwrap(); monomorphizer.locals.clear(); + let i = &monomorphizer.interner; + println!("{} is_unconstrained: {}", i.function_name(&next_fn_id), is_unconstrained); monomorphizer.in_unconstrained_function = is_unconstrained; perform_instantiation_bindings(&bindings); @@ -356,6 +358,9 @@ impl<'interner> Monomorphizer<'interner> { }; let return_type = Self::convert_type(return_type, meta.location)?; + + let n = self.interner.function_name(&f); + eprintln!("Setting {n} to {}", self.in_unconstrained_function); let unconstrained = self.in_unconstrained_function; let attributes = self.interner.function_attributes(&f); @@ -2048,8 +2053,11 @@ impl<'interner> Monomorphizer<'interner> { } fn is_unconstrained(&self, func_id: node_interner::FuncId) -> bool { - self.in_unconstrained_function - || self.interner.function_modifiers(&func_id).is_unconstrained + let a = self.in_unconstrained_function; + let b = self.interner.function_modifiers(&func_id).is_unconstrained; + let n = self.interner.function_name(&func_id); + println!("is_unconstrained({n}): self {a} || function modifiers {b}"); + a || b } } From 16d4b5e2157319c5c2eaad37bbaac8cb090d7880 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 10:26:42 -0600 Subject: [PATCH 05/16] Fix frontend tests --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 43ce89811d4..e89d1d2a0c3 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -4,7 +4,7 @@ use acvm::{acir::AcirField, FieldElement}; use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::ast::{BinaryOpKind, Signedness}; -use noirc_frontend::monomorphization::ast::{self, InlineType, LocalId, Parameters}; +use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use crate::errors::RuntimeError; diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 3d908d1aa0c..3377c260922 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1244,7 +1244,7 @@ fn resolve_fmt_strings() { fn monomorphize_program(src: &str) -> Result { let (_program, mut context, _errors) = get_program(src); let main_func_id = context.def_interner.find_function("main").unwrap(); - monomorphize(main_func_id, &mut context.def_interner) + monomorphize(main_func_id, &mut context.def_interner, false) } fn get_monomorphization_error(src: &str) -> Option { From f1eccde7411236a5c8abbdd9118be377e8d9b7cc Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 20 Dec 2024 10:28:27 -0600 Subject: [PATCH 06/16] Remove printlns --- compiler/noirc_frontend/src/monomorphization/mod.rs | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index be8c457f795..398be2731ab 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -146,8 +146,6 @@ pub fn monomorphize_debug( monomorphizer.queue.pop_front().unwrap(); monomorphizer.locals.clear(); - let i = &monomorphizer.interner; - println!("{} is_unconstrained: {}", i.function_name(&next_fn_id), is_unconstrained); monomorphizer.in_unconstrained_function = is_unconstrained; perform_instantiation_bindings(&bindings); @@ -358,9 +356,6 @@ impl<'interner> Monomorphizer<'interner> { }; let return_type = Self::convert_type(return_type, meta.location)?; - - let n = self.interner.function_name(&f); - eprintln!("Setting {n} to {}", self.in_unconstrained_function); let unconstrained = self.in_unconstrained_function; let attributes = self.interner.function_attributes(&f); @@ -2053,11 +2048,8 @@ impl<'interner> Monomorphizer<'interner> { } fn is_unconstrained(&self, func_id: node_interner::FuncId) -> bool { - let a = self.in_unconstrained_function; - let b = self.interner.function_modifiers(&func_id).is_unconstrained; - let n = self.interner.function_name(&func_id); - println!("is_unconstrained({n}): self {a} || function modifiers {b}"); - a || b + self.in_unconstrained_function + || self.interner.function_modifiers(&func_id).is_unconstrained } } From 07078f84d98c168af50e5a9c58699bc88e6ad818 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 2 Jan 2025 10:05:26 -0600 Subject: [PATCH 07/16] Add check --- compiler/noirc_evaluator/src/ssa/opt/hint.rs | 1 - compiler/noirc_frontend/src/monomorphization/mod.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/hint.rs b/compiler/noirc_evaluator/src/ssa/opt/hint.rs index 567a0795edc..1326c2cc010 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/hint.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/hint.rs @@ -14,7 +14,6 @@ mod tests { let options = &SsaEvaluatorOptions { ssa_logging: SsaLogging::None, enable_brillig_logging: false, - force_brillig_output: false, print_codegen_timings: false, expression_width: ExpressionWidth::default(), emit_ssa: None, diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 398be2731ab..fa2489e5b47 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -162,7 +162,7 @@ pub fn monomorphize_debug( .finished_functions .iter() .flat_map(|(_, f)| { - if f.inline_type.is_entry_point() || f.id == Program::main_id() { + if (!force_unconstrained && f.inline_type.is_entry_point()) || f.id == Program::main_id() { Some(f.func_sig.clone()) } else { None From 0f1f17fbeea527807856b5d53cb1223d19a98b06 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 2 Jan 2025 10:05:35 -0600 Subject: [PATCH 08/16] fmt --- compiler/noirc_frontend/src/monomorphization/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index fa2489e5b47..62ff7fc25c5 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -162,7 +162,9 @@ pub fn monomorphize_debug( .finished_functions .iter() .flat_map(|(_, f)| { - if (!force_unconstrained && f.inline_type.is_entry_point()) || f.id == Program::main_id() { + if (!force_unconstrained && f.inline_type.is_entry_point()) + || f.id == Program::main_id() + { Some(f.func_sig.clone()) } else { None From 73ce9b6786f576f020b9d7fd07a81566687105e6 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 2 Jan 2025 14:29:28 -0600 Subject: [PATCH 09/16] Fix issues with remove unused functions pass --- compiler/noirc_evaluator/src/ssa.rs | 1 + compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + .../src/ssa/opt/normalize_value_ids.rs | 4 +- .../src/ssa/opt/remove_unreachable.rs | 89 +++++++++++++++++++ .../src/ssa/ssa_gen/program.rs | 24 ++--- 5 files changed, 102 insertions(+), 17 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 70a77b37aac..789e25df2ae 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -152,6 +152,7 @@ pub(crate) fn optimize_into_acir( fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result { Ok(builder .run_pass(Ssa::defunctionalize, "Defunctionalization") + .run_pass(Ssa::remove_unreachable_functions, "Removing Unreachable Functions") .run_pass(Ssa::remove_paired_rc, "Removing Paired rc_inc & rc_decs") .run_pass(Ssa::resolve_is_unconstrained, "Resolving IsUnconstrained") .run_pass(|ssa| ssa.inline_functions(options.inliner_aggressiveness), "Inlining (1st)") diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index fbcda682dcc..58dbc25e869 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -20,6 +20,7 @@ mod rc; mod remove_bit_shifts; mod remove_enable_side_effects; mod remove_if_else; +mod remove_unreachable; mod resolve_is_unconstrained; mod simplify_cfg; mod unrolling; 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 63ca523bd57..63e8ccf7af4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -180,7 +180,9 @@ impl IdMaps { } Value::Function(id) => { - let new_id = self.function_ids[id]; + let new_id = *self.function_ids.get(id).unwrap_or_else(|| { + unreachable!("Unmapped function with id {id}") + }); new_function.dfg.import_function(new_id) } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs new file mode 100644 index 00000000000..edcee896e2d --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs @@ -0,0 +1,89 @@ +use std::collections::BTreeSet; + +use fxhash::FxHashSet as HashSet; + +use crate::ssa::{ + ir::{ + function::{Function, FunctionId}, + instruction::Instruction, + value::{Value, ValueId}, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + /// Removes any unreachable functions from the code. These can result from + /// optimizations making existing functions unreachable, e.g. `if false { foo() }`, + /// or even from monomorphizing an unconstrained version of a constrained function + /// where the original constrained version ends up never being used. + /// + /// This pass only checks functions that are called directly and thus depends on + /// the defunctionalize pass to be called beforehand. + pub(crate) fn remove_unreachable_functions(mut self) -> Self { + let mut reachable_functions = HashSet::default(); + + for function_id in self.functions.keys() { + if self.is_entry_point(*function_id) { + collect_reachable_functions(&self, *function_id, &mut reachable_functions); + } + } + + prune_unreachable_functions(&mut self); + self + } +} + +fn collect_reachable_functions( + ssa: &Ssa, + current_func_id: FunctionId, + reachable_functions: &mut HashSet, +) { + if reachable_functions.contains(¤t_func_id) { + return; + } + reachable_functions.insert(current_func_id); + + let func = &ssa.functions[¤t_func_id]; + let called_functions = called_functions(func); + + for called_func_id in called_functions.iter() { + collect_reachable_functions(ssa, *called_func_id, reachable_functions); + } +} + +fn prune_unreachable_functions(ssa: &mut Ssa) { + let mut reachable_functions = HashSet::default(); + collect_reachable_functions(ssa, ssa.main_id, &mut reachable_functions); + + ssa.functions.retain(|id, _value| reachable_functions.contains(id)); +} + +// We only consider direct calls to functions since functions as values should have been resolved +fn called_functions_values(func: &Function) -> BTreeSet { + let mut called_function_ids = BTreeSet::default(); + for block_id in func.reachable_blocks() { + for instruction_id in func.dfg[block_id].instructions() { + let Instruction::Call { func: called_value_id, .. } = &func.dfg[*instruction_id] else { + continue; + }; + + if let Value::Function(_) = func.dfg[*called_value_id] { + called_function_ids.insert(*called_value_id); + } + } + } + + called_function_ids +} + +fn called_functions(func: &Function) -> BTreeSet { + called_functions_values(func) + .into_iter() + .map(|value_id| { + let Value::Function(func_id) = func.dfg[value_id] else { + unreachable!("Value should be a function") + }; + func_id + }) + .collect() +} diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index de01a4596ad..07a2f69ad08 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -6,7 +6,7 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use crate::ssa::ir::{ - function::{Function, FunctionId, RuntimeType}, + function::{Function, FunctionId}, map::AtomicCounter, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -85,21 +85,9 @@ impl Ssa { new_id } pub(crate) fn generate_entry_point_index(mut self) -> Self { - self.entry_point_to_generated_index = btree_map( - self.functions - .iter() - .filter(|(_, func)| { - let runtime = func.runtime(); - match func.runtime() { - RuntimeType::Acir(_) => { - runtime.is_entry_point() || func.id() == self.main_id - } - RuntimeType::Brillig(_) => false, - } - }) - .enumerate(), - |(i, (id, _))| (*id, i as u32), - ); + let entry_points = + self.functions.keys().filter(|function| self.is_entry_point(**function)).enumerate(); + self.entry_point_to_generated_index = btree_map(entry_points, |(i, id)| (*id, i as u32)); self } @@ -111,6 +99,10 @@ impl Ssa { ); self.entry_point_to_generated_index.get(func_id).copied() } + + pub(crate) fn is_entry_point(&self, function: FunctionId) -> bool { + function == self.main_id || self.functions[&function].runtime().is_entry_point() + } } impl Display for Ssa { From ac10ed059a39d4d2ad17c7b09253ca966f5766d0 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 2 Jan 2025 14:56:15 -0600 Subject: [PATCH 10/16] Make lambdas unconstrained too to fix regression --- .../noirc_frontend/src/monomorphization/mod.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 62ff7fc25c5..fbc80cc0ac9 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -1679,19 +1679,15 @@ impl<'interner> Monomorphizer<'interner> { let parameters = self.parameters(¶meters)?; let body = self.expr(lambda.body)?; - let id = self.next_function_id(); - let return_type = ret_type.clone(); - let name = lambda_name.to_owned(); - let unconstrained = false; let function = ast::Function { id, - name, + name: lambda_name.to_owned(), parameters, body, - return_type, - unconstrained, + return_type: ret_type.clone(), + unconstrained: self.in_unconstrained_function, inline_type: InlineType::default(), func_sig: FunctionSignature::default(), }; @@ -1814,18 +1810,16 @@ impl<'interner> Monomorphizer<'interner> { typ: lambda_fn_typ.clone(), }); - let mut parameters = vec![]; - parameters.push((env_local_id, true, env_name.to_string(), env_typ.clone())); + let mut parameters = vec![(env_local_id, true, env_name.to_string(), env_typ.clone())]; parameters.append(&mut converted_parameters); - let unconstrained = false; let function = ast::Function { id, name, parameters, body, return_type, - unconstrained, + unconstrained: self.in_unconstrained_function, inline_type: InlineType::default(), func_sig: FunctionSignature::default(), }; From 48022e88547591989b0c9f959413d223eb7b5b9a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 3 Jan 2025 13:03:51 -0600 Subject: [PATCH 11/16] Remove println --- compiler/noirc_evaluator/src/ssa.rs | 3 +- .../src/ssa/opt/remove_unreachable.rs | 53 +++++-------------- 2 files changed, 16 insertions(+), 40 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 789e25df2ae..80334ce485e 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -151,8 +151,8 @@ pub(crate) fn optimize_into_acir( /// Run all SSA passes. fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result { Ok(builder - .run_pass(Ssa::defunctionalize, "Defunctionalization") .run_pass(Ssa::remove_unreachable_functions, "Removing Unreachable Functions") + .run_pass(Ssa::defunctionalize, "Defunctionalization") .run_pass(Ssa::remove_paired_rc, "Removing Paired rc_inc & rc_decs") .run_pass(Ssa::resolve_is_unconstrained, "Resolving IsUnconstrained") .run_pass(|ssa| ssa.inline_functions(options.inliner_aggressiveness), "Inlining (1st)") @@ -160,6 +160,7 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result Self { - let mut reachable_functions = HashSet::default(); + let mut used_functions = HashSet::default(); for function_id in self.functions.keys() { if self.is_entry_point(*function_id) { - collect_reachable_functions(&self, *function_id, &mut reachable_functions); + collect_reachable_functions(&self, *function_id, &mut used_functions); } } - prune_unreachable_functions(&mut self); + self.functions.retain(|id, _| used_functions.contains(id)); self } } @@ -44,46 +40,25 @@ fn collect_reachable_functions( reachable_functions.insert(current_func_id); let func = &ssa.functions[¤t_func_id]; - let called_functions = called_functions(func); + let used_functions = used_functions(func); - for called_func_id in called_functions.iter() { + for called_func_id in used_functions.iter() { collect_reachable_functions(ssa, *called_func_id, reachable_functions); } } -fn prune_unreachable_functions(ssa: &mut Ssa) { - let mut reachable_functions = HashSet::default(); - collect_reachable_functions(ssa, ssa.main_id, &mut reachable_functions); +fn used_functions(func: &Function) -> BTreeSet { + let mut used_function_ids = BTreeSet::default(); - ssa.functions.retain(|id, _value| reachable_functions.contains(id)); -} - -// We only consider direct calls to functions since functions as values should have been resolved -fn called_functions_values(func: &Function) -> BTreeSet { - let mut called_function_ids = BTreeSet::default(); for block_id in func.reachable_blocks() { for instruction_id in func.dfg[block_id].instructions() { - let Instruction::Call { func: called_value_id, .. } = &func.dfg[*instruction_id] else { - continue; - }; - - if let Value::Function(_) = func.dfg[*called_value_id] { - called_function_ids.insert(*called_value_id); - } + func.dfg[*instruction_id].for_each_value(|value| { + if let Value::Function(function) = func.dfg[value] { + used_function_ids.insert(function); + } + }); } } - called_function_ids -} - -fn called_functions(func: &Function) -> BTreeSet { - called_functions_values(func) - .into_iter() - .map(|value_id| { - let Value::Function(func_id) = func.dfg[value_id] else { - unreachable!("Value should be a function") - }; - func_id - }) - .collect() + used_function_ids } From b5a151b247e5855103d234e1fc096d2da3c40d09 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 3 Jan 2025 13:56:48 -0600 Subject: [PATCH 12/16] Finally find and fix bug --- .../src/ssa/opt/remove_unreachable.rs | 18 ++++++++++++------ .../noirc_evaluator/src/ssa/ssa_gen/program.rs | 1 + 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs index 9fe01d009ae..f72b565736a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs @@ -50,14 +50,20 @@ fn collect_reachable_functions( fn used_functions(func: &Function) -> BTreeSet { let mut used_function_ids = BTreeSet::default(); + let mut find_functions = |value| { + if let Value::Function(function) = func.dfg[value] { + used_function_ids.insert(function); + } + }; + for block_id in func.reachable_blocks() { - for instruction_id in func.dfg[block_id].instructions() { - func.dfg[*instruction_id].for_each_value(|value| { - if let Value::Function(function) = func.dfg[value] { - used_function_ids.insert(function); - } - }); + let block = &func.dfg[block_id]; + + for instruction_id in block.instructions() { + func.dfg[*instruction_id].for_each_value(&mut find_functions); } + + block.unwrap_terminator().for_each_value(&mut find_functions); } used_function_ids diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 07a2f69ad08..42681b1ae6d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -84,6 +84,7 @@ impl Ssa { self.functions.insert(new_id, function); new_id } + pub(crate) fn generate_entry_point_index(mut self) -> Self { let entry_points = self.functions.keys().filter(|function| self.is_entry_point(**function)).enumerate(); From 54ca620c1f48b0365ad47ca98971724c0e0ccf75 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 3 Jan 2025 14:00:10 -0600 Subject: [PATCH 13/16] Remove clone_fn --- compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 42681b1ae6d..6824d488321 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -77,14 +77,6 @@ impl Ssa { new_id } - /// Clones an already existing function with a fresh id - pub(crate) fn clone_fn(&mut self, existing_function_id: FunctionId) -> FunctionId { - let new_id = self.next_id.next(); - let function = Function::clone_with_id(new_id, &self.functions[&existing_function_id]); - self.functions.insert(new_id, function); - new_id - } - pub(crate) fn generate_entry_point_index(mut self) -> Self { let entry_points = self.functions.keys().filter(|function| self.is_entry_point(**function)).enumerate(); From a34c46bf74c96e4bef2afa82e418e15e26b7cb3b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 3 Jan 2025 14:11:11 -0600 Subject: [PATCH 14/16] Avoid issuing inc & dec_rc outside of unconstrained code --- .../src/ssa/function_builder/mod.rs | 44 ++++++++++--------- .../src/ssa/opt/remove_unreachable.rs | 7 ++- 2 files changed, 30 insertions(+), 21 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 855034cedd2..d76c7d0cbda 100644 --- a/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -467,29 +467,33 @@ impl FunctionBuilder { /// /// Returns whether a reference count instruction was issued. fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> bool { - match self.type_of_value(value) { - Type::Numeric(_) => false, - Type::Function => false, - Type::Reference(element) => { - if element.contains_an_array() { - let reference = value; - let value = self.insert_load(reference, element.as_ref().clone()); - self.update_array_reference_count(value, increment); - true - } else { - false + if self.current_function.runtime().is_brillig() { + match self.type_of_value(value) { + Type::Numeric(_) => false, + Type::Function => false, + Type::Reference(element) => { + if element.contains_an_array() { + let reference = value; + let value = self.insert_load(reference, element.as_ref().clone()); + self.update_array_reference_count(value, increment); + true + } else { + false + } } - } - Type::Array(..) | Type::Slice(..) => { - // If there are nested arrays or slices, we wait until ArrayGet - // is issued to increment the count of that array. - if increment { - self.insert_inc_rc(value); - } else { - self.insert_dec_rc(value); + Type::Array(..) | Type::Slice(..) => { + // If there are nested arrays or slices, we wait until ArrayGet + // is issued to increment the count of that array. + if increment { + self.insert_inc_rc(value); + } else { + self.insert_dec_rc(value); + } + true } - true } + } else { + false } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs index f72b565736a..5f45d466854 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs @@ -5,6 +5,7 @@ use fxhash::FxHashSet as HashSet; use crate::ssa::{ ir::{ function::{Function, FunctionId}, + instruction::Instruction, value::Value, }, ssa_gen::Ssa, @@ -60,7 +61,11 @@ fn used_functions(func: &Function) -> BTreeSet { let block = &func.dfg[block_id]; for instruction_id in block.instructions() { - func.dfg[*instruction_id].for_each_value(&mut find_functions); + let instruction = &func.dfg[*instruction_id]; + + if matches!(instruction, Instruction::Store { .. } | Instruction::Call { .. }) { + instruction.for_each_value(&mut find_functions); + } } block.unwrap_terminator().for_each_value(&mut find_functions); From 8c973ea851599ed3fdad388d07def3fd81f3bd10 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 3 Jan 2025 14:32:16 -0600 Subject: [PATCH 15/16] Fix evaluator tests --- .../src/ssa/opt/constant_folding.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/opt/die.rs | 17 ++++++++++------- .../noirc_evaluator/src/ssa/parser/tests.rs | 4 ++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index c81a557178b..43fd96a3abf 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -1550,7 +1550,7 @@ mod test { fn deduplicates_side_effecting_intrinsics() { let src = " // After EnableSideEffectsIf removal: - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: Field, v1: Field, v2: u1): v4 = call is_unconstrained() -> u1 v7 = call to_be_radix(v0, u32 256) -> [u8; 1] // `a.to_be_radix(256)`; @@ -1567,7 +1567,7 @@ mod test { "; let ssa = Ssa::from_str(src).unwrap(); let expected = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: Field, v1: Field, v2: u1): v4 = call is_unconstrained() -> u1 v7 = call to_be_radix(v0, u32 256) -> [u8; 1] diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7b38b764eab..7fdcb4c26c2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -637,10 +637,12 @@ mod test { use std::sync::Arc; use im::vector; + use noirc_frontend::monomorphization::ast::InlineType; use crate::ssa::{ function_builder::FunctionBuilder, ir::{ + function::RuntimeType, map::Id, types::{NumericType, Type}, }, @@ -742,7 +744,7 @@ mod test { #[test] fn keep_paired_rcs_with_array_set() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 2]): inc_rc v0 v2 = array_set v0, index u32 0, value u32 0 @@ -759,7 +761,7 @@ mod test { #[test] fn keep_inc_rc_on_borrowed_array_store() { - // acir(inline) fn main f0 { + // brillig(inline) fn main f0 { // b0(): // v1 = make_array [u32 0, u32 0] // v2 = allocate @@ -776,6 +778,7 @@ mod test { // Compiling main let mut builder = FunctionBuilder::new("main".into(), main_id); + builder.set_runtime(RuntimeType::Brillig(InlineType::Inline)); let zero = builder.numeric_constant(0u128, NumericType::unsigned(32)); let array_type = Type::Array(Arc::new(vec![Type::unsigned(32)]), 2); let v1 = builder.insert_make_array(vector![zero, zero], array_type.clone()); @@ -810,7 +813,7 @@ mod test { #[test] fn keep_inc_rc_on_borrowed_array_set() { - // acir(inline) fn main f0 { + // brillig(inline) fn main f0 { // b0(v0: [u32; 2]): // inc_rc v0 // v3 = array_set v0, index u32 0, value u32 1 @@ -821,7 +824,7 @@ mod test { // return v4 // } let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [u32; 2]): inc_rc v0 v3 = array_set v0, index u32 0, value u32 1 @@ -837,7 +840,7 @@ mod test { // We expect the output to be unchanged // Except for the repeated inc_rc instructions let expected = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [u32; 2]): inc_rc v0 v3 = array_set v0, index u32 0, value u32 1 @@ -875,7 +878,7 @@ mod test { #[test] fn remove_inc_rcs_that_are_never_mutably_borrowed() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 2]): inc_rc v0 inc_rc v0 @@ -893,7 +896,7 @@ mod test { assert_eq!(main.dfg[main.entry_block()].instructions().len(), 5); let expected = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 2]): v2 = array_get v0, index u32 0 -> Field return v2 diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index dab96dfa04f..747f8f9a55a 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -415,7 +415,7 @@ fn test_store() { #[test] fn test_inc_rc() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 3]): inc_rc v0 return @@ -427,7 +427,7 @@ fn test_inc_rc() { #[test] fn test_dec_rc() { let src = " - acir(inline) fn main f0 { + brillig(inline) fn main f0 { b0(v0: [Field; 3]): dec_rc v0 return From a3db9ccec8e2e98bee3260fb0250f55d7130d86a Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Mon, 6 Jan 2025 12:04:25 -0600 Subject: [PATCH 16/16] Add exception for the debugger --- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 5 ++--- .../noirc_evaluator/src/ssa/opt/remove_unreachable.rs | 9 +++++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 322639a03d2..598f7c27eff 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -51,9 +51,8 @@ fn value(function: &Function, id: ValueId) -> String { } Value::Function(id) => id.to_string(), Value::Intrinsic(intrinsic) => intrinsic.to_string(), - Value::Param { .. } | Value::Instruction { .. } | Value::ForeignFunction(_) => { - id.to_string() - } + Value::ForeignFunction(function) => function.clone(), + Value::Param { .. } | Value::Instruction { .. } => id.to_string(), } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs index 5f45d466854..41023b5f376 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_unreachable.rs @@ -40,7 +40,12 @@ fn collect_reachable_functions( } reachable_functions.insert(current_func_id); - let func = &ssa.functions[¤t_func_id]; + // If the debugger is used, its possible for function inlining + // to remove functions that the debugger still references + let Some(func) = ssa.functions.get(¤t_func_id) else { + return; + }; + let used_functions = used_functions(func); for called_func_id in used_functions.iter() { @@ -52,7 +57,7 @@ fn used_functions(func: &Function) -> BTreeSet { let mut used_function_ids = BTreeSet::default(); let mut find_functions = |value| { - if let Value::Function(function) = func.dfg[value] { + if let Value::Function(function) = func.dfg[func.dfg.resolve(value)] { used_function_ids.insert(function); } };