diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index f48cd091cc2..b1fe9ccd5bc 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/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 { /// 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>, + + /// `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>, } impl SharedContext { @@ -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)?); } diff --git a/compiler/noirc_evaluator/src/acir/ssa.rs b/compiler/noirc_evaluator/src/acir/ssa.rs index ed5b0679add..b058a01f265 100644 --- a/compiler/noirc_evaluator/src/acir/ssa.rs +++ b/compiler/noirc_evaluator/src/acir/ssa.rs @@ -41,9 +41,15 @@ pub(super) fn codegen_acir( expression_width: ExpressionWidth, ) -> Result { 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( diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 46607051949..67ddecc9dab 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/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 diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index ff7ca13491c..a5b9c76c3de 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/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 { 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::>(); - 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 diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 3c041743c42..d52a4b5b77a 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/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> { + pub(crate) fn used_globals_in_functions(&self) -> HashMap> { 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 {