fix(inlining): Do not inline globals and lower them during ACIR gen

compiler/noirc_evaluator/src/acir/mod.rs

@@ -6,9 +6,9 @@
 //! ACIR generation is performed by calling the [Ssa::into_acir] method, providing any necessary brillig bytecode.
 //! The compiled program will be returned as an [`Artifacts`] type.
 
-use fxhash::FxHashMap as HashMap;
+use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
 use noirc_errors::call_stack::CallStack;
-use std::collections::{BTreeMap, HashSet};
+use std::collections::BTreeMap;
 use types::{AcirDynamicArray, AcirValue};
 
 use acvm::acir::{
@@ -85,6 +85,12 @@ struct SharedContext<F: AcirField> {
     /// Keeps track of Brillig std lib calls per function that need to still be resolved
     /// with the correct function pointer from the `brillig_stdlib_func_pointer` map.
     brillig_stdlib_calls_to_resolve: HashMap<FunctionId, Vec<(OpcodeLocation, BrilligFunctionId)>>,
+
+    /// `used_globals` needs to be built from a function call graph.
+    ///
+    /// Maps an ACIR function to the globals used in that function.
+    /// This includes all globals used in functions called internally.
+    used_globals: HashMap<FunctionId, HashSet<ValueId>>,
 }
 
 impl<F: AcirField> SharedContext<F> {
@@ -239,7 +245,7 @@ impl<'a> Context<'a> {
             ssa_values: HashMap::default(),
             current_side_effects_enabled_var,
             acir_context,
-            initialized_arrays: HashSet::new(),
+            initialized_arrays: HashSet::default(),
             memory_blocks: HashMap::default(),
             internal_memory_blocks: HashMap::default(),
             internal_mem_block_lengths: HashMap::default(),
@@ -315,8 +321,37 @@ impl<'a> Context<'a> {
             expr.to_witness().expect("return vars should be witnesses")
         });
 
-        self.data_bus = dfg.data_bus.to_owned();
         let mut warnings = Vec::new();
+
+        let used_globals =
+            self.shared_context.used_globals.remove(&main_func.id()).unwrap_or_default();
+
+        let globals_dfg = (*main_func.dfg.globals).clone();
+        let globals_dfg = DataFlowGraph::from(globals_dfg);
+        for (id, value) in globals_dfg.values_iter() {
+            if !used_globals.contains(&id) {
+                continue;
+            }
+            match value {
+                Value::NumericConstant { .. } => {
+                    self.convert_value(id, dfg);
+                }
+                Value::Instruction { instruction, .. } => {
+                    warnings.extend(self.convert_ssa_instruction(
+                        *instruction,
+                        &globals_dfg,
+                        ssa,
+                    )?);
+                }
+                _ => {
+                    panic!(
+                        "Expected either an instruction or a numeric constant for a global value"
+                    )
+                }
+            }
+        }
+
+        self.data_bus = dfg.data_bus.to_owned();
         for instruction_id in entry_block.instructions() {
             warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa)?);
         }

compiler/noirc_evaluator/src/acir/ssa.rs

