chore(ssa): Consolidate should inline check into a single filter

compiler/noirc_evaluator/src/ssa/opt/inlining.rs

@@ -16,7 +16,7 @@ use crate::ssa::{
         basic_block::BasicBlockId,
         call_graph::CallGraph,
         dfg::InsertInstructionResult,
-        function::{Function, FunctionId, RuntimeType},
+        function::{Function, FunctionId},
         instruction::{Instruction, InstructionId, TerminatorInstruction},
         value::{Value, ValueId},
     },
@@ -79,8 +79,7 @@ impl Ssa {
                 inline_no_predicates_functions,
                 aggressiveness,
             );
-            self =
-                Self::inline_functions_inner(self, &inline_infos, inline_no_predicates_functions)?;
+            self = Self::inline_functions_inner(self, &inline_infos)?;
 
             let num_functions_after = self.functions.len();
             if num_functions_after == num_functions_before {
@@ -91,30 +90,15 @@ impl Ssa {
         Ok(self)
     }
 
-    fn inline_functions_inner(
-        mut self,
-        inline_infos: &InlineInfos,
-        inline_no_predicates_functions: bool,
-    ) -> Result<Ssa, RuntimeError> {
+    fn inline_functions_inner(mut self, inline_infos: &InlineInfos) -> Result<Ssa, RuntimeError> {
         let inline_targets = inline_infos.iter().filter_map(|(id, info)| {
             let dfg = &self.functions[id].dfg;
             info.is_inline_target(dfg).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())
-                }
-            }
+            // We defer to the inline info computation to determine whether a function should be inlined
+            InlineInfo::should_inline(inline_infos, callee.id())
         };
 
         // NOTE: Functions are processed independently of each other, with the final mapping replacing the original,
@@ -464,29 +448,23 @@ impl<'function> PerFunctionContext<'function> {
                 Instruction::Call { func, arguments } => match self.get_function(*func) {
                     Some(func_id) => {
                         let call_stack = self.source_function.dfg.get_instruction_call_stack(*id);
-                        if let Some(callee) = self.should_inline_call(ssa, func_id, call_stack)? {
-                            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::EnableSideEffectsIf`
-                                // 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::EnableSideEffectsIf` 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);
+
+                        let callee = &ssa.functions[&func_id];
+                        // Sanity check to validate runtime compatibility
+                        self.validate_callee(callee, call_stack)?;
+                        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::EnableSideEffectsIf`
+                            // 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::EnableSideEffectsIf` 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);
@@ -504,46 +482,33 @@ impl<'function> PerFunctionContext<'function> {
         Ok(())
     }
 
-    fn should_inline_call<'a>(
+    /// Extra error check where given a caller's runtime its callee runtime is valid.
+    /// We determine validity as the following (where we have caller -> callee).
+    /// Valid:
+    /// - ACIR -> ACIR
+    /// - ACIR -> Brillig
+    /// - Brillig -> Brillig
+    ///
+    /// Invalid:
+    /// - Brillig -> ACIR
+    ///
+    /// Whether a valid callee should be inlined is determined separately by the inline info computation.
+    fn validate_callee(
         &self,
-        ssa: &'a Ssa,
-        called_func_id: FunctionId,
+        callee: &Function,
         call_stack: Vec<Location>,
-    ) -> Result<Option<&'a Function>, RuntimeError> {
-        // 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.entry_function.id() == called_func_id {
-            return Ok(None);
-        }
-
-        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 it is called from brillig, it cannot be inlined because runtimes do not share the same semantic
-                if self.entry_function.runtime().is_brillig() {
-                    return Err(RuntimeError::UnconstrainedCallingConstrained {
-                        call_stack,
-                        constrained: callee.name().to_string(),
-                        unconstrained: self.entry_function.name().to_string(),
-                    });
-                }
-                if inline_type.is_entry_point() {
-                    assert!(!self.entry_function.runtime().is_brillig());
-                    return Ok(None);
-                }
-            }
-            RuntimeType::Brillig(_) => {
-                if self.entry_function.runtime().is_acir() {
-                    // We never inline a brillig function into an ACIR function.
-                    return Ok(None);
-                }
-            }
+    ) -> Result<(), RuntimeError> {
+        if self.entry_function.runtime().is_brillig() && callee.runtime().is_acir() {
+            // If the caller is Brillig and the called function is ACIR,
+            // it cannot be inlined because runtimes do not share the same semantics
+            return Err(RuntimeError::UnconstrainedCallingConstrained {
+                call_stack,
+                constrained: callee.name().to_string(),
+                unconstrained: self.entry_function.name().to_string(),
+            });
         }
 
-        Ok(Some(callee))
+        Ok(())
     }
 
     /// Inline a function call and remember the inlined return values in the values map

