diff --git a/Cargo.lock b/Cargo.lock index 4d3e2cbd86d..89287609a2b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1764,9 +1764,9 @@ checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" [[package]] name = "foldhash" -version = "0.1.3" +version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f81ec6369c545a7d40e4589b5597581fa1c441fe1cce96dd1de43159910a36a2" +checksum = "a0d2fde1f7b3d48b8395d5f2de76c18a528bd6a9cdde438df747bfcba3e05d6f" [[package]] name = "form_urlencoded" diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index e57f5d18830..1acf5365f09 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1892,6 +1892,9 @@ impl<'a> Context<'a> { Value::Instruction { .. } | Value::Param { .. } => { unreachable!("ICE: Should have been in cache {value_id} {value:?}") } + Value::Global(_) => { + unreachable!("ICE: All globals should have been inlined"); + } }; self.ssa_values.insert(value_id, acir_value.clone()); acir_value diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 31c99bf433e..13a156953fc 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -646,7 +646,10 @@ impl<'block> BrilligBlock<'block> { } } } - Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => { + Value::Instruction { .. } + | Value::Param { .. } + | Value::NumericConstant { .. } + | Value::Global(_) => { unreachable!("unsupported function call type {:?}", dfg[*func]) } }, @@ -1557,6 +1560,9 @@ impl<'block> BrilligBlock<'block> { let value = &dfg[value_id]; match value { + Value::Global(_) => { + unreachable!("ICE: All globals should have been inlined"); + } Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been // converted to registers so we fetch from the cache. diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs index d6851a9ecf9..c0248a50412 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -53,9 +53,10 @@ pub(crate) fn collect_variables_of_value( let value = &dfg[value_id]; match value { - Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => { - Some(value_id) - } + Value::Instruction { .. } + | Value::Param { .. } + | Value::NumericConstant { .. } + | Value::Global(_) => Some(value_id), // Functions are not variables in a defunctionalized SSA. Only constant function values should appear. Value::ForeignFunction(_) | Value::Function(_) | Value::Intrinsic(..) => None, } diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 206fe8d9084..45021fa6158 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -453,7 +453,7 @@ impl SsaBuilder { let ssa_path = emit_ssa.with_extension("ssa.json"); write_to_file(&serde_json::to_vec(&ssa).unwrap(), &ssa_path); } - Ok(SsaBuilder { ssa_logging, print_codegen_timings, ssa }.print("Initial SSA:")) + Ok(SsaBuilder { ssa_logging, print_codegen_timings, ssa }.print("Initial SSA")) } fn finish(self) -> Ssa { diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index d61dd27d02a..80dde5e27f3 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -332,7 +332,8 @@ impl DependencyContext { } Value::Instruction { .. } | Value::NumericConstant { .. } - | Value::Param { .. } => { + | Value::Param { .. } + | Value::Global(_) => { panic!( "calling non-function value with ID {func_id} in function {}", function.name() @@ -618,7 +619,8 @@ impl Context { } Value::Instruction { .. } | Value::NumericConstant { .. } - | Value::Param { .. } => { + | Value::Param { .. } + | Value::Global(_) => { panic!("At the point we are running disconnect there shouldn't be any other values as arguments") } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 57e833af7eb..bf43d825841 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -325,6 +325,10 @@ impl DataFlowGraph { id } + pub(crate) fn make_global(&mut self, typ: Type) -> ValueId { + self.values.insert(Value::Global(typ)) + } + /// Gets or creates a ValueId for the given FunctionId. pub(crate) fn import_function(&mut self, function: FunctionId) -> ValueId { if let Some(existing) = self.functions.get(&function) { @@ -613,6 +617,9 @@ impl DataFlowGraph { } _ => false, }, + // TODO: Make this true and handle instruction simplifications with globals. + // Currently all globals are inlined as a temporary measure so this is fine to have as false. + Value::Global(_) => false, _ => true, } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index 109c2a59781..a2068d94661 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -76,7 +76,7 @@ pub(crate) struct Function { /// Name of the function for debugging only name: String, - id: FunctionId, + id: Option, /// The DataFlowGraph holds the majority of data pertaining to the function /// including its blocks, instructions, and values. @@ -90,14 +90,22 @@ impl Function { pub(crate) fn new(name: String, id: FunctionId) -> Self { let mut dfg = DataFlowGraph::default(); let entry_block = dfg.make_block(); - Self { name, id, entry_block, dfg } + Self { name, id: Some(id), entry_block, dfg } + } + + /// Globals are generated using the same codegen process as functions. + /// To avoid a recursive global context we should create a pseudo function to mock a globals context. + pub(crate) fn new_for_globals() -> Self { + let mut dfg = DataFlowGraph::default(); + let entry_block = dfg.make_block(); + Self { name: "globals".to_owned(), id: None, entry_block, dfg } } /// Creates a new function as a clone of the one passed in with the passed in id. pub(crate) fn clone_with_id(id: FunctionId, another: &Function) -> Self { let dfg = another.dfg.clone(); let entry_block = another.entry_block; - Self { name: another.name.clone(), id, entry_block, dfg } + Self { name: another.name.clone(), id: Some(id), entry_block, dfg } } /// Takes the signature (function name & runtime) from a function but does not copy the body. @@ -115,7 +123,7 @@ impl Function { /// The id of the function. pub(crate) fn id(&self) -> FunctionId { - self.id + self.id.expect("FunctionId should be initialized") } /// Runtime type of the function. @@ -228,12 +236,6 @@ pub(crate) struct Signature { pub(crate) returns: Vec, } -impl std::fmt::Display for Function { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - super::printer::display_function(self, f) - } -} - #[test] fn sign_smoke() { let mut signature = Signature::default(); diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index d30fce170dc..7fe12b83ea9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -1,11 +1,14 @@ //! This file is for pretty-printing the SSA IR in a human-readable form for debugging. -use std::fmt::{Formatter, Result}; +use std::fmt::{Display, Formatter, Result}; use acvm::acir::AcirField; use im::Vector; use iter_extended::vecmap; -use crate::ssa::ir::types::{NumericType, Type}; +use crate::ssa::{ + ir::types::{NumericType, Type}, + Ssa, +}; use super::{ basic_block::BasicBlockId, @@ -15,8 +18,42 @@ use super::{ value::{Value, ValueId}, }; +impl Display for Ssa { + fn fmt(&self, f: &mut Formatter<'_>) -> Result { + for (id, global_value) in self.globals.dfg.values_iter() { + match global_value { + Value::NumericConstant { constant, typ } => { + writeln!(f, "g{} = {typ} {constant}", id.to_u32())?; + } + Value::Instruction { instruction, .. } => { + display_instruction(&self.globals.dfg, *instruction, true, f)?; + } + Value::Global(_) => { + panic!("Value::Global should only be in the function dfg"); + } + _ => panic!("Expected only numeric constant or instruction"), + }; + } + + if self.globals.dfg.values_iter().len() > 0 { + writeln!(f)?; + } + + for function in self.functions.values() { + writeln!(f, "{function}")?; + } + Ok(()) + } +} + +impl Display for Function { + fn fmt(&self, f: &mut Formatter<'_>) -> Result { + display_function(self, f) + } +} + /// Helper function for Function's Display impl to pretty-print the function with the given formatter. -pub(crate) fn display_function(function: &Function, f: &mut Formatter) -> Result { +fn display_function(function: &Function, f: &mut Formatter) -> Result { writeln!(f, "{} fn {} {} {{", function.runtime(), function.name(), function.id())?; for block_id in function.reachable_blocks() { display_block(&function.dfg, block_id, f)?; @@ -25,17 +62,13 @@ pub(crate) fn display_function(function: &Function, f: &mut Formatter) -> Result } /// Display a single block. This will not display the block's successors. -pub(crate) fn display_block( - dfg: &DataFlowGraph, - block_id: BasicBlockId, - f: &mut Formatter, -) -> Result { +fn display_block(dfg: &DataFlowGraph, block_id: BasicBlockId, f: &mut Formatter) -> Result { let block = &dfg[block_id]; writeln!(f, " {}({}):", block_id, value_list_with_types(dfg, block.parameters()))?; for instruction in block.instructions() { - display_instruction(dfg, *instruction, f)?; + display_instruction(dfg, *instruction, false, f)?; } display_terminator(dfg, block.terminator(), f) @@ -53,6 +86,9 @@ fn value(dfg: &DataFlowGraph, id: ValueId) -> String { Value::Intrinsic(intrinsic) => intrinsic.to_string(), Value::ForeignFunction(function) => function.clone(), Value::Param { .. } | Value::Instruction { .. } => id.to_string(), + Value::Global(_) => { + format!("g{}", id.to_u32()) + } } } @@ -72,7 +108,7 @@ fn value_list(dfg: &DataFlowGraph, values: &[ValueId]) -> String { } /// Display a terminator instruction -pub(crate) fn display_terminator( +fn display_terminator( dfg: &DataFlowGraph, terminator: Option<&TerminatorInstruction>, f: &mut Formatter, @@ -107,17 +143,24 @@ pub(crate) fn display_terminator( } /// Display an arbitrary instruction -pub(crate) fn display_instruction( +fn display_instruction( dfg: &DataFlowGraph, instruction: InstructionId, + in_global_space: bool, f: &mut Formatter, ) -> Result { - // instructions are always indented within a function - write!(f, " ")?; + if !in_global_space { + // instructions are always indented within a function + write!(f, " ")?; + } let results = dfg.instruction_results(instruction); if !results.is_empty() { - write!(f, "{} = ", value_list(dfg, results))?; + let mut value_list = value_list(dfg, results); + if in_global_space { + value_list = value_list.replace('v', "g"); + } + write!(f, "{} = ", value_list)?; } display_instruction_inner(dfg, &dfg[instruction], results, f) diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index 39c63e3efcd..53f87a260c3 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -53,6 +53,9 @@ pub(crate) enum Value { /// ForeignFunction's always have the type Type::Function and have similar semantics to Function, /// other than generating different backend operations and being only accessible through Brillig. ForeignFunction(String), + + /// This Value indicates we have a reserved slot that needs to be accessed in a separate global context + Global(Type), } impl Value { @@ -64,6 +67,7 @@ impl Value { Value::Function { .. } | Value::Intrinsic { .. } | Value::ForeignFunction { .. } => { Cow::Owned(Type::Function) } + Value::Global(typ) => Cow::Borrowed(typ), } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 36955480728..7dd8fe57735 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -129,6 +129,8 @@ struct PerFunctionContext<'function> { /// True if we're currently working on the entry point function. inlining_entry: bool, + + globals: &'function Function, } /// Utility function to find out the direct calls of a function. @@ -358,7 +360,7 @@ impl InlineContext { entry_point: FunctionId, inline_no_predicates_functions: bool, functions_not_to_inline: BTreeSet, - ) -> InlineContext { + ) -> Self { let source = &ssa.functions[&entry_point]; let mut builder = FunctionBuilder::new(source.name().to_owned(), entry_point); builder.set_runtime(source.runtime()); @@ -376,9 +378,14 @@ impl InlineContext { fn inline_all(mut self, ssa: &Ssa) -> Function { let entry_point = &ssa.functions[&self.entry_point]; - let mut context = PerFunctionContext::new(&mut self, entry_point); + // let globals = self.globals; + let mut context = PerFunctionContext::new(&mut self, entry_point, &ssa.globals); context.inlining_entry = true; + for (_, value) in ssa.globals.dfg.values_iter() { + context.context.builder.current_function.dfg.make_global(value.get_type().into_owned()); + } + // The entry block is already inserted so we have to add it to context.blocks and add // its parameters here. Failing to do so would cause context.translate_block() to add // a fresh block for the entry block rather than use the existing one. @@ -422,7 +429,7 @@ impl InlineContext { ); } - let mut context = PerFunctionContext::new(self, source_function); + let mut context = PerFunctionContext::new(self, source_function, &ssa.globals); let parameters = source_function.parameters(); assert_eq!(parameters.len(), arguments.len()); @@ -442,13 +449,18 @@ impl<'function> PerFunctionContext<'function> { /// The value and block mappings for this context are initially empty except /// for containing the mapping between parameters in the source_function and /// the arguments of the destination function. - fn new(context: &'function mut InlineContext, source_function: &'function Function) -> Self { + fn new( + context: &'function mut InlineContext, + source_function: &'function Function, + globals: &'function Function, + ) -> Self { Self { context, source_function, blocks: HashMap::default(), values: HashMap::default(), inlining_entry: false, + globals, } } @@ -478,6 +490,28 @@ impl<'function> PerFunctionContext<'function> { Value::ForeignFunction(function) => { self.context.builder.import_foreign_function(function) } + Value::Global(_) => { + // TODO: Inlining the global into the function is only a temporary measure + // until Brillig gen with globals is working end to end + match &self.globals.dfg[id] { + Value::Instruction { instruction, .. } => { + let Instruction::MakeArray { elements, typ } = + &self.globals.dfg[*instruction] + else { + panic!("Only expect Instruction::MakeArray for a global"); + }; + let elements = elements + .iter() + .map(|element| self.translate_value(*element)) + .collect::>(); + self.context.builder.insert_make_array(elements, typ.clone()) + } + Value::NumericConstant { constant, typ } => { + self.context.builder.numeric_constant(*constant, *typ) + } + _ => panic!("Expected only an instruction or a numeric constant"), + } + } }; self.values.insert(id, new_value); 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 56f69a912d4..5f21e3816f0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -3,6 +3,7 @@ use std::collections::BTreeMap; use crate::ssa::{ ir::{ basic_block::BasicBlockId, + dfg::DataFlowGraph, function::{Function, FunctionId}, map::SparseMap, post_order::PostOrder, @@ -25,7 +26,7 @@ impl Ssa { let mut context = Context::default(); context.populate_functions(&self.functions); for function in self.functions.values_mut() { - context.normalize_ids(function); + context.normalize_ids(function, &self.globals.dfg); } self.functions = context.functions.into_btree(); } @@ -65,13 +66,17 @@ impl Context { } } - fn normalize_ids(&mut self, old_function: &mut Function) { + fn normalize_ids(&mut self, old_function: &mut Function, globals: &DataFlowGraph) { self.new_ids.blocks.clear(); self.new_ids.values.clear(); let new_function_id = self.new_ids.function_ids[&old_function.id()]; let new_function = &mut self.functions[new_function_id]; + for (_, value) in globals.values_iter() { + new_function.dfg.make_global(value.get_type().into_owned()); + } + let mut reachable_blocks = PostOrder::with_function(old_function).into_vec(); reachable_blocks.reverse(); @@ -192,6 +197,11 @@ impl IdMaps { } Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), + Value::Global(_) => { + // Globals are computed at compile-time and thus are expected to be remain normalized + // between SSA passes + old_value + }, } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e89d1d2a0c3..9aaa23cbd3b 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1,10 +1,11 @@ +use std::collections::BTreeMap; use std::sync::{Arc, Mutex, RwLock}; 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, LocalId, Parameters}; +use noirc_frontend::monomorphization::ast::{self, GlobalId, InlineType, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; use crate::errors::RuntimeError; @@ -71,6 +72,14 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, + /// A pseudo function that represents global values. + /// Globals are only concerned with the values and instructions (due to Instruction::MakeArray) + /// in a function's DataFlowGraph. However, in order to re-use various codegen methods + /// we need to use the same `Function` type. + pub(super) globals_context: Function, + + pub(super) globals: BTreeMap, + /// The entire monomorphized source program pub(super) program: Program, } @@ -108,8 +117,10 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); + let definitions = HashMap::default(); let mut this = Self { definitions, builder, shared_context, loops: Vec::new() }; + this.add_globals(); this.add_parameters_to_scope(parameters); this } @@ -126,9 +137,18 @@ impl<'a> FunctionContext<'a> { } else { self.builder.new_function(func.name.clone(), id, func.inline_type); } + + self.add_globals(); + self.add_parameters_to_scope(&func.parameters); } + fn add_globals(&mut self) { + for (_, value) in self.shared_context.globals_context.dfg.values_iter() { + self.builder.current_function.dfg.make_global(value.get_type().into_owned()); + } + } + /// Add each parameter to the current scope, and return the list of parameter types. /// /// The returned parameter type list will be flattened, so any struct parameters will @@ -633,6 +653,10 @@ impl<'a> FunctionContext<'a> { self.definitions.get(&id).expect("lookup: variable not defined").clone() } + pub(super) fn lookup_global(&self, id: GlobalId) -> Values { + self.shared_context.globals.get(&id).expect("lookup_global: variable not defined").clone() + } + /// Extract the given field of the tuple. Panics if the given Values is not /// a Tree::Branch or does not have enough fields. pub(super) fn get_field(tuple: Values, field_index: usize) -> Values { @@ -1022,11 +1046,45 @@ fn convert_operator(op: BinaryOpKind) -> BinaryOp { impl SharedContext { /// Create a new SharedContext for the given monomorphized program. pub(super) fn new(program: Program) -> Self { + let globals_shared_context = SharedContext::new_for_globals(); + + let globals_id = Program::global_space_id(); + + // Queue the function representing the globals space for compilation + globals_shared_context.get_or_queue_function(globals_id); + + let mut context = FunctionContext::new( + "globals".to_owned(), + &vec![], + RuntimeType::Brillig(InlineType::default()), + &globals_shared_context, + ); + let mut globals = BTreeMap::default(); + for (id, global) in program.globals.iter() { + let values = context.codegen_expression(global).unwrap(); + globals.insert(*id, values); + } + Self { functions: Default::default(), function_queue: Default::default(), function_counter: Default::default(), program, + globals_context: context.builder.current_function, + globals, + } + } + + pub(super) fn new_for_globals() -> Self { + let globals_context = Function::new_for_globals(); + + Self { + functions: Default::default(), + function_queue: Default::default(), + function_counter: Default::default(), + program: Default::default(), + globals_context, + globals: Default::default(), } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 10b8129e8db..e007cf89b59 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -121,7 +121,9 @@ pub(crate) fn generate_ssa(program: Program) -> Result { function_context.codegen_function_body(&function.body)?; } - Ok(function_context.builder.finish()) + let mut ssa = function_context.builder.finish(); + ssa.globals = context.globals_context; + Ok(ssa) } impl<'a> FunctionContext<'a> { @@ -179,6 +181,7 @@ impl<'a> FunctionContext<'a> { fn codegen_ident_reference(&mut self, ident: &ast::Ident) -> Values { match &ident.definition { ast::Definition::Local(id) => self.lookup(*id), + ast::Definition::Global(id) => self.lookup_global(*id), ast::Definition::Function(id) => self.get_or_queue_function(*id), ast::Definition::Oracle(name) => self.builder.import_foreign_function(name).into(), ast::Definition::Builtin(name) | ast::Definition::LowLevel(name) => { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 98cdc0adad9..f3474b9f4a5 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeMap, fmt::Display}; +use std::collections::BTreeMap; use acvm::acir::circuit::ErrorSelector; use iter_extended::btree_map; @@ -17,6 +17,7 @@ use noirc_frontend::hir_def::types::Type as HirType; pub(crate) struct Ssa { #[serde_as(as = "Vec<(_, _)>")] pub(crate) functions: BTreeMap, + pub(crate) globals: Function, pub(crate) main_id: FunctionId, #[serde(skip)] pub(crate) next_id: AtomicCounter, @@ -53,6 +54,9 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, + // This field should be set afterwards as globals are generated + // outside of the FunctionBuilder, which is where the `Ssa` is instantiated. + globals: Function::new_for_globals(), } } @@ -99,15 +103,6 @@ impl Ssa { } } -impl Display for Ssa { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - for function in self.functions.values() { - writeln!(f, "{function}")?; - } - Ok(()) - } -} - #[cfg(test)] mod test { use crate::ssa::ir::map::Id; diff --git a/compiler/noirc_frontend/src/ast/mod.rs b/compiler/noirc_frontend/src/ast/mod.rs index 35e57cd4528..f8a82574bee 100644 --- a/compiler/noirc_frontend/src/ast/mod.rs +++ b/compiler/noirc_frontend/src/ast/mod.rs @@ -580,12 +580,13 @@ impl std::fmt::Display for ItemVisibility { } } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, Default)] /// Represents whether the parameter is public or known only to the prover. pub enum Visibility { Public, // Constants are not allowed in the ABI for main at the moment. // Constant, + #[default] Private, /// DataBus is public input handled as private input. We use the fact that return values are properly computed by the program to avoid having them as public inputs /// it is useful for recursion and is handled by the proving system. diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index b5496507e80..77933ba9361 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -521,6 +521,10 @@ impl Value { ) } + pub(crate) fn is_closure(&self) -> bool { + matches!(self, Value::Closure(..)) + } + /// Converts any non-negative `Value` into a `FieldElement`. /// Returns `None` for negative integers and non-integral `Value`s. pub(crate) fn to_field_element(&self) -> Option { diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index c9ae3438e42..d219e8f7c2d 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -1,4 +1,4 @@ -use std::fmt::Display; +use std::{collections::BTreeMap, fmt::Display}; use acvm::FieldElement; use iter_extended::vecmap; @@ -59,6 +59,7 @@ impl Expression { #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum Definition { Local(LocalId), + Global(GlobalId), Function(FuncId), Builtin(String), LowLevel(String), @@ -71,6 +72,10 @@ pub enum Definition { #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub struct LocalId(pub u32); +/// A function ID corresponds directly to an index of `Program::globals` +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)] +pub struct GlobalId(pub u32); + /// A function ID corresponds directly to an index of `Program::functions` #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] pub struct FuncId(pub u32); @@ -322,13 +327,14 @@ impl Type { } } -#[derive(Debug, Clone, Hash)] +#[derive(Debug, Clone, Hash, Default)] pub struct Program { pub functions: Vec, pub function_signatures: Vec, pub main_function_signature: FunctionSignature, pub return_location: Option, pub return_visibility: Visibility, + pub globals: BTreeMap, pub debug_variables: DebugVariables, pub debug_functions: DebugFunctions, pub debug_types: DebugTypes, @@ -342,6 +348,7 @@ impl Program { main_function_signature: FunctionSignature, return_location: Option, return_visibility: Visibility, + globals: BTreeMap, debug_variables: DebugVariables, debug_functions: DebugFunctions, debug_types: DebugTypes, @@ -352,6 +359,7 @@ impl Program { main_function_signature, return_location, return_visibility, + globals, debug_variables, debug_functions, debug_types, @@ -370,6 +378,13 @@ impl Program { FuncId(0) } + /// Globals are expected to be generated within a different context than + /// all other functions in the program. Thus, the globals space has the same + /// ID as `main`, although we should never expect a clash in these IDs. + pub fn global_space_id() -> FuncId { + FuncId(0) + } + /// Takes a function body by replacing it with `false` and /// returning the previous value pub fn take_function_body(&mut self, function: FuncId) -> Expression { diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index c3a32f979d4..b0c8744ea8f 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -25,6 +25,7 @@ use crate::{ Kind, Type, TypeBinding, TypeBindings, }; use acvm::{acir::AcirField, FieldElement}; +use ast::GlobalId; use fxhash::FxHashMap as HashMap; use iter_extended::{btree_map, try_vecmap, vecmap}; use noirc_errors::Location; @@ -70,6 +71,11 @@ struct Monomorphizer<'interner> { /// confuse users. locals: HashMap, + /// Globals are keyed by their unique ID because they are never duplicated during monomorphization. + globals: HashMap, + + finished_globals: 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, is_unconstrained, location) queue: VecDeque<( @@ -91,6 +97,7 @@ struct Monomorphizer<'interner> { lambda_envs_stack: Vec, next_local_id: u32, + next_global_id: u32, next_function_id: u32, is_range_loop: bool, @@ -175,14 +182,18 @@ pub fn monomorphize_debug( let functions = vecmap(monomorphizer.finished_functions, |(_, f)| f); let FuncMeta { return_visibility, .. } = monomorphizer.interner.function_meta(&main); + let globals = monomorphizer.finished_globals.into_iter().collect::>(); + let (debug_variables, debug_functions, debug_types) = monomorphizer.debug_type_tracker.extract_vars_and_types(); + let program = Program::new( functions, func_sigs, function_sig, monomorphizer.return_location, *return_visibility, + globals, debug_variables, debug_functions, debug_types, @@ -195,9 +206,12 @@ impl<'interner> Monomorphizer<'interner> { Monomorphizer { functions: HashMap::default(), locals: HashMap::default(), + globals: HashMap::default(), + finished_globals: HashMap::default(), queue: VecDeque::new(), finished_functions: BTreeMap::new(), next_local_id: 0, + next_global_id: 0, next_function_id: 0, interner, lambda_envs_stack: Vec::new(), @@ -220,6 +234,12 @@ impl<'interner> Monomorphizer<'interner> { ast::FuncId(id) } + fn next_global_id(&mut self) -> GlobalId { + let id = self.next_global_id; + self.next_global_id += 1; + GlobalId(id) + } + fn lookup_local(&mut self, id: node_interner::DefinitionId) -> Option { self.locals.get(&id).copied().map(Definition::Local) } @@ -927,17 +947,7 @@ impl<'interner> Monomorphizer<'interner> { } } DefinitionKind::Global(global_id) => { - let global = self.interner.get_global(*global_id); - - let expr = if let GlobalValue::Resolved(value) = global.value.clone() { - value - .into_hir_expression(self.interner, global.location) - .map_err(MonomorphizationError::InterpreterError)? - } else { - unreachable!("All global values should be resolved at compile time and before monomorphization"); - }; - - self.expr(expr)? + self.global_ident(*global_id, definition.name.clone(), &typ, ident.location)? } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { Some(expr) => expr, @@ -984,6 +994,66 @@ impl<'interner> Monomorphizer<'interner> { Ok(ident) } + fn global_ident( + &mut self, + global_id: node_interner::GlobalId, + name: String, + typ: &HirType, + location: Location, + ) -> Result { + let global = self.interner.get_global(global_id); + let id = global.id; + let expr = if let Some(seen_global) = self.globals.get(&id) { + let typ = Self::convert_type(typ, location)?; + let ident = ast::Ident { + location: Some(location), + definition: Definition::Global(*seen_global), + mutable: false, + name, + typ, + }; + ast::Expression::Ident(ident) + } else { + let (expr, is_closure) = if let GlobalValue::Resolved(value) = global.value.clone() { + let is_closure = value.is_closure(); + let expr = value + .into_hir_expression(self.interner, global.location) + .map_err(MonomorphizationError::InterpreterError)?; + (expr, is_closure) + } else { + unreachable!("All global values should be resolved at compile time and before monomorphization"); + }; + + let expr = self.expr(expr)?; + + // Globals are meant to be computed at compile time and are stored in their own context to be shared across functions. + // Closures are defined as normal functions among all SSA functions and later need to be defunctionalized. + // Thus, this means we would have to re-define any global closures. + // The effect of defunctionalization would be the same if we were redefining a global closure or a local closure + // just with an extra step of indirection through a global variable. + // For simplicity, we chose to instead inline closures at their callsite as we do not expect + // placing a closure in the global context to change the final result of the program. + if !is_closure { + let new_id = self.next_global_id(); + self.globals.insert(id, new_id); + + self.finished_globals.insert(new_id, expr); + let typ = Self::convert_type(typ, location)?; + let ident = ast::Ident { + location: Some(location), + definition: Definition::Global(new_id), + mutable: false, + name, + typ, + }; + ast::Expression::Ident(ident) + } else { + expr + } + }; + Ok(expr) + } + /// Convert a non-tuple/struct type to a monomorphized type fn convert_type(typ: &HirType, location: Location) -> Result { let typ = typ.follow_bindings_shallow(); diff --git a/compiler/noirc_frontend/src/monomorphization/printer.rs b/compiler/noirc_frontend/src/monomorphization/printer.rs index 9c1072a4117..25ac1336075 100644 --- a/compiler/noirc_frontend/src/monomorphization/printer.rs +++ b/compiler/noirc_frontend/src/monomorphization/printer.rs @@ -293,6 +293,7 @@ impl Display for Definition { fn fmt(&self, f: &mut Formatter) -> std::fmt::Result { match self { Definition::Local(id) => write!(f, "l{}", id.0), + Definition::Global(id) => write!(f, "g{}", id.0), Definition::Function(id) => write!(f, "f{}", id), Definition::Builtin(name) => write!(f, "{name}"), Definition::LowLevel(name) => write!(f, "{name}"), diff --git a/test_programs/execution_success/global_var_regression_simple/Nargo.toml b/test_programs/execution_success/global_var_regression_simple/Nargo.toml new file mode 100644 index 00000000000..50b4902de22 --- /dev/null +++ b/test_programs/execution_success/global_var_regression_simple/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "global_var_regression_simple" +type = "bin" +authors = [""] + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/global_var_regression_simple/Prover.toml b/test_programs/execution_success/global_var_regression_simple/Prover.toml new file mode 100644 index 00000000000..2c1854573a4 --- /dev/null +++ b/test_programs/execution_success/global_var_regression_simple/Prover.toml @@ -0,0 +1,2 @@ +x = 1 +y = 2 diff --git a/test_programs/execution_success/global_var_regression_simple/src/main.nr b/test_programs/execution_success/global_var_regression_simple/src/main.nr new file mode 100644 index 00000000000..b1bf753a73c --- /dev/null +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -0,0 +1,25 @@ +global EXPONENTIATE: [[Field; 2]; 2] = [[1, 1], [0, 0]]; + +fn main(x: Field, y: pub Field) { + let mut acc: Field = 0; + for i in 0..2 { + for j in 0..2 { + acc += EXPONENTIATE[i][j]; + } + } + assert(!acc.lt(x)); + assert(x != y); + + dummy_again(x, y); +} + +fn dummy_again(x: Field, y: Field) { + let mut acc: Field = 0; + for i in 0..2 { + for j in 0..2 { + acc += EXPONENTIATE[i][j]; + } + } + assert(!acc.lt(x)); + assert(x != y); +}