@@ -41,9 +41,15 @@ pub(super) fn codegen_acir(
     expression_width: ExpressionWidth,
 ) -> Result<Artifacts, RuntimeError> {
     let mut acirs = Vec::new();
+
+    let used_globals = ssa.used_globals_in_functions();
+
     // TODO: can we parallelize this?
-    let mut shared_context =
-        SharedContext { brillig_stdlib: brillig_stdlib.clone(), ..SharedContext::default() };
+    let mut shared_context = SharedContext {
+        brillig_stdlib: brillig_stdlib.clone(),
+        used_globals,
+        ..SharedContext::default()
+    };
 
     for function in ssa.functions.values() {
         let context = Context::new(

compiler/noirc_evaluator/src/brillig/mod.rs

@@ -139,7 +139,7 @@ impl Ssa {
     /// Compile Brillig functions and ACIR functions reachable from them
     #[tracing::instrument(level = "trace", skip_all)]
     pub fn to_brillig(&self, options: &BrilligOptions) -> Brillig {
-        let used_globals_map = self.used_globals_in_brillig_functions();
+        let used_globals_map = self.used_globals_in_functions();
 
         // Collect all the function ids that are reachable from brillig
         // That means all the functions marked as brillig and ACIR functions called by them

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

@@ -15,7 +15,7 @@ use crate::ssa::{
     ir::{
         basic_block::BasicBlockId,
         call_graph::CallGraph,
-        dfg::{GlobalsGraph, InsertInstructionResult},
+        dfg::InsertInstructionResult,
         function::{Function, FunctionId, RuntimeType},
         instruction::{Instruction, InstructionId, TerminatorInstruction},
         value::{Value, ValueId},
@@ -189,8 +189,6 @@ struct PerFunctionContext<'function> {
 
     /// True if we're currently working on the entry point function.
     inlining_entry: bool,
-
-    globals: &'function GlobalsGraph,
 }
 
 impl InlineContext {
@@ -213,8 +211,7 @@ impl InlineContext {
     ) -> Result<Function, RuntimeError> {
         let entry_point = &ssa.functions[&self.entry_point];
 
-        let globals = &entry_point.dfg.globals;
-        let mut context = PerFunctionContext::new(&mut self, entry_point, entry_point, globals);
+        let mut context = PerFunctionContext::new(&mut self, entry_point, entry_point);
         context.inlining_entry = true;
 
         // The entry block is already inserted so we have to add it to context.blocks and add
@@ -263,8 +260,7 @@ impl InlineContext {
         }
 
         let entry_point = &ssa.functions[&self.entry_point];
-        let globals = &source_function.dfg.globals;
-        let mut context = PerFunctionContext::new(self, entry_point, source_function, globals);
+        let mut context = PerFunctionContext::new(self, entry_point, source_function);
 
         let parameters = source_function.parameters();
         assert_eq!(parameters.len(), arguments.len());
@@ -288,7 +284,6 @@ impl<'function> PerFunctionContext<'function> {
         context: &'function mut InlineContext,
         entry_function: &'function Function,
         source_function: &'function Function,
-        globals: &'function GlobalsGraph,
     ) -> Self {
         Self {
             context,
@@ -297,7 +292,6 @@ impl<'function> PerFunctionContext<'function> {
             blocks: HashMap::default(),
             values: HashMap::default(),
             inlining_entry: false,
-            globals,
         }
     }
 
@@ -312,21 +306,9 @@ impl<'function> PerFunctionContext<'function> {
         }
 
         let new_value = match &self.source_function.dfg[id] {
-            value @ Value::Instruction { instruction, .. } => {
+            value @ Value::Instruction { .. } => {
                 if self.source_function.dfg.is_global(id) {
-                    if self.context.builder.current_function.dfg.runtime().is_acir() {
-                        let Instruction::MakeArray { elements, typ } = &self.globals[*instruction]
-                        else {
-                            panic!("Only expect Instruction::MakeArray for a global");
-                        };
-                        let elements = elements
-                            .iter()
-                            .map(|element| self.translate_value(*element))
-                            .collect::<im::Vector<_>>();
-                        return self.context.builder.insert_make_array(elements, typ.clone());
-                    } else {
-                        return id;
-                    }
+                    return id;
                 }
                 unreachable!(
                     "All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value {id} = {value:?}"
@@ -340,10 +322,7 @@ impl<'function> PerFunctionContext<'function> {
             Value::NumericConstant { constant, typ } => {
                 // The dfg indexes a global's inner value directly, so we need to check here
                 // whether we have a global.
-                // We also only keep a global and do not inline it in a Brillig runtime.
-                if self.source_function.dfg.is_global(id)
-                    && self.context.builder.current_function.dfg.runtime().is_brillig()
-                {
+                if self.source_function.dfg.is_global(id) {
                     id
                 } else {
                     self.context.builder.numeric_constant(*constant, *typ)
@@ -1205,7 +1184,7 @@ mod test {
     }
 
     #[test]
-    fn acir_global_arrays_are_inlined_with_new_value_ids() {
+    fn acir_global_arrays_keep_same_value_ids() {
         let src = "
         g0 = Field 1
         g1 = Field 2
@@ -1232,8 +1211,7 @@ mod test {
 
         acir(inline) fn main f0 {
           b0():
-            v3 = make_array [Field 1, Field 2] : [Field; 2]
-            return v3
+            return g2
         }
         ");
     }
@@ -1272,7 +1250,7 @@ mod test {
     }
 
     #[test]
-    fn acir_global_constants_are_inlined_with_new_value_ids() {
+    fn acir_global_constants_keep_same_value_ids() {
         let src = "
         g0 = Field 1
 
@@ -1298,8 +1276,7 @@ mod test {
             panic!("Expected return");
         };
         assert_eq!(return_values.len(), 1);
-        // TODO(https://github.com/noir-lang/noir/issues/9408)
-        // assert!(!main.dfg.is_global(return_values[0]));
+        assert!(main.dfg.is_global(return_values[0]));
 
         assert_ssa_snapshot!(ssa, @r"
         g0 = Field 1

compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs

@@ -99,9 +99,7 @@ impl Ssa {
         function == self.main_id || self.functions[&function].runtime().is_entry_point()
     }
 
-    pub(crate) fn used_globals_in_brillig_functions(
-        &self,
-    ) -> HashMap<FunctionId, HashSet<ValueId>> {
+    pub(crate) fn used_globals_in_functions(&self) -> HashMap<FunctionId, HashSet<ValueId>> {
         fn add_value_to_globals_if_global(
             function: &Function,
             value_id: ValueId,
@@ -128,10 +126,6 @@ impl Ssa {
         let mut used_globals = HashMap::default();
 
         for (function_id, function) in &self.functions {
-            if !function.runtime().is_brillig() {
-                continue;
-            }
-
             let mut used_globals_in_function = HashSet::default();
 
             for call_data in &function.dfg.data_bus.call_data {