compiler/noirc_evaluator/src/ssa/opt/inlining/inline_info.rs

@@ -156,7 +156,12 @@ fn compute_function_should_be_inlined(
     index: PetGraphIndex,
 ) {
     let func_id = call_graph.indices_to_ids()[&index];
-    if inline_infos.get(&func_id).is_some_and(|info| info.should_inline || info.weight != 0) {
+    if inline_infos.get(&func_id).is_some_and(|info| {
+        info.should_inline
+            || info.weight != 0
+            || info.is_brillig_entry_point
+            || info.is_acir_entry_point
+    }) {
         return; // Already processed
     }
 
@@ -199,7 +204,7 @@ fn compute_function_should_be_inlined(
     info.weight = total_weight;
     info.cost = net_cost;
 
-    if info.is_recursive {
+    if runtime.is_brillig() && info.is_recursive {
         return;
     }
 
@@ -502,16 +507,36 @@ mod tests {
 
         let ssa = Ssa::from_str(src).unwrap();
         let call_graph = CallGraph::from_ssa_weighted(&ssa);
+        let infos = compute_inline_infos(&ssa, &call_graph, false, i64::MIN);
+        let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
+        assert!(
+            !f1.should_inline,
+            "no_predicates functions should NOT be inlined if the flag is false"
+        );
+
         let infos = compute_inline_infos(&ssa, &call_graph, false, 0);
+        let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
+        assert!(
+            !f1.should_inline,
+            "no_predicates functions should NOT be inlined if the flag is false"
+        );
 
+        let infos = compute_inline_infos(&ssa, &call_graph, false, i64::MAX);
         let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
         assert!(
             !f1.should_inline,
             "no_predicates functions should NOT be inlined if the flag is false"
         );
 
+        let infos = compute_inline_infos(&ssa, &call_graph, true, i64::MIN);
+        let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
+        assert!(f1.should_inline, "no_predicates functions should be inlined if the flag is true");
+
         let infos = compute_inline_infos(&ssa, &call_graph, true, 0);
+        let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
+        assert!(f1.should_inline, "no_predicates functions should be inlined if the flag is true");
 
+        let infos = compute_inline_infos(&ssa, &call_graph, true, i64::MAX);
         let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
         assert!(f1.should_inline, "no_predicates functions should be inlined if the flag is true");
     }
@@ -538,4 +563,25 @@ mod tests {
         let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
         assert!(f1.should_inline, "inline_always functions should be inlined");
     }
+
+    #[test]
+    fn basic_inlining_brillig_not_inlined_into_acir() {
+        let src = "
+        acir(inline) fn foo f0 {
+          b0():
+            v1 = call f1() -> Field
+            return v1
+        }
+        brillig(inline) fn bar f1 {
+          b0():
+            return Field 72
+        }
+        ";
+        let ssa = Ssa::from_str(src).unwrap();
+        let call_graph = CallGraph::from_ssa_weighted(&ssa);
+        let infos = compute_inline_infos(&ssa, &call_graph, false, 0);
+
+        let f1 = infos.get(&Id::test_new(1)).expect("Should analyze f1");
+        assert!(!f1.should_inline, "Brillig entry points should never be inlined");
+    }
 }