From beb1d3479ae4cebcd9872b43e14389cea11da558 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 22 Jan 2025 12:57:25 -0300 Subject: [PATCH 01/13] chore: let `Function::inlined` take a `should_inline_call` function --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 88 ++++++++++--------- .../src/ssa/opt/preprocess_fns.rs | 16 +++- 2 files changed, 61 insertions(+), 43 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index c3b771d9102..bf7a34650ee 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -68,8 +68,16 @@ impl Ssa { // instead of inlining the "leaf" functions, moving up towards the entry point. self.functions = btree_map(inline_targets, |entry_point| { let function = &self.functions[&entry_point]; - let new_function = - function.inlined(&self, inline_no_predicates_functions, inline_infos); + let should_inline_call = + |_context: &PerFunctionContext, ssa: &Ssa, called_func_id: FunctionId| -> bool { + function.should_inline_call( + ssa, + called_func_id, + inline_no_predicates_functions, + inline_infos, + ) + }; + let new_function = function.inlined(&self, &should_inline_call); (entry_point, new_function) }); self @@ -81,47 +89,47 @@ impl Function { pub(super) fn inlined( &self, ssa: &Ssa, + should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + ) -> Function { + InlineContext::new(ssa, self.id()).inline_all(ssa, &should_inline_call) + } + + /// Generic function that determines whether a function should inline a call. + pub(super) fn should_inline_call( + &self, + ssa: &Ssa, + called_func_id: FunctionId, inline_no_predicates_functions: bool, inline_infos: &InlineInfos, - ) -> Function { - let caller_runtime = self.runtime(); - - let should_inline_call = - |_context: &PerFunctionContext, ssa: &Ssa, called_func_id: FunctionId| -> bool { - // Do not inline self-recursive functions on the top level. - // Inlining a self-recursive function works when there is something to inline into - // by importing all the recursive blocks, but for the entry function there is no wrapper. - if called_func_id == self.id() { - return false; - } - let callee = &ssa.functions[&called_func_id]; + ) -> bool { + // Do not inline self-recursive functions on the top level. + // Inlining a self-recursive function works when there is something to inline into + // by importing all the recursive blocks, but for the entry function there is no wrapper. + if called_func_id == self.id() { + return false; + } + let callee = &ssa.functions[&called_func_id]; - match callee.runtime() { - RuntimeType::Acir(inline_type) => { - // If the called function is acir, we inline if it's not an entry point + match callee.runtime() { + RuntimeType::Acir(inline_type) => { + // If the called function is acir, we inline if it's not an entry point - // If we have not already finished the flattening pass, functions marked - // to not have predicates should be preserved. - let preserve_function = - !inline_no_predicates_functions && callee.is_no_predicates(); + // If we have not already finished the flattening pass, functions marked + // to not have predicates should be preserved. + let preserve_function = + !inline_no_predicates_functions && callee.is_no_predicates(); - !inline_type.is_entry_point() && !preserve_function - } - RuntimeType::Brillig(_) => { - if caller_runtime.is_acir() { - // We never inline a brillig function into an ACIR function. - return false; - } - // We inline inline if the function called wasn't ruled out as too costly or recursive. - inline_infos - .get(&called_func_id) - .map(|info| info.should_inline) - .unwrap_or_default() - } + !inline_type.is_entry_point() && !preserve_function + } + RuntimeType::Brillig(_) => { + if self.runtime().is_acir() { + // We never inline a brillig function into an ACIR function. + return false; } - }; - - InlineContext::new(ssa, self.id()).inline_all(ssa, &should_inline_call) + // We inline inline if the function called wasn't ruled out as too costly or recursive. + inline_infos.get(&called_func_id).map(|info| info.should_inline).unwrap_or_default() + } + } } } @@ -145,7 +153,7 @@ struct InlineContext { /// layer to translate between BlockId to BlockId for the current function and the function to /// inline into. The same goes for ValueIds, InstructionIds, and for storing other data like /// parameter to argument mappings. -struct PerFunctionContext<'function> { +pub(crate) struct PerFunctionContext<'function> { /// The source function is the function we're currently inlining into the function being built. source_function: &'function Function, @@ -205,7 +213,7 @@ pub(super) struct InlineInfo { is_brillig_entry_point: bool, is_acir_entry_point: bool, is_recursive: bool, - should_inline: bool, + pub(super) should_inline: bool, weight: i64, cost: i64, } @@ -519,7 +527,7 @@ fn mark_brillig_functions_to_retain( inline_no_predicates_functions: bool, aggressiveness: i64, times_called: &HashMap, - inline_infos: &mut BTreeMap, + inline_infos: &mut InlineInfos, ) { let brillig_entry_points = inline_infos .iter() diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index 439c2da5a2d..a3890df9931 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -1,8 +1,8 @@ //! Pre-process functions before inlining them into others. -use crate::ssa::Ssa; +use crate::ssa::{ir::function::FunctionId, Ssa}; -use super::inlining; +use super::inlining::{self, PerFunctionContext}; impl Ssa { /// Run pre-processing steps on functions in isolation. @@ -34,7 +34,17 @@ impl Ssa { } let function = &self.functions[&id]; // Start with an inline pass. - let mut function = function.inlined(&self, false, &inline_infos); + let should_inline_call = + |_context: &PerFunctionContext, ssa: &Ssa, called_func_id: FunctionId| -> bool { + function.should_inline_call( + ssa, + called_func_id, + false, // inline_no_predicates_functions + &inline_infos, + ) + }; + + let mut function = function.inlined(&self, &should_inline_call); // Help unrolling determine bounds. function.as_slice_optimization(); // Prepare for unrolling From 0309ea95a52b9a771347f4f7081b0ed0c154a1d0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 22 Jan 2025 16:47:27 -0300 Subject: [PATCH 02/13] Remove `PerFunctionContext` from callback --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 33 +++++++++---------- .../src/ssa/opt/preprocess_fns.rs | 19 +++++------ 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index bf7a34650ee..2d504117638 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -68,15 +68,14 @@ impl Ssa { // instead of inlining the "leaf" functions, moving up towards the entry point. self.functions = btree_map(inline_targets, |entry_point| { let function = &self.functions[&entry_point]; - let should_inline_call = - |_context: &PerFunctionContext, ssa: &Ssa, called_func_id: FunctionId| -> bool { - function.should_inline_call( - ssa, - called_func_id, - inline_no_predicates_functions, - inline_infos, - ) - }; + let should_inline_call = |ssa: &Ssa, called_func_id: FunctionId| -> bool { + function.should_inline_call( + ssa, + called_func_id, + inline_no_predicates_functions, + inline_infos, + ) + }; let new_function = function.inlined(&self, &should_inline_call); (entry_point, new_function) }); @@ -89,7 +88,7 @@ impl Function { pub(super) fn inlined( &self, ssa: &Ssa, - should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, ) -> Function { InlineContext::new(ssa, self.id()).inline_all(ssa, &should_inline_call) } @@ -153,7 +152,7 @@ struct InlineContext { /// layer to translate between BlockId to BlockId for the current function and the function to /// inline into. The same goes for ValueIds, InstructionIds, and for storing other data like /// parameter to argument mappings. -pub(crate) struct PerFunctionContext<'function> { +struct PerFunctionContext<'function> { /// The source function is the function we're currently inlining into the function being built. source_function: &'function Function, @@ -582,7 +581,7 @@ impl InlineContext { fn inline_all( mut self, ssa: &Ssa, - should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, ) -> Function { let entry_point = &ssa.functions[&self.entry_point]; @@ -625,7 +624,7 @@ impl InlineContext { ssa: &Ssa, id: FunctionId, arguments: &[ValueId], - should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, ) -> Vec { self.recursion_level += 1; @@ -785,7 +784,7 @@ impl<'function> PerFunctionContext<'function> { fn inline_blocks( &mut self, ssa: &Ssa, - should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, ) -> Vec { let mut seen_blocks = HashSet::new(); let mut block_queue = VecDeque::new(); @@ -852,7 +851,7 @@ impl<'function> PerFunctionContext<'function> { &mut self, ssa: &Ssa, block_id: BasicBlockId, - should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, ) { let mut side_effects_enabled: Option = None; @@ -861,7 +860,7 @@ impl<'function> PerFunctionContext<'function> { match &self.source_function.dfg[*id] { Instruction::Call { func, arguments } => match self.get_function(*func) { Some(func_id) => { - if should_inline_call(self, ssa, func_id) { + if should_inline_call(ssa, func_id) { self.inline_function(ssa, *id, func_id, arguments, should_inline_call); // This is only relevant during handling functions with `InlineType::NoPredicates` as these @@ -897,7 +896,7 @@ impl<'function> PerFunctionContext<'function> { call_id: InstructionId, function: FunctionId, arguments: &[ValueId], - should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, ) { let old_results = self.source_function.dfg.instruction_results(call_id); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index a3890df9931..d63c86220c5 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -2,7 +2,7 @@ use crate::ssa::{ir::function::FunctionId, Ssa}; -use super::inlining::{self, PerFunctionContext}; +use super::inlining; impl Ssa { /// Run pre-processing steps on functions in isolation. @@ -34,15 +34,14 @@ impl Ssa { } let function = &self.functions[&id]; // Start with an inline pass. - let should_inline_call = - |_context: &PerFunctionContext, ssa: &Ssa, called_func_id: FunctionId| -> bool { - function.should_inline_call( - ssa, - called_func_id, - false, // inline_no_predicates_functions - &inline_infos, - ) - }; + let should_inline_call = |ssa: &Ssa, called_func_id: FunctionId| -> bool { + function.should_inline_call( + ssa, + called_func_id, + false, // inline_no_predicates_functions + &inline_infos, + ) + }; let mut function = function.inlined(&self, &should_inline_call); // Help unrolling determine bounds. From 891bc452f365c0eb60e1cfb74276ecb54971a1ee Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 22 Jan 2025 17:06:57 -0300 Subject: [PATCH 03/13] Always check some conditions --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 93 ++++++++++--------- .../src/ssa/opt/preprocess_fns.rs | 24 +++-- 2 files changed, 64 insertions(+), 53 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 2d504117638..96213895dc0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -69,12 +69,20 @@ impl Ssa { self.functions = btree_map(inline_targets, |entry_point| { let function = &self.functions[&entry_point]; let should_inline_call = |ssa: &Ssa, called_func_id: FunctionId| -> bool { - function.should_inline_call( - ssa, - called_func_id, - inline_no_predicates_functions, - inline_infos, - ) + let callee = &ssa.functions[&called_func_id]; + match callee.runtime() { + RuntimeType::Acir(_) => { + // If we have not already finished the flattening pass, functions marked + // to not have predicates should be preserved. + let preserve_function = + !inline_no_predicates_functions && callee.is_no_predicates(); + !preserve_function + } + RuntimeType::Brillig(_) => { + // We inline inline if the function called wasn't ruled out as too costly or recursive. + InlineInfo::should_inline(inline_infos, called_func_id) + } + } }; let new_function = function.inlined(&self, &should_inline_call); (entry_point, new_function) @@ -92,44 +100,6 @@ impl Function { ) -> Function { InlineContext::new(ssa, self.id()).inline_all(ssa, &should_inline_call) } - - /// Generic function that determines whether a function should inline a call. - pub(super) fn should_inline_call( - &self, - ssa: &Ssa, - called_func_id: FunctionId, - inline_no_predicates_functions: bool, - inline_infos: &InlineInfos, - ) -> bool { - // Do not inline self-recursive functions on the top level. - // Inlining a self-recursive function works when there is something to inline into - // by importing all the recursive blocks, but for the entry function there is no wrapper. - if called_func_id == self.id() { - return false; - } - let callee = &ssa.functions[&called_func_id]; - - match callee.runtime() { - RuntimeType::Acir(inline_type) => { - // If the called function is acir, we inline if it's not an entry point - - // If we have not already finished the flattening pass, functions marked - // to not have predicates should be preserved. - let preserve_function = - !inline_no_predicates_functions && callee.is_no_predicates(); - - !inline_type.is_entry_point() && !preserve_function - } - RuntimeType::Brillig(_) => { - if self.runtime().is_acir() { - // We never inline a brillig function into an ACIR function. - return false; - } - // We inline inline if the function called wasn't ruled out as too costly or recursive. - inline_infos.get(&called_func_id).map(|info| info.should_inline).unwrap_or_default() - } - } - } } /// The context for the function inlining pass. @@ -225,6 +195,10 @@ impl InlineInfo { || self.is_recursive || !self.should_inline } + + pub(super) fn should_inline(inline_infos: &InlineInfos, called_func_id: FunctionId) -> bool { + inline_infos.get(&called_func_id).map(|info| info.should_inline).unwrap_or_default() + } } type InlineInfos = BTreeMap; @@ -860,7 +834,8 @@ impl<'function> PerFunctionContext<'function> { match &self.source_function.dfg[*id] { Instruction::Call { func, arguments } => match self.get_function(*func) { Some(func_id) => { - if should_inline_call(ssa, func_id) { + if self.should_inline_call(ssa, func_id) && should_inline_call(ssa, func_id) + { self.inline_function(ssa, *id, func_id, arguments, should_inline_call); // This is only relevant during handling functions with `InlineType::NoPredicates` as these @@ -889,6 +864,34 @@ impl<'function> PerFunctionContext<'function> { } } + fn should_inline_call(&self, ssa: &Ssa, called_func_id: FunctionId) -> bool { + // Do not inline self-recursive functions on the top level. + // Inlining a self-recursive function works when there is something to inline into + // by importing all the recursive blocks, but for the entry function there is no wrapper. + if self.source_function.id() == called_func_id { + return false; + } + + let callee = &ssa.functions[&called_func_id]; + + match callee.runtime() { + RuntimeType::Acir(inline_type) => { + // If the called function is acir, we inline if it's not an entry point + if inline_type.is_entry_point() { + return false; + } + } + RuntimeType::Brillig(_) => { + if self.source_function.runtime().is_acir() { + // We never inline a brillig function into an ACIR function. + return false; + } + } + } + + true + } + /// Inline a function call and remember the inlined return values in the values map fn inline_function( &mut self, diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index d63c86220c5..007d2b1fea7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -1,8 +1,11 @@ //! Pre-process functions before inlining them into others. -use crate::ssa::{ir::function::FunctionId, Ssa}; +use crate::ssa::{ + ir::function::{FunctionId, RuntimeType}, + Ssa, +}; -use super::inlining; +use super::inlining::{self, InlineInfo}; impl Ssa { /// Run pre-processing steps on functions in isolation. @@ -35,12 +38,17 @@ impl Ssa { let function = &self.functions[&id]; // Start with an inline pass. let should_inline_call = |ssa: &Ssa, called_func_id: FunctionId| -> bool { - function.should_inline_call( - ssa, - called_func_id, - false, // inline_no_predicates_functions - &inline_infos, - ) + let callee = &ssa.functions[&called_func_id]; + match callee.runtime() { + RuntimeType::Acir(_) => { + // Functions marked to not have predicates should be preserved. + !callee.is_no_predicates() + } + RuntimeType::Brillig(_) => { + // We inline inline if the function called wasn't ruled out as too costly or recursive. + InlineInfo::should_inline(&inline_infos, called_func_id) + } + } }; let mut function = function.inlined(&self, &should_inline_call); From abaa595f8def9d7680787e20ba1491fad5f9b0f3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 22 Jan 2025 17:14:37 -0300 Subject: [PATCH 04/13] Let the callback receive &Function --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 68 +++++++++++-------- .../src/ssa/opt/preprocess_fns.rs | 7 +- 2 files changed, 43 insertions(+), 32 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 96213895dc0..df0c7ff3470 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -68,8 +68,7 @@ impl Ssa { // instead of inlining the "leaf" functions, moving up towards the entry point. self.functions = btree_map(inline_targets, |entry_point| { let function = &self.functions[&entry_point]; - let should_inline_call = |ssa: &Ssa, called_func_id: FunctionId| -> bool { - let callee = &ssa.functions[&called_func_id]; + let should_inline_call = |callee: &Function| -> bool { match callee.runtime() { RuntimeType::Acir(_) => { // If we have not already finished the flattening pass, functions marked @@ -80,7 +79,7 @@ impl Ssa { } RuntimeType::Brillig(_) => { // We inline inline if the function called wasn't ruled out as too costly or recursive. - InlineInfo::should_inline(inline_infos, called_func_id) + InlineInfo::should_inline(inline_infos, callee.id()) } } }; @@ -96,7 +95,7 @@ impl Function { pub(super) fn inlined( &self, ssa: &Ssa, - should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Function) -> bool, ) -> Function { InlineContext::new(ssa, self.id()).inline_all(ssa, &should_inline_call) } @@ -555,7 +554,7 @@ impl InlineContext { fn inline_all( mut self, ssa: &Ssa, - should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Function) -> bool, ) -> Function { let entry_point = &ssa.functions[&self.entry_point]; @@ -598,7 +597,7 @@ impl InlineContext { ssa: &Ssa, id: FunctionId, arguments: &[ValueId], - should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Function) -> bool, ) -> Vec { self.recursion_level += 1; @@ -758,7 +757,7 @@ impl<'function> PerFunctionContext<'function> { fn inline_blocks( &mut self, ssa: &Ssa, - should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Function) -> bool, ) -> Vec { let mut seen_blocks = HashSet::new(); let mut block_queue = VecDeque::new(); @@ -825,7 +824,7 @@ impl<'function> PerFunctionContext<'function> { &mut self, ssa: &Ssa, block_id: BasicBlockId, - should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Function) -> bool, ) { let mut side_effects_enabled: Option = None; @@ -834,20 +833,29 @@ impl<'function> PerFunctionContext<'function> { match &self.source_function.dfg[*id] { Instruction::Call { func, arguments } => match self.get_function(*func) { Some(func_id) => { - if self.should_inline_call(ssa, func_id) && should_inline_call(ssa, func_id) - { - self.inline_function(ssa, *id, func_id, arguments, should_inline_call); - - // This is only relevant during handling functions with `InlineType::NoPredicates` as these - // can pollute the function they're being inlined into with `Instruction::EnabledSideEffects`, - // resulting in predicates not being applied properly. - // - // Note that this doesn't cover the case in which there exists an `Instruction::EnabledSideEffects` - // within the function being inlined whilst the source function has not encountered one yet. - // In practice this isn't an issue as the last `Instruction::EnabledSideEffects` in the - // function being inlined will be to turn off predicates rather than to create one. - if let Some(condition) = side_effects_enabled { - self.context.builder.insert_enable_side_effects_if(condition); + if let Some(callee) = self.should_inline_call(ssa, func_id) { + if should_inline_call(callee) { + self.inline_function( + ssa, + *id, + func_id, + arguments, + should_inline_call, + ); + + // This is only relevant during handling functions with `InlineType::NoPredicates` as these + // can pollute the function they're being inlined into with `Instruction::EnabledSideEffects`, + // resulting in predicates not being applied properly. + // + // Note that this doesn't cover the case in which there exists an `Instruction::EnabledSideEffects` + // within the function being inlined whilst the source function has not encountered one yet. + // In practice this isn't an issue as the last `Instruction::EnabledSideEffects` in the + // function being inlined will be to turn off predicates rather than to create one. + if let Some(condition) = side_effects_enabled { + self.context.builder.insert_enable_side_effects_if(condition); + } + } else { + self.push_instruction(*id); } } else { self.push_instruction(*id); @@ -864,12 +872,16 @@ impl<'function> PerFunctionContext<'function> { } } - fn should_inline_call(&self, ssa: &Ssa, called_func_id: FunctionId) -> bool { + fn should_inline_call<'a>( + &self, + ssa: &'a Ssa, + called_func_id: FunctionId, + ) -> Option<&'a Function> { // Do not inline self-recursive functions on the top level. // Inlining a self-recursive function works when there is something to inline into // by importing all the recursive blocks, but for the entry function there is no wrapper. if self.source_function.id() == called_func_id { - return false; + return None; } let callee = &ssa.functions[&called_func_id]; @@ -878,18 +890,18 @@ impl<'function> PerFunctionContext<'function> { RuntimeType::Acir(inline_type) => { // If the called function is acir, we inline if it's not an entry point if inline_type.is_entry_point() { - return false; + return None; } } RuntimeType::Brillig(_) => { if self.source_function.runtime().is_acir() { // We never inline a brillig function into an ACIR function. - return false; + return None; } } } - true + Some(callee) } /// Inline a function call and remember the inlined return values in the values map @@ -899,7 +911,7 @@ impl<'function> PerFunctionContext<'function> { call_id: InstructionId, function: FunctionId, arguments: &[ValueId], - should_inline_call: &impl Fn(&Ssa, FunctionId) -> bool, + should_inline_call: &impl Fn(&Function) -> bool, ) { let old_results = self.source_function.dfg.instruction_results(call_id); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index 007d2b1fea7..7f607ac3b84 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -1,7 +1,7 @@ //! Pre-process functions before inlining them into others. use crate::ssa::{ - ir::function::{FunctionId, RuntimeType}, + ir::function::{Function, RuntimeType}, Ssa, }; @@ -37,8 +37,7 @@ impl Ssa { } let function = &self.functions[&id]; // Start with an inline pass. - let should_inline_call = |ssa: &Ssa, called_func_id: FunctionId| -> bool { - let callee = &ssa.functions[&called_func_id]; + let should_inline_call = |callee: &Function| -> bool { match callee.runtime() { RuntimeType::Acir(_) => { // Functions marked to not have predicates should be preserved. @@ -46,7 +45,7 @@ impl Ssa { } RuntimeType::Brillig(_) => { // We inline inline if the function called wasn't ruled out as too costly or recursive. - InlineInfo::should_inline(&inline_infos, called_func_id) + InlineInfo::should_inline(&inline_infos, callee.id()) } } }; From 27818df321563b7afe23c01539b13d3b701ede8a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 22 Jan 2025 18:03:54 -0300 Subject: [PATCH 05/13] Don't confuse entry_function with source_function --- compiler/noirc_evaluator/src/ssa/opt/inlining.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index df0c7ff3470..158a8193dce 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -122,6 +122,9 @@ struct InlineContext { /// inline into. The same goes for ValueIds, InstructionIds, and for storing other data like /// parameter to argument mappings. struct PerFunctionContext<'function> { + /// The function that we are inlining calls into. + entry_function: &'function Function, + /// The source function is the function we're currently inlining into the function being built. source_function: &'function Function, @@ -558,7 +561,8 @@ impl InlineContext { ) -> Function { let entry_point = &ssa.functions[&self.entry_point]; - let mut context = PerFunctionContext::new(&mut self, entry_point, &ssa.globals); + let mut context = + PerFunctionContext::new(&mut self, entry_point, entry_point, &ssa.globals); context.inlining_entry = true; for (_, value) in entry_point.dfg.globals.values_iter() { @@ -609,7 +613,8 @@ impl InlineContext { ); } - let mut context = PerFunctionContext::new(self, source_function, &ssa.globals); + let entry_point = &ssa.functions[&self.entry_point]; + let mut context = PerFunctionContext::new(self, entry_point, source_function, &ssa.globals); let parameters = source_function.parameters(); assert_eq!(parameters.len(), arguments.len()); @@ -631,11 +636,13 @@ impl<'function> PerFunctionContext<'function> { /// the arguments of the destination function. fn new( context: &'function mut InlineContext, + entry_function: &'function Function, source_function: &'function Function, globals: &'function Function, ) -> Self { Self { context, + entry_function, source_function, blocks: HashMap::default(), values: HashMap::default(), @@ -880,7 +887,7 @@ impl<'function> PerFunctionContext<'function> { // Do not inline self-recursive functions on the top level. // Inlining a self-recursive function works when there is something to inline into // by importing all the recursive blocks, but for the entry function there is no wrapper. - if self.source_function.id() == called_func_id { + if self.entry_function.id() == called_func_id { return None; } @@ -894,7 +901,7 @@ impl<'function> PerFunctionContext<'function> { } } RuntimeType::Brillig(_) => { - if self.source_function.runtime().is_acir() { + if self.entry_function.runtime().is_acir() { // We never inline a brillig function into an ACIR function. return None; } From f8ab025a9e5c6ec0039849dffbe3fef5d79d928e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 22 Jan 2025 18:07:20 -0300 Subject: [PATCH 06/13] Check that runtimes match --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 158a8193dce..5ff9622353e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -892,19 +892,17 @@ impl<'function> PerFunctionContext<'function> { } let callee = &ssa.functions[&called_func_id]; + let callee_runtime = callee.runtime(); - match callee.runtime() { - RuntimeType::Acir(inline_type) => { - // If the called function is acir, we inline if it's not an entry point - if inline_type.is_entry_point() { - return None; - } - } - RuntimeType::Brillig(_) => { - if self.entry_function.runtime().is_acir() { - // We never inline a brillig function into an ACIR function. - return None; - } + // Wd never inline one runtime into another + if self.entry_function.runtime().is_acir() != callee_runtime.is_acir() { + return None; + } + + if let RuntimeType::Acir(inline_type) = callee_runtime { + // If the called function is acir, we inline if it's not an entry point + if inline_type.is_entry_point() { + return None; } } From b8f4aad54ea567a5d7e3181fe8c8957e87220bf4 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 08:03:47 -0300 Subject: [PATCH 07/13] Revert "Check that runtimes match" This reverts commit f8ab025a9e5c6ec0039849dffbe3fef5d79d928e. --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 5ff9622353e..158a8193dce 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -892,17 +892,19 @@ impl<'function> PerFunctionContext<'function> { } let callee = &ssa.functions[&called_func_id]; - let callee_runtime = callee.runtime(); - // Wd never inline one runtime into another - if self.entry_function.runtime().is_acir() != callee_runtime.is_acir() { - return None; - } - - if let RuntimeType::Acir(inline_type) = callee_runtime { - // If the called function is acir, we inline if it's not an entry point - if inline_type.is_entry_point() { - return None; + match callee.runtime() { + RuntimeType::Acir(inline_type) => { + // If the called function is acir, we inline if it's not an entry point + if inline_type.is_entry_point() { + return None; + } + } + RuntimeType::Brillig(_) => { + if self.entry_function.runtime().is_acir() { + // We never inline a brillig function into an ACIR function. + return None; + } } } From 5f6ec4b0aa59e45443ffff31f0c576055af0bbff Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 10:06:32 -0300 Subject: [PATCH 08/13] Move `should_inline_call` up --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 31 ++++++++++--------- .../src/ssa/opt/preprocess_fns.rs | 26 ++++++++-------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 158a8193dce..4437731c8be 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -64,25 +64,26 @@ impl Ssa { let inline_targets = inline_infos.iter().filter_map(|(id, info)| info.is_inline_target().then_some(*id)); + let should_inline_call = |callee: &Function| -> bool { + match callee.runtime() { + RuntimeType::Acir(_) => { + // If we have not already finished the flattening pass, functions marked + // to not have predicates should be preserved. + let preserve_function = + !inline_no_predicates_functions && callee.is_no_predicates(); + !preserve_function + } + RuntimeType::Brillig(_) => { + // We inline inline if the function called wasn't ruled out as too costly or recursive. + InlineInfo::should_inline(inline_infos, callee.id()) + } + } + }; + // NOTE: Functions are processed independently of each other, with the final mapping replacing the original, // instead of inlining the "leaf" functions, moving up towards the entry point. self.functions = btree_map(inline_targets, |entry_point| { let function = &self.functions[&entry_point]; - let should_inline_call = |callee: &Function| -> bool { - match callee.runtime() { - RuntimeType::Acir(_) => { - // If we have not already finished the flattening pass, functions marked - // to not have predicates should be preserved. - let preserve_function = - !inline_no_predicates_functions && callee.is_no_predicates(); - !preserve_function - } - RuntimeType::Brillig(_) => { - // We inline inline if the function called wasn't ruled out as too costly or recursive. - InlineInfo::should_inline(inline_infos, callee.id()) - } - } - }; let new_function = function.inlined(&self, &should_inline_call); (entry_point, new_function) }); diff --git a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs index 7f607ac3b84..a2011eb5ecc 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/preprocess_fns.rs @@ -22,6 +22,19 @@ impl Ssa { // Preliminary inlining decisions. let inline_infos = inlining::compute_inline_infos(&self, false, aggressiveness); + let should_inline_call = |callee: &Function| -> bool { + match callee.runtime() { + RuntimeType::Acir(_) => { + // Functions marked to not have predicates should be preserved. + !callee.is_no_predicates() + } + RuntimeType::Brillig(_) => { + // We inline inline if the function called wasn't ruled out as too costly or recursive. + InlineInfo::should_inline(&inline_infos, callee.id()) + } + } + }; + for (id, (own_weight, transitive_weight)) in bottom_up { // Skip preprocessing heavy functions that gained most of their weight from transitive accumulation. // These can be processed later by the regular SSA passes. @@ -37,19 +50,6 @@ impl Ssa { } let function = &self.functions[&id]; // Start with an inline pass. - let should_inline_call = |callee: &Function| -> bool { - match callee.runtime() { - RuntimeType::Acir(_) => { - // Functions marked to not have predicates should be preserved. - !callee.is_no_predicates() - } - RuntimeType::Brillig(_) => { - // We inline inline if the function called wasn't ruled out as too costly or recursive. - InlineInfo::should_inline(&inline_infos, callee.id()) - } - } - }; - let mut function = function.inlined(&self, &should_inline_call); // Help unrolling determine bounds. function.as_slice_optimization(); From 8c966900085839240da53c17b9a6eecdd1b467f0 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 10:26:11 -0300 Subject: [PATCH 09/13] feat: inline simple functions --- compiler/noirc_evaluator/src/ssa.rs | 1 + .../noirc_evaluator/src/ssa/opt/inlining.rs | 29 +++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 12ea04daebd..320eafdcf14 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -152,6 +152,7 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result Ssa { + let callers = compute_callers(&self); + let times_called = compute_times_called(&callers); + + let should_inline_call = |function: &Function| { + let entry_block_id = function.entry_block(); + let entry_block = &function.dfg[entry_block_id]; + + // If a function is only called once, inline it + if times_called.get(&function.id()) == Some(&1) { + return true; + } + + // Only inline functions with a single block + if entry_block.successors().next().is_some() { + return false; + } + + // Only inline functions with 0 or 1 instructions + entry_block.instructions().len() <= 1 + }; + + self.functions = btree_map(self.functions.iter(), |(id, function)| { + (*id, function.inlined(&self, &should_inline_call)) + }); + + self + } } impl Function { From edde1a2657f65e27a0ca70b18b79af5fdcf94b50 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 10:34:41 -0300 Subject: [PATCH 10/13] Try to first see how it changes without the "called only once" condition --- compiler/noirc_evaluator/src/ssa/opt/inlining.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index afb8c8e0d17..3bc7b56cfb4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -91,18 +91,10 @@ impl Ssa { } pub(crate) fn inline_simple_functions(mut self: Ssa) -> Ssa { - let callers = compute_callers(&self); - let times_called = compute_times_called(&callers); - let should_inline_call = |function: &Function| { let entry_block_id = function.entry_block(); let entry_block = &function.dfg[entry_block_id]; - // If a function is only called once, inline it - if times_called.get(&function.id()) == Some(&1) { - return true; - } - // Only inline functions with a single block if entry_block.successors().next().is_some() { return false; From 36ff6adad4dadaf133305e78cd5a430ca84e3012 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 10:45:23 -0300 Subject: [PATCH 11/13] Add a couple of tests --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 3bc7b56cfb4..6731e363942 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -1144,6 +1144,7 @@ mod test { map::Id, types::{NumericType, Type}, }, + opt::assert_normalized_ssa_equals, Ssa, }; @@ -1618,4 +1619,76 @@ mod test { ); assert!(tws[3] > max(tws[1], tws[2]), "ideally 'main' has the most weight"); } + + #[test] + fn inline_simple_functions_with_zero_instructions() { + let src = " + acir(inline) fn main f0 { + b0(v0: Field): + v2 = call f1(v0) -> Field + v3 = call f1(v0) -> Field + v4 = add v2, v3 + return v4 + } + + acir(inline) fn foo f1 { + b0(v0: Field): + return v0 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + acir(inline) fn main f0 { + b0(v0: Field): + v1 = add v0, v0 + return v1 + } + acir(inline) fn foo f1 { + b0(v0: Field): + return v0 + } + "; + + let ssa = ssa.inline_simple_functions(); + assert_normalized_ssa_equals(ssa, expected); + } + + #[test] + fn inline_simple_functions_with_one_instruction() { + let src = " + acir(inline) fn main f0 { + b0(v0: Field): + v2 = call f1(v0) -> Field + v3 = call f1(v0) -> Field + v4 = add v2, v3 + return v4 + } + + acir(inline) fn foo f1 { + b0(v0: Field): + v2 = add v0, Field 1 + return v2 + } + "; + let ssa = Ssa::from_str(src).unwrap(); + + let expected = " + acir(inline) fn main f0 { + b0(v0: Field): + v2 = add v0, Field 1 + v3 = add v0, Field 1 + v4 = add v2, v3 + return v4 + } + acir(inline) fn foo f1 { + b0(v0: Field): + v2 = add v0, Field 1 + return v2 + } + "; + + let ssa = ssa.inline_simple_functions(); + assert_normalized_ssa_equals(ssa, expected); + } } From 01ceaa3ce99794816a2c9440d8c4de79a0a0d85a Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 11:22:03 -0300 Subject: [PATCH 12/13] Add the usual check we do when inlining --- compiler/noirc_evaluator/src/ssa/opt/inlining.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 6731e363942..d836a3f32b3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -91,9 +91,16 @@ impl Ssa { } pub(crate) fn inline_simple_functions(mut self: Ssa) -> Ssa { - let should_inline_call = |function: &Function| { - let entry_block_id = function.entry_block(); - let entry_block = &function.dfg[entry_block_id]; + let should_inline_call = |callee: &Function| { + if let RuntimeType::Acir(_) = callee.runtime() { + // Functions marked to not have predicates should be preserved. + if callee.is_no_predicates() { + return false; + } + } + + let entry_block_id = callee.entry_block(); + let entry_block = &callee.dfg[entry_block_id]; // Only inline functions with a single block if entry_block.successors().next().is_some() { From a2208d7e09bee4770fee602389f1b18bd5fbaf67 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 23 Jan 2025 15:19:21 -0300 Subject: [PATCH 13/13] Add mem2reg after simple inlining pass --- compiler/noirc_evaluator/src/ssa.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 320eafdcf14..4cefce1d647 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -153,6 +153,7 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result Result Result