From c7d8009072837ee105d8337eb273b88893235949 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 02:46:27 +0000 Subject: [PATCH 01/19] cleanup global generation with GlobalsBuilder --- compiler/noirc_evaluator/src/acir/mod.rs | 3 + .../src/brillig/brillig_gen/brillig_block.rs | 7 +- .../brillig/brillig_gen/variable_liveness.rs | 2 +- compiler/noirc_evaluator/src/ssa.rs | 2 +- .../check_for_underconstrained_values.rs | 6 +- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 4 + .../noirc_evaluator/src/ssa/ir/printer.rs | 17 +- compiler/noirc_evaluator/src/ssa/ir/value.rs | 3 + .../noirc_evaluator/src/ssa/opt/inlining.rs | 4 + .../src/ssa/opt/normalize_value_ids.rs | 3 + .../src/ssa/ssa_gen/context.rs | 175 +++++++++++++++++- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 5 +- .../src/ssa/ssa_gen/program.rs | 41 +++- .../src/monomorphization/ast.rs | 10 +- .../src/monomorphization/mod.rs | 76 ++++++-- .../src/monomorphization/printer.rs | 1 + .../global_var_regression_simple/Nargo.toml | 6 + .../global_var_regression_simple/Prover.toml | 2 + .../global_var_regression_simple/src/main.nr | 28 +++ 19 files changed, 365 insertions(+), 30 deletions(-) create mode 100644 test_programs/execution_success/global_var_regression_simple/Nargo.toml create mode 100644 test_programs/execution_success/global_var_regression_simple/Prover.toml create mode 100644 test_programs/execution_success/global_var_regression_simple/src/main.nr diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index a82c54d8ce6..02b94a1c550 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1891,6 +1891,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 66cc213a986..91a05b78163 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,7 @@ 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]) } }, @@ -1575,6 +1575,11 @@ impl<'block> BrilligBlock<'block> { let value = &dfg[value_id]; match value { + Value::Global(_) => { + // let variable = *self.function_context.globals.get(&value_id).unwrap_or_else(|| panic!("ICE: Global value not found in cache {value_id}")); + // variable + panic!("globals not handled, these should be 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..98db967e5dd 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -53,7 +53,7 @@ pub(crate) fn collect_variables_of_value( let value = &dfg[value_id]; match value { - Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } => { + 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. diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 2500b8685a1..d8d990c2ef9 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -490,7 +490,7 @@ impl SsaBuilder { } }; if print_ssa_pass { - self.ssa.normalize_ids(); + // self.ssa.normalize_ids(); println!("After {msg}:\n{}", self.ssa); } self 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..be375dcc3b4 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 f531e8307f1..d3c0b9257ce 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -297,6 +297,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) { diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 598f7c27eff..53a575f7396 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -43,9 +43,9 @@ pub(crate) fn display_block( /// Specialize displaying value ids so that if they refer to a numeric /// constant or a function we print those directly. -fn value(function: &Function, id: ValueId) -> String { - let id = function.dfg.resolve(id); - match &function.dfg[id] { +pub(crate) fn value(dfg: &DataFlowGraph, id: ValueId) -> String { + let id = dfg.resolve(id); + match &dfg[id] { Value::NumericConstant { constant, typ } => { format!("{typ} {constant}") } @@ -53,13 +53,16 @@ fn value(function: &Function, id: ValueId) -> String { Value::Intrinsic(intrinsic) => intrinsic.to_string(), Value::ForeignFunction(function) => function.clone(), Value::Param { .. } | Value::Instruction { .. } => id.to_string(), + Value::Global(_) => { + format!("@{id}") + } } } /// Display each value along with its type. E.g. `v0: Field, v1: u64, v2: u1` fn value_list_with_types(function: &Function, values: &[ValueId]) -> String { vecmap(values, |id| { - let value = value(function, *id); + let value = value(&function.dfg, *id); let typ = function.dfg.type_of_value(*id); format!("{value}: {typ}") }) @@ -68,7 +71,7 @@ fn value_list_with_types(function: &Function, values: &[ValueId]) -> String { /// Display each value separated by a comma fn value_list(function: &Function, values: &[ValueId]) -> String { - vecmap(values, |id| value(function, *id)).join(", ") + vecmap(values, |id| value(&function.dfg, *id)).join(", ") } /// Display a terminator instruction @@ -90,7 +93,7 @@ pub(crate) fn display_terminator( writeln!( f, " jmpif {} then: {}, else: {}", - value(function, *condition), + value(&function.dfg, *condition), then_destination, else_destination ) @@ -129,7 +132,7 @@ fn display_instruction_inner( results: &[ValueId], f: &mut Formatter, ) -> Result { - let show = |id| value(function, id); + let show = |id| value(&function.dfg, id); match instruction { Instruction::Binary(binary) => { diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index ec7a8e25246..6325438b569 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -53,6 +53,8 @@ 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), + + Global(Type), } impl Value { @@ -64,6 +66,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..4d8eb482c48 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -465,6 +465,7 @@ impl<'function> PerFunctionContext<'function> { let new_value = match &self.source_function.dfg[id] { value @ Value::Instruction { .. } => { + unreachable!("All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value {id} = {value:?}") } value @ Value::Param { .. } => { @@ -478,6 +479,9 @@ impl<'function> PerFunctionContext<'function> { Value::ForeignFunction(function) => { self.context.builder.import_foreign_function(function) } + Value::Global(_) => { + id + } }; 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..7bd6d82de80 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -192,6 +192,9 @@ impl IdMaps { } Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), + Value::Global(_) => { + panic!("handle globals in value ids normalizing") + }, } } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e89d1d2a0c3..8c9a0a9505d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1,16 +1,21 @@ +use std::collections::BTreeMap; use std::sync::{Arc, Mutex, RwLock}; use acvm::{acir::AcirField, FieldElement}; -use iter_extended::vecmap; +use iter_extended::{try_vecmap, 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, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; +use serde::{Deserialize, Serialize}; use crate::errors::RuntimeError; use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::basic_block::BasicBlockId; +use crate::ssa::ir::dfg::DataFlowGraph; use crate::ssa::ir::function::FunctionId as IrFunctionId; +use crate::ssa::ir::value::Value as IrValue; +use crate::ssa::ir::value::ValueId as IrValueId; use crate::ssa::ir::function::{Function, RuntimeType}; use crate::ssa::ir::instruction::BinaryOp; use crate::ssa::ir::instruction::Instruction; @@ -71,6 +76,18 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, + // pub(super) values: DenseMap, + + // instructions: DenseMap, + + // results: HashMap>, + + // constants: HashMap<(FieldElement, NumericType), IrValueId>, + + pub(super) globals_builder: GlobalsBuilder, + + pub(super) globals: BTreeMap, + /// The entire monomorphized source program pub(super) program: Program, } @@ -108,6 +125,17 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); + + + for (_, value) in shared_context.globals_builder.dfg.values_iter() { + // TODO: clean this up, we do not need to differentiate based off instruction here + if let IrValue::Instruction { instruction, typ, .. } = value { + builder.current_function.dfg.make_global(typ.clone()); + } else { + builder.current_function.dfg.make_global(value.get_type().into_owned()); + } + } + let definitions = HashMap::default(); let mut this = Self { definitions, builder, shared_context, loops: Vec::new() }; this.add_parameters_to_scope(parameters); @@ -126,6 +154,11 @@ impl<'a> FunctionContext<'a> { } else { self.builder.new_function(func.name.clone(), id, func.inline_type); } + + for (_, value) in self.shared_context.globals_builder.dfg.values_iter() { + self.builder.current_function.dfg.make_global(value.get_type().into_owned()); + } + self.add_parameters_to_scope(&func.parameters); } @@ -633,6 +666,10 @@ impl<'a> FunctionContext<'a> { self.definitions.get(&id).expect("lookup: variable not defined").clone() } + pub(super) fn lookup_global(&self, id: GlobalId) -> ValueId { + 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 +1059,25 @@ 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 mut globals_builder = GlobalsBuilder::default(); + let mut globals = BTreeMap::default(); + for (id, global) in program.globals.iter() { + let values = globals_builder.codegen_expression(global).unwrap(); + + let value = match values.into_leaf() { + Value::Normal(value) => value, + _ => panic!("Must have Value::Normal for globals"), + }; + globals.insert(*id, value); + } + dbg!(globals.clone()); Self { functions: Default::default(), function_queue: Default::default(), function_counter: Default::default(), program, + globals_builder, + globals } } @@ -1068,3 +1119,123 @@ pub(super) enum LValue { MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, Dereference { reference: Values }, } + +#[derive(Default, Serialize, Deserialize)] +pub(crate) struct GlobalsBuilder { + pub(super) dfg: DataFlowGraph, +} + +impl GlobalsBuilder { + fn codegen_expression(&mut self, expr: &ast::Expression) -> Result { + match expr { + ast::Expression::Literal(literal) => self.codegen_literal(literal), + _ => { + panic!("Only expect literals for global expressions") + } + } + } + + fn codegen_literal(&mut self, literal: &ast::Literal) -> Result { + match literal { + ast::Literal::Array(array) => { + let elements = self.codegen_array_elements(&array.contents)?; + + let typ = FunctionContext::convert_type(&array.typ).flatten(); + Ok(match array.typ { + ast::Type::Array(_, _) => { + self.codegen_array_checked(elements, typ[0].clone())? + } + _ => unreachable!("ICE: unexpected array literal type, got {}", array.typ), + }) + } + ast::Literal::Slice(_) => todo!(), + ast::Literal::Integer(value, negative, typ, location) => { + let numeric_type = FunctionContext::convert_non_tuple_type(typ).unwrap_numeric(); + + let value = (*value).into(); + + if let Some(range) = numeric_type.value_is_outside_limits(value, *negative) { + return Err(RuntimeError::IntegerOutOfBounds { + value: if *negative { -value } else { value }, + typ: numeric_type, + range, + call_stack: vec![*location], + }); + } + + let value = if *negative { + match numeric_type { + NumericType::NativeField => -value, + NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => { + let base = 1_u128 << bit_size; + FieldElement::from(base) - value + } + } + } else { + value + }; + + Ok(self.dfg.make_constant(value, numeric_type).into()) + } + ast::Literal::Bool(_) => todo!(), + ast::Literal::Unit => todo!(), + ast::Literal::Str(_) => todo!(), + ast::Literal::FmtStr(_, _, _) => todo!(), + } + } + + fn codegen_array_elements( + &mut self, + elements: &[ast::Expression], + ) -> Result, RuntimeError> { + try_vecmap(elements, |element| { + let value = self.codegen_expression(element)?; + Ok((value, element.is_array_or_slice_literal())) + }) + } + + fn codegen_array_checked( + &mut self, + elements: Vec<(Values, bool)>, + typ: Type, + ) -> Result { + if typ.is_nested_slice() { + return Err(RuntimeError::NestedSlice { call_stack: vec![] }); + } + Ok(self.codegen_array(elements, typ)) + } + + fn codegen_array(&mut self, elements: Vec<(Values, bool)>, typ: Type) -> Values { + let mut array = im::Vector::new(); + + for (element, _) in elements { + element.for_each(|element| { + // let element = element.eval(self); + let element = match element { + Value::Normal(value) => value, + _ => panic!("Must have Value::Normal for globals"), + }; + array.push_back(element); + }); + } + + self.insert_make_array(array, typ).into() + } + + + /// Insert a `make_array` instruction to create a new array or slice. + /// Returns the new array value. Expects `typ` to be an array or slice type. + fn insert_make_array( + &mut self, + elements: im::Vector, + typ: Type, + ) -> ValueId { + assert!(matches!(typ, Type::Array(..) | Type::Slice(_))); + + let id = self.dfg.make_instruction(Instruction::MakeArray { elements, typ: typ.clone() }, None); + self.dfg.instruction_results(id)[0] + } + +} + + diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index b3a8ecf1314..6acd1a73eee 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -120,7 +120,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_builder; + Ok(ssa) } impl<'a> FunctionContext<'a> { @@ -178,6 +180,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).into(), 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..00081847a1c 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -6,17 +6,19 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use crate::ssa::ir::{ - function::{Function, FunctionId}, - map::AtomicCounter, + function::{Function, FunctionId}, instruction::Instruction, map::AtomicCounter, printer::{display_instruction, value}, value::Value }; use noirc_frontend::hir_def::types::Type as HirType; +use super::context::GlobalsBuilder; + /// Contains the entire SSA representation of the program. #[serde_as] #[derive(Serialize, Deserialize)] pub(crate) struct Ssa { #[serde_as(as = "Vec<(_, _)>")] pub(crate) functions: BTreeMap, + pub(crate) globals: GlobalsBuilder, pub(crate) main_id: FunctionId, #[serde(skip)] pub(crate) next_id: AtomicCounter, @@ -53,6 +55,7 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, + globals: GlobalsBuilder::default(), } } @@ -101,6 +104,40 @@ impl Ssa { impl Display for Ssa { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + writeln!(f, "globals: ")?; + let show = |id| value(&self.globals.dfg, id); + + for (id, value) in self.globals.dfg.values_iter() { + write!(f, "@{} = ", id)?; + match value { + Value::NumericConstant { constant, typ } => { + writeln!(f, "{typ} {constant}")?; + } + Value::Instruction { instruction, .. } => { + match &self.globals.dfg[*instruction] { + Instruction::MakeArray { elements, typ } => { + write!(f, "make_array [")?; + + for (i, element) in elements.iter().enumerate() { + if i != 0 { + write!(f, ", ")?; + } + write!(f, "{}", show(*element))?; + } + + writeln!(f, "] : {typ}")?; + } + _ => panic!("Expected MakeArray"), + } + } + Value::Global(_) => { + panic!("we should only have these in the function values map"); + } + _ => panic!("Expected only numeric const or array"), + }; + } + writeln!(f, "")?; + for function in self.functions.values() { writeln!(f, "{function}")?; } diff --git a/compiler/noirc_frontend/src/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index c9ae3438e42..ee9174dd916 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); @@ -329,6 +334,7 @@ pub struct Program { 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, diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index fbc80cc0ac9..0e69bae5c1b 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,8 +97,9 @@ struct Monomorphizer<'interner> { lambda_envs_stack: Vec, next_local_id: u32, + next_global_id: u32, next_function_id: u32, - + is_range_loop: bool, return_location: Option, @@ -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) } @@ -928,18 +948,50 @@ 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)? + let id = global.id; + let name = definition.name.clone(); + if let Some(seen_global) = self.globals.get(&id) { + let typ = Self::convert_type(&typ, ident.location)?; + let ident = ast::Ident { + location: Some(ident.location), + definition: Definition::Global(*seen_global), + mutable: false, + name, + typ, + }; + let expr = ast::Expression::Ident(ident); + expr } else { - let let_ = self.interner.get_global_let_statement(*global_id).expect( - "Globals should have a corresponding let statement by monomorphization", - ); - let_.expression - }; - self.expr(expr)? + let expr = if let GlobalValue::Resolved(value) = global.value.clone() { + value + .into_hir_expression(self.interner, global.location) + .map_err(MonomorphizationError::InterpreterError)? + } else { + dbg!("got here"); + let let_ = self.interner.get_global_let_statement(*global_id).expect( + "Globals should have a corresponding let statement by monomorphization", + ); + let_.expression + }; + + let expr = self.expr(expr)?; + 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, ident.location)?; + let ident = ast::Ident { + location: Some(ident.location), + definition: Definition::Global(new_id), + mutable: false, + name, + typ, + }; + let expr = ast::Expression::Ident(ident); + + expr + } } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { Some(expr) => expr, 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..94ab0e36baf --- /dev/null +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -0,0 +1,28 @@ +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); +} \ No newline at end of file From 4b19a7b9c8f0808aa3d2756c2185e98f14691868 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 05:12:55 +0000 Subject: [PATCH 02/19] all tests paassing --- compiler/noirc_evaluator/src/ssa.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 2 + .../noirc_evaluator/src/ssa/opt/inlining.rs | 43 ++++-- .../src/ssa/opt/normalize_value_ids.rs | 16 +- .../src/ssa/ssa_gen/context.rs | 138 ++++++++++++++---- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- .../src/ssa/ssa_gen/program.rs | 2 +- .../noirc_frontend/src/hir/comptime/value.rs | 4 + .../src/monomorphization/mod.rs | 62 +++++--- 9 files changed, 202 insertions(+), 69 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index d8d990c2ef9..2500b8685a1 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -490,7 +490,7 @@ impl SsaBuilder { } }; if print_ssa_pass { - // self.ssa.normalize_ids(); + self.ssa.normalize_ids(); println!("After {msg}:\n{}", self.ssa); } self diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index d3c0b9257ce..d58427d893d 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -565,6 +565,8 @@ impl DataFlowGraph { } _ => false, }, + // TODO: make this true, + Value::Global(_) => false, _ => true, } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 4d8eb482c48..18f771e91f9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -17,7 +17,7 @@ use crate::ssa::{ instruction::{Instruction, InstructionId, TerminatorInstruction}, value::{Value, ValueId}, }, - ssa_gen::Ssa, + ssa_gen::{context::GlobalsBuilder, Ssa}, }; use fxhash::FxHashMap as HashMap; @@ -80,7 +80,7 @@ impl Ssa { /// This works using an internal FunctionBuilder to build a new main function from scratch. /// Doing it this way properly handles importing instructions between functions and lets us /// reuse the existing API at the cost of essentially cloning each of main's instructions. -struct InlineContext { +struct InlineContext<'global> { recursion_level: u32, builder: FunctionBuilder, @@ -98,6 +98,8 @@ struct InlineContext { // These are the functions of the program that we shouldn't inline. functions_not_to_inline: BTreeSet, + + globals: &'global GlobalsBuilder, } /// The per-function inlining context contains information that is only valid for one function. @@ -105,13 +107,13 @@ 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> { +struct PerFunctionContext<'function, 'global> { /// The source function is the function we're currently inlining into the function being built. source_function: &'function Function, /// The shared inlining context for all functions. This notably contains the FunctionBuilder used /// to build the function we're inlining into. - context: &'function mut InlineContext, + context: &'function mut InlineContext<'global>, /// Maps ValueIds in the function being inlined to the new ValueIds to use in the function /// being inlined into. This mapping also contains the mapping from parameter values to @@ -347,18 +349,18 @@ fn compute_function_interface_cost(func: &Function) -> usize { func.parameters().len() + func.returns().len() } -impl InlineContext { +impl<'global> InlineContext<'global> { /// Create a new context object for the function inlining pass. /// This starts off with an empty mapping of instructions for main's parameters. /// The function being inlined into will always be the main function, although it is /// actually a copy that is created in case the original main is still needed from a function /// that could not be inlined calling it. fn new( - ssa: &Ssa, + ssa: &'global Ssa, 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()); @@ -369,6 +371,7 @@ impl InlineContext { call_stack: CallStackId::root(), inline_no_predicates_functions, functions_not_to_inline, + globals: &ssa.globals, } } @@ -379,6 +382,10 @@ impl InlineContext { let mut context = PerFunctionContext::new(&mut self, entry_point); 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. @@ -437,12 +444,12 @@ impl InlineContext { } } -impl<'function> PerFunctionContext<'function> { +impl<'function, 'global> PerFunctionContext<'function, 'global> { /// Create a new PerFunctionContext from the source 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<'global>, source_function: &'function Function) -> Self { Self { context, source_function, @@ -465,7 +472,6 @@ impl<'function> PerFunctionContext<'function> { let new_value = match &self.source_function.dfg[id] { value @ Value::Instruction { .. } => { - unreachable!("All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value {id} = {value:?}") } value @ Value::Param { .. } => { @@ -480,7 +486,22 @@ impl<'function> PerFunctionContext<'function> { self.context.builder.import_foreign_function(function) } Value::Global(_) => { - id + // TODO: Inlining the global into the function is only a temporary measure + // until Brillig gen with globals is working end to end + let new_id = match &self.context.globals.dfg[id] { + Value::Instruction { instruction, .. } => { + let Instruction::MakeArray { elements, typ } = &self.context.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"), + }; + new_id } }; 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 7bd6d82de80..5ad5d4f70f7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -8,7 +8,7 @@ use crate::ssa::{ post_order::PostOrder, value::{Value, ValueId}, }, - ssa_gen::Ssa, + ssa_gen::{context::GlobalsBuilder, Ssa}, }; use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; @@ -25,7 +25,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); } self.functions = context.functions.into_btree(); } @@ -60,18 +60,22 @@ impl Context { for (id, function) in functions { self.functions.insert_with_id(|new_id| { self.new_ids.function_ids.insert(*id, new_id); - Function::clone_signature(new_id, function) + Function::clone_signature(new_id, function) }); } } - fn normalize_ids(&mut self, old_function: &mut Function) { + fn normalize_ids(&mut self, old_function: &mut Function, globals: &GlobalsBuilder) { 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.dfg.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(); @@ -193,7 +197,9 @@ impl IdMaps { Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), Value::Global(_) => { - panic!("handle globals in value ids normalizing") + // 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 8c9a0a9505d..b79753494a5 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -76,17 +76,9 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, - // pub(super) values: DenseMap, - - // instructions: DenseMap, - - // results: HashMap>, - - // constants: HashMap<(FieldElement, NumericType), IrValueId>, - pub(super) globals_builder: GlobalsBuilder, - pub(super) globals: BTreeMap, + pub(super) globals: BTreeMap, /// The entire monomorphized source program pub(super) program: Program, @@ -126,14 +118,8 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); - for (_, value) in shared_context.globals_builder.dfg.values_iter() { - // TODO: clean this up, we do not need to differentiate based off instruction here - if let IrValue::Instruction { instruction, typ, .. } = value { - builder.current_function.dfg.make_global(typ.clone()); - } else { - builder.current_function.dfg.make_global(value.get_type().into_owned()); - } + builder.current_function.dfg.make_global(value.get_type().into_owned()); } let definitions = HashMap::default(); @@ -666,7 +652,7 @@ impl<'a> FunctionContext<'a> { self.definitions.get(&id).expect("lookup: variable not defined").clone() } - pub(super) fn lookup_global(&self, id: GlobalId) -> ValueId { + pub(super) fn lookup_global(&self, id: GlobalId) -> Values { self.shared_context.globals.get(&id).expect("lookup_global: variable not defined").clone() } @@ -1062,22 +1048,18 @@ impl SharedContext { let mut globals_builder = GlobalsBuilder::default(); let mut globals = BTreeMap::default(); for (id, global) in program.globals.iter() { + dbg!(global.clone()); let values = globals_builder.codegen_expression(global).unwrap(); - - let value = match values.into_leaf() { - Value::Normal(value) => value, - _ => panic!("Must have Value::Normal for globals"), - }; - globals.insert(*id, value); + globals.insert(*id, values); } - dbg!(globals.clone()); + Self { functions: Default::default(), function_queue: Default::default(), function_counter: Default::default(), program, globals_builder, - globals + globals, } } @@ -1122,19 +1104,89 @@ pub(super) enum LValue { #[derive(Default, Serialize, Deserialize)] pub(crate) struct GlobalsBuilder { - pub(super) dfg: DataFlowGraph, + pub(crate) dfg: DataFlowGraph, + + #[serde(skip)] + definitions: HashMap, } impl GlobalsBuilder { fn codegen_expression(&mut self, expr: &ast::Expression) -> Result { match expr { + ast::Expression::Ident(ident) => Ok(self.codegen_ident(ident)), ast::Expression::Literal(literal) => self.codegen_literal(literal), + ast::Expression::Block(block) => self.codegen_block(block), + ast::Expression::Let(let_expr) => self.codegen_let(let_expr), + ast::Expression::Tuple(tuple) => self.codegen_tuple(tuple), + _ => { + panic!("Only expected literals for global expressions but got {expr}") + } + } + } + + /// Looks up the value of a given local variable. Expects the variable to have + /// been previously defined or panics otherwise. + pub(super) fn lookup(&self, id: LocalId) -> Values { + self.definitions.get(&id).expect("lookup: variable not defined").clone() + } + + fn codegen_ident(&mut self, ident: &ast::Ident) -> Values { + match &ident.definition { + ast::Definition::Local(id) => self.lookup(*id), + // ast::Definition::Function(id) => self.get_or_queue_function(*id), _ => { - panic!("Only expect literals for global expressions") + panic!("Expected only Definition::Local but got {ident:#?}"); } } } + /// Retrieves the given function, adding it to the function queue + /// if it is not yet compiled. + // pub(super) fn get_or_queue_function(&mut self, id: FuncId) -> Values { + // let function = self.shared_context.get_or_queue_function(id); + // self.builder.import_function(function).into() + // } + + fn unit_value() -> Values { + Values::empty() + } + + fn codegen_block(&mut self, block: &[ast::Expression]) -> Result { + let mut result = Self::unit_value(); + for expr in block { + result = self.codegen_expression(expr)?; + } + Ok(result) + } + + fn codegen_let(&mut self, let_expr: &ast::Let) -> Result { + assert_eq!(let_expr.mutable, false, "Expected global let expression to be immutable"); + let mut values = self.codegen_expression(&let_expr.expression)?; + + values = values.map(|value| { + let value = match value { + Value::Normal(value) => value, + _ => panic!("Must have Value::Normal for globals"), + }; + + Tree::Leaf(Value::Normal(value)) + }); + + self.define(let_expr.id, values); + Ok(Self::unit_value()) + } + + fn codegen_tuple(&mut self, tuple: &[ast::Expression]) -> Result { + Ok(Tree::Branch(try_vecmap(tuple, |expr| self.codegen_expression(expr))?)) + } + + /// Define a local variable to be some Values that can later be retrieved + /// by calling self.lookup(id) + pub(super) fn define(&mut self, id: LocalId, value: Values) { + let existing = self.definitions.insert(id, value); + assert!(existing.is_none(), "Variable {id:?} was defined twice in ssa-gen pass"); + } + fn codegen_literal(&mut self, literal: &ast::Literal) -> Result { match literal { ast::Literal::Array(array) => { @@ -1148,7 +1200,22 @@ impl GlobalsBuilder { _ => unreachable!("ICE: unexpected array literal type, got {}", array.typ), }) } - ast::Literal::Slice(_) => todo!(), + ast::Literal::Slice(array) => { + let elements = self.codegen_array_elements(&array.contents)?; + + let typ = FunctionContext::convert_type(&array.typ).flatten(); + Ok(match array.typ { + ast::Type::Slice(_) => { + // let slice_length = + // self.builder.length_constant(array.contents.len() as u128); + let slice_length = self.dfg.make_constant(array.contents.len().into(), NumericType::length_type()); + let slice_contents = + self.codegen_array_checked(elements, typ[1].clone())?; + Tree::Branch(vec![slice_length.into(), slice_contents]) + } + _ => unreachable!("ICE: unexpected slice literal type, got {}", array.typ), + }) + } ast::Literal::Integer(value, negative, typ, location) => { let numeric_type = FunctionContext::convert_non_tuple_type(typ).unwrap_numeric(); @@ -1177,13 +1244,24 @@ impl GlobalsBuilder { Ok(self.dfg.make_constant(value, numeric_type).into()) } - ast::Literal::Bool(_) => todo!(), + ast::Literal::Bool(value) => { + Ok(self.dfg.make_constant((*value).into(), NumericType::bool()).into()) + } ast::Literal::Unit => todo!(), - ast::Literal::Str(_) => todo!(), + ast::Literal::Str(string) => Ok(self.codegen_string(string)), ast::Literal::FmtStr(_, _, _) => todo!(), } } + fn codegen_string(&mut self, string: &str) -> Values { + let elements = vecmap(string.as_bytes(), |byte| { + let char = self.dfg.make_constant((*byte as u128).into(), NumericType::char()); + (char.into(), false) + }); + let typ = FunctionContext::convert_non_tuple_type(&ast::Type::String(elements.len() as u32)); + self.codegen_array(elements, typ) + } + fn codegen_array_elements( &mut self, elements: &[ast::Expression], diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 6acd1a73eee..73b60f95030 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -180,7 +180,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).into(), + 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 00081847a1c..fefcdff844e 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -104,7 +104,7 @@ impl Ssa { impl Display for Ssa { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "globals: ")?; + writeln!(f, "Globals: ")?; let show = |id| value(&self.globals.dfg, id); for (id, value) in self.globals.dfg.values_iter() { diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index e5c55e2b0be..880cafa8557 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -513,6 +513,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/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 0e69bae5c1b..705892e07ec 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -962,35 +962,57 @@ impl<'interner> Monomorphizer<'interner> { let expr = ast::Expression::Ident(ident); expr } else { - let expr = if let GlobalValue::Resolved(value) = global.value.clone() { - value + let (expr, is_function) = if let GlobalValue::Resolved(value) = global.value.clone() { + dbg!(value.is_closure()); + dbg!(value.clone()); + let is_function = value.is_closure(); + let expr = value .into_hir_expression(self.interner, global.location) - .map_err(MonomorphizationError::InterpreterError)? + .map_err(MonomorphizationError::InterpreterError)?; + (expr, is_function) } else { - dbg!("got here"); let let_ = self.interner.get_global_let_statement(*global_id).expect( "Globals should have a corresponding let statement by monomorphization", ); - let_.expression + // TODO: update this + (let_.expression, false) }; let expr = self.expr(expr)?; - 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, ident.location)?; - let ident = ast::Ident { - location: Some(ident.location), - definition: Definition::Global(new_id), - mutable: false, - name, - typ, - }; - let expr = ast::Expression::Ident(ident); + // let new_id = self.next_global_id(); + // self.globals.insert(id, new_id); + + if !is_function { + 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, ident.location)?; + let ident = ast::Ident { + location: Some(ident.location), + definition: Definition::Global(new_id), + mutable: false, + name, + typ, + }; + let expr = ast::Expression::Ident(ident); + + expr + } else { + expr + } - expr + // let typ = Self::convert_type(&typ, ident.location)?; + // let ident = ast::Ident { + // location: Some(ident.location), + // definition: Definition::Global(new_id), + // mutable: false, + // name, + // typ, + // }; + // let expr = ast::Expression::Ident(ident); + + // expr } } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { From c8e08d338ec085400c25740fa594b4487858cede Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 05:13:07 +0000 Subject: [PATCH 03/19] cargo fmt --- .../src/brillig/brillig_gen/brillig_block.rs | 5 ++- .../brillig/brillig_gen/variable_liveness.rs | 7 ++-- .../check_for_underconstrained_values.rs | 4 +-- compiler/noirc_evaluator/src/ssa/ir/value.rs | 17 +++++++-- .../noirc_evaluator/src/ssa/opt/inlining.rs | 16 ++++++--- .../src/ssa/opt/normalize_value_ids.rs | 2 +- .../src/ssa/ssa_gen/context.rs | 26 ++++++-------- .../src/ssa/ssa_gen/program.rs | 36 ++++++++++--------- .../src/monomorphization/mod.rs | 31 ++++++++-------- 9 files changed, 83 insertions(+), 61 deletions(-) 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 91a05b78163..3d64f8294a0 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::Global(_) => { + Value::Instruction { .. } + | Value::Param { .. } + | Value::NumericConstant { .. } + | Value::Global(_) => { unreachable!("unsupported function call type {:?}", dfg[*func]) } }, 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 98db967e5dd..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 { .. } | Value::Global(_) => { - 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/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index be375dcc3b4..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,7 @@ impl DependencyContext { } Value::Instruction { .. } | Value::NumericConstant { .. } - | Value::Param { .. } + | Value::Param { .. } | Value::Global(_) => { panic!( "calling non-function value with ID {func_id} in function {}", @@ -619,7 +619,7 @@ 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/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index 6325438b569..94bf8aec993 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -27,16 +27,27 @@ pub(crate) enum Value { /// Example, if you add two numbers together, then the resulting /// value would have position `0`, the typ would be the type /// of the operands, and the instruction would map to an add instruction. - Instruction { instruction: InstructionId, position: usize, typ: Type }, + Instruction { + instruction: InstructionId, + position: usize, + typ: Type, + }, /// This Value originates from a block parameter. Since function parameters /// are also represented as block parameters, this includes function parameters as well. /// /// position -- the index of this Value in the block parameters list - Param { block: BasicBlockId, position: usize, typ: Type }, + Param { + block: BasicBlockId, + position: usize, + typ: Type, + }, /// This Value originates from a numeric constant - NumericConstant { constant: FieldElement, typ: NumericType }, + NumericConstant { + constant: FieldElement, + typ: NumericType, + }, /// This Value refers to a function in the IR. /// Functions always have the type Type::Function. diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 18f771e91f9..0c495005fd0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -449,7 +449,10 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { /// 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<'global>, source_function: &'function Function) -> Self { + fn new( + context: &'function mut InlineContext<'global>, + source_function: &'function Function, + ) -> Self { Self { context, source_function, @@ -486,14 +489,19 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { self.context.builder.import_foreign_function(function) } Value::Global(_) => { - // TODO: Inlining the global into the function is only a temporary measure + // TODO: Inlining the global into the function is only a temporary measure // until Brillig gen with globals is working end to end let new_id = match &self.context.globals.dfg[id] { Value::Instruction { instruction, .. } => { - let Instruction::MakeArray { elements, typ } = &self.context.globals.dfg[*instruction] else { + let Instruction::MakeArray { elements, typ } = + &self.context.globals.dfg[*instruction] + else { panic!("Only expect Instruction::MakeArray for a global"); }; - let elements = elements.iter().map(|element| self.translate_value(*element)).collect::>(); + let elements = elements + .iter() + .map(|element| self.translate_value(*element)) + .collect::>(); self.context.builder.insert_make_array(elements, typ.clone()) } Value::NumericConstant { constant, typ } => { 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 5ad5d4f70f7..f6c32cc2a63 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -60,7 +60,7 @@ impl Context { for (id, function) in functions { self.functions.insert_with_id(|new_id| { self.new_ids.function_ids.insert(*id, new_id); - Function::clone_signature(new_id, function) + Function::clone_signature(new_id, function) }); } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b79753494a5..e1de29db18e 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -14,13 +14,13 @@ use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::basic_block::BasicBlockId; use crate::ssa::ir::dfg::DataFlowGraph; use crate::ssa::ir::function::FunctionId as IrFunctionId; -use crate::ssa::ir::value::Value as IrValue; -use crate::ssa::ir::value::ValueId as IrValueId; use crate::ssa::ir::function::{Function, RuntimeType}; use crate::ssa::ir::instruction::BinaryOp; use crate::ssa::ir::instruction::Instruction; use crate::ssa::ir::map::AtomicCounter; use crate::ssa::ir::types::{NumericType, Type}; +use crate::ssa::ir::value::Value as IrValue; +use crate::ssa::ir::value::ValueId as IrValueId; use crate::ssa::ir::value::ValueId; use super::value::{Tree, Value, Values}; @@ -1207,8 +1207,10 @@ impl GlobalsBuilder { Ok(match array.typ { ast::Type::Slice(_) => { // let slice_length = - // self.builder.length_constant(array.contents.len() as u128); - let slice_length = self.dfg.make_constant(array.contents.len().into(), NumericType::length_type()); + // self.builder.length_constant(array.contents.len() as u128); + let slice_length = self + .dfg + .make_constant(array.contents.len().into(), NumericType::length_type()); let slice_contents = self.codegen_array_checked(elements, typ[1].clone())?; Tree::Branch(vec![slice_length.into(), slice_contents]) @@ -1258,7 +1260,8 @@ impl GlobalsBuilder { let char = self.dfg.make_constant((*byte as u128).into(), NumericType::char()); (char.into(), false) }); - let typ = FunctionContext::convert_non_tuple_type(&ast::Type::String(elements.len() as u32)); + let typ = + FunctionContext::convert_non_tuple_type(&ast::Type::String(elements.len() as u32)); self.codegen_array(elements, typ) } @@ -1300,20 +1303,13 @@ impl GlobalsBuilder { self.insert_make_array(array, typ).into() } - /// Insert a `make_array` instruction to create a new array or slice. /// Returns the new array value. Expects `typ` to be an array or slice type. - fn insert_make_array( - &mut self, - elements: im::Vector, - typ: Type, - ) -> ValueId { + fn insert_make_array(&mut self, elements: im::Vector, typ: Type) -> ValueId { assert!(matches!(typ, Type::Array(..) | Type::Slice(_))); - let id = self.dfg.make_instruction(Instruction::MakeArray { elements, typ: typ.clone() }, None); + let id = + self.dfg.make_instruction(Instruction::MakeArray { elements, typ: typ.clone() }, None); self.dfg.instruction_results(id)[0] } - } - - diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index fefcdff844e..9527e14100d 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -6,7 +6,11 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use crate::ssa::ir::{ - function::{Function, FunctionId}, instruction::Instruction, map::AtomicCounter, printer::{display_instruction, value}, value::Value + function::{Function, FunctionId}, + instruction::Instruction, + map::AtomicCounter, + printer::{display_instruction, value}, + value::Value, }; use noirc_frontend::hir_def::types::Type as HirType; @@ -106,30 +110,28 @@ impl Display for Ssa { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, "Globals: ")?; let show = |id| value(&self.globals.dfg, id); - + for (id, value) in self.globals.dfg.values_iter() { write!(f, "@{} = ", id)?; match value { Value::NumericConstant { constant, typ } => { writeln!(f, "{typ} {constant}")?; } - Value::Instruction { instruction, .. } => { - match &self.globals.dfg[*instruction] { - Instruction::MakeArray { elements, typ } => { - write!(f, "make_array [")?; - - for (i, element) in elements.iter().enumerate() { - if i != 0 { - write!(f, ", ")?; - } - write!(f, "{}", show(*element))?; + Value::Instruction { instruction, .. } => match &self.globals.dfg[*instruction] { + Instruction::MakeArray { elements, typ } => { + write!(f, "make_array [")?; + + for (i, element) in elements.iter().enumerate() { + if i != 0 { + write!(f, ", ")?; } - - writeln!(f, "] : {typ}")?; + write!(f, "{}", show(*element))?; } - _ => panic!("Expected MakeArray"), + + writeln!(f, "] : {typ}")?; } - } + _ => panic!("Expected MakeArray"), + }, Value::Global(_) => { panic!("we should only have these in the function values map"); } @@ -137,7 +139,7 @@ impl Display for Ssa { }; } writeln!(f, "")?; - + for function in self.functions.values() { writeln!(f, "{function}")?; } diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 705892e07ec..9cd3d3c3eac 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -99,7 +99,7 @@ struct Monomorphizer<'interner> { next_local_id: u32, next_global_id: u32, next_function_id: u32, - + is_range_loop: bool, return_location: Option, @@ -962,26 +962,27 @@ impl<'interner> Monomorphizer<'interner> { let expr = ast::Expression::Ident(ident); expr } else { - let (expr, is_function) = if let GlobalValue::Resolved(value) = global.value.clone() { - dbg!(value.is_closure()); - dbg!(value.clone()); - let is_function = value.is_closure(); - let expr = value - .into_hir_expression(self.interner, global.location) - .map_err(MonomorphizationError::InterpreterError)?; - (expr, is_function) - } else { - let let_ = self.interner.get_global_let_statement(*global_id).expect( + let (expr, is_function) = + if let GlobalValue::Resolved(value) = global.value.clone() { + dbg!(value.is_closure()); + dbg!(value.clone()); + let is_function = value.is_closure(); + let expr = value + .into_hir_expression(self.interner, global.location) + .map_err(MonomorphizationError::InterpreterError)?; + (expr, is_function) + } else { + let let_ = self.interner.get_global_let_statement(*global_id).expect( "Globals should have a corresponding let statement by monomorphization", ); - // TODO: update this - (let_.expression, false) - }; + // TODO: update this + (let_.expression, false) + }; let expr = self.expr(expr)?; // let new_id = self.next_global_id(); // self.globals.insert(id, new_id); - + if !is_function { let new_id = self.next_global_id(); self.globals.insert(id, new_id); From 8a24746cf7d578d289abbf62891ff56cdeec1582 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 15:02:20 +0000 Subject: [PATCH 04/19] fmt and clippy --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 4 +-- .../src/ssa/opt/normalize_value_ids.rs | 4 +-- .../src/ssa/ssa_gen/context.rs | 22 ++++-------- .../src/ssa/ssa_gen/program.rs | 10 +++--- .../src/monomorphization/mod.rs | 35 +++++-------------- 5 files changed, 23 insertions(+), 52 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 0c495005fd0..53bbe20989a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -17,7 +17,7 @@ use crate::ssa::{ instruction::{Instruction, InstructionId, TerminatorInstruction}, value::{Value, ValueId}, }, - ssa_gen::{context::GlobalsBuilder, Ssa}, + ssa_gen::{context::GlobalsContext, Ssa}, }; use fxhash::FxHashMap as HashMap; @@ -99,7 +99,7 @@ struct InlineContext<'global> { // These are the functions of the program that we shouldn't inline. functions_not_to_inline: BTreeSet, - globals: &'global GlobalsBuilder, + globals: &'global GlobalsContext, } /// The per-function inlining context contains information that is only valid for one function. 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 f6c32cc2a63..8bd2a2a08b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -8,7 +8,7 @@ use crate::ssa::{ post_order::PostOrder, value::{Value, ValueId}, }, - ssa_gen::{context::GlobalsBuilder, Ssa}, + ssa_gen::{context::GlobalsContext, Ssa}, }; use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; @@ -65,7 +65,7 @@ impl Context { } } - fn normalize_ids(&mut self, old_function: &mut Function, globals: &GlobalsBuilder) { + fn normalize_ids(&mut self, old_function: &mut Function, globals: &GlobalsContext) { self.new_ids.blocks.clear(); self.new_ids.values.clear(); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e1de29db18e..60fc17ebbbb 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -19,8 +19,6 @@ use crate::ssa::ir::instruction::BinaryOp; use crate::ssa::ir::instruction::Instruction; use crate::ssa::ir::map::AtomicCounter; use crate::ssa::ir::types::{NumericType, Type}; -use crate::ssa::ir::value::Value as IrValue; -use crate::ssa::ir::value::ValueId as IrValueId; use crate::ssa::ir::value::ValueId; use super::value::{Tree, Value, Values}; @@ -76,7 +74,7 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, - pub(super) globals_builder: GlobalsBuilder, + pub(super) globals_builder: GlobalsContext, pub(super) globals: BTreeMap, @@ -1045,7 +1043,7 @@ 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 mut globals_builder = GlobalsBuilder::default(); + let mut globals_builder = GlobalsContext::default(); let mut globals = BTreeMap::default(); for (id, global) in program.globals.iter() { dbg!(global.clone()); @@ -1103,14 +1101,14 @@ pub(super) enum LValue { } #[derive(Default, Serialize, Deserialize)] -pub(crate) struct GlobalsBuilder { +pub(crate) struct GlobalsContext { pub(crate) dfg: DataFlowGraph, #[serde(skip)] definitions: HashMap, } -impl GlobalsBuilder { +impl GlobalsContext { fn codegen_expression(&mut self, expr: &ast::Expression) -> Result { match expr { ast::Expression::Ident(ident) => Ok(self.codegen_ident(ident)), @@ -1133,20 +1131,12 @@ impl GlobalsBuilder { fn codegen_ident(&mut self, ident: &ast::Ident) -> Values { match &ident.definition { ast::Definition::Local(id) => self.lookup(*id), - // ast::Definition::Function(id) => self.get_or_queue_function(*id), _ => { panic!("Expected only Definition::Local but got {ident:#?}"); } } } - /// Retrieves the given function, adding it to the function queue - /// if it is not yet compiled. - // pub(super) fn get_or_queue_function(&mut self, id: FuncId) -> Values { - // let function = self.shared_context.get_or_queue_function(id); - // self.builder.import_function(function).into() - // } - fn unit_value() -> Values { Values::empty() } @@ -1160,7 +1150,7 @@ impl GlobalsBuilder { } fn codegen_let(&mut self, let_expr: &ast::Let) -> Result { - assert_eq!(let_expr.mutable, false, "Expected global let expression to be immutable"); + assert!(!let_expr.mutable, "Expected global let expression to be immutable"); let mut values = self.codegen_expression(&let_expr.expression)?; values = values.map(|value| { @@ -1221,7 +1211,7 @@ impl GlobalsBuilder { ast::Literal::Integer(value, negative, typ, location) => { let numeric_type = FunctionContext::convert_non_tuple_type(typ).unwrap_numeric(); - let value = (*value).into(); + let value = *value; if let Some(range) = numeric_type.value_is_outside_limits(value, *negative) { return Err(RuntimeError::IntegerOutOfBounds { diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 9527e14100d..bf00de41ad4 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -9,12 +9,12 @@ use crate::ssa::ir::{ function::{Function, FunctionId}, instruction::Instruction, map::AtomicCounter, - printer::{display_instruction, value}, + printer::value, value::Value, }; use noirc_frontend::hir_def::types::Type as HirType; -use super::context::GlobalsBuilder; +use super::context::GlobalsContext; /// Contains the entire SSA representation of the program. #[serde_as] @@ -22,7 +22,7 @@ use super::context::GlobalsBuilder; pub(crate) struct Ssa { #[serde_as(as = "Vec<(_, _)>")] pub(crate) functions: BTreeMap, - pub(crate) globals: GlobalsBuilder, + pub(crate) globals: GlobalsContext, pub(crate) main_id: FunctionId, #[serde(skip)] pub(crate) next_id: AtomicCounter, @@ -59,7 +59,7 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, - globals: GlobalsBuilder::default(), + globals: GlobalsContext::default(), } } @@ -138,7 +138,7 @@ impl Display for Ssa { _ => panic!("Expected only numeric const or array"), }; } - writeln!(f, "")?; + writeln!(f)?; for function in self.functions.values() { writeln!(f, "{function}")?; diff --git a/compiler/noirc_frontend/src/monomorphization/mod.rs b/compiler/noirc_frontend/src/monomorphization/mod.rs index 9cd3d3c3eac..8926e8ee815 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -959,31 +959,26 @@ impl<'interner> Monomorphizer<'interner> { name, typ, }; - let expr = ast::Expression::Ident(ident); - expr + ast::Expression::Ident(ident) } else { - let (expr, is_function) = + let (expr, is_closure) = if let GlobalValue::Resolved(value) = global.value.clone() { - dbg!(value.is_closure()); - dbg!(value.clone()); - let is_function = value.is_closure(); + let is_closure = value.is_closure(); let expr = value .into_hir_expression(self.interner, global.location) .map_err(MonomorphizationError::InterpreterError)?; - (expr, is_function) + (expr, is_closure) } else { let let_ = self.interner.get_global_let_statement(*global_id).expect( "Globals should have a corresponding let statement by monomorphization", ); - // TODO: update this - (let_.expression, false) + let is_closure = let_.r#type.is_function(); + (let_.expression, is_closure) }; let expr = self.expr(expr)?; - // let new_id = self.next_global_id(); - // self.globals.insert(id, new_id); - if !is_function { + if !is_closure { let new_id = self.next_global_id(); self.globals.insert(id, new_id); @@ -996,24 +991,10 @@ impl<'interner> Monomorphizer<'interner> { name, typ, }; - let expr = ast::Expression::Ident(ident); - - expr + ast::Expression::Ident(ident) } else { expr } - - // let typ = Self::convert_type(&typ, ident.location)?; - // let ident = ast::Ident { - // location: Some(ident.location), - // definition: Definition::Global(new_id), - // mutable: false, - // name, - // typ, - // }; - // let expr = ast::Expression::Ident(ident); - - // expr } } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { From e3358e3e31a9054dd83c7bfb1fcb39af4fc078d9 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 15:13:49 +0000 Subject: [PATCH 05/19] some comments for context --- .../noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs | 4 +--- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 3 ++- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) 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 3d64f8294a0..82678b46f53 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -1579,9 +1579,7 @@ impl<'block> BrilligBlock<'block> { match value { Value::Global(_) => { - // let variable = *self.function_context.globals.get(&value_id).unwrap_or_else(|| panic!("ICE: Global value not found in cache {value_id}")); - // variable - panic!("globals not handled, these should be inlined"); + unreachable!("ICE: all globals should have been inlined"); } Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index d58427d893d..175d3efd801 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -565,7 +565,8 @@ impl DataFlowGraph { } _ => false, }, - // TODO: make this true, + // 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/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 60fc17ebbbb..b11b4221e43 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1046,7 +1046,6 @@ impl SharedContext { let mut globals_builder = GlobalsContext::default(); let mut globals = BTreeMap::default(); for (id, global) in program.globals.iter() { - dbg!(global.clone()); let values = globals_builder.codegen_expression(global).unwrap(); globals.insert(*id, values); } From c3948fe42174b344f82b4b1895500bafe76075fd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 15:22:57 +0000 Subject: [PATCH 06/19] nargo fmt --- .../global_var_regression_simple/src/main.nr | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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 index 94ab0e36baf..b1bf753a73c 100644 --- a/test_programs/execution_success/global_var_regression_simple/src/main.nr +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -1,7 +1,4 @@ -global EXPONENTIATE: [[Field; 2]; 2] = [ - [1, 1], - [0, 0], -]; +global EXPONENTIATE: [[Field; 2]; 2] = [[1, 1], [0, 0]]; fn main(x: Field, y: pub Field) { let mut acc: Field = 0; @@ -25,4 +22,4 @@ fn dummy_again(x: Field, y: Field) { } assert(!acc.lt(x)); assert(x != y); -} \ No newline at end of file +} From 40c50fce7946fc879365a523d03338d3908e6489 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 16:11:31 +0000 Subject: [PATCH 07/19] remove type from Value::Global and cleanup --- compiler/noirc_evaluator/src/acir/mod.rs | 2 +- .../src/brillig/brillig_gen/brillig_block.rs | 4 ++-- .../src/brillig/brillig_gen/variable_liveness.rs | 2 +- .../src/ssa/checks/check_for_underconstrained_values.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/value.rs | 5 +++-- compiler/noirc_evaluator/src/ssa/opt/inlining.rs | 6 +++--- .../noirc_evaluator/src/ssa/opt/normalize_value_ids.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 8 ++++---- compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs | 3 +-- 11 files changed, 24 insertions(+), 24 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index fd3ac0f1ab9..a8e7cae35c3 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1892,7 +1892,7 @@ impl<'a> Context<'a> { Value::Instruction { .. } | Value::Param { .. } => { unreachable!("ICE: Should have been in cache {value_id} {value:?}") } - Value::Global(_) => { + Value::Global => { unreachable!("ICE: all globals should have been inlined"); } }; 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 356f4db9da6..8ad6b6362df 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -649,7 +649,7 @@ impl<'block> BrilligBlock<'block> { Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } - | Value::Global(_) => { + | Value::Global => { unreachable!("unsupported function call type {:?}", dfg[*func]) } }, @@ -1560,7 +1560,7 @@ impl<'block> BrilligBlock<'block> { let value = &dfg[value_id]; match value { - Value::Global(_) => { + Value::Global => { unreachable!("ICE: all globals should have been inlined"); } Value::Param { .. } | Value::Instruction { .. } => { 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 c0248a50412..04996caccb7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -56,7 +56,7 @@ pub(crate) fn collect_variables_of_value( Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } - | Value::Global(_) => Some(value_id), + | 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/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 80dde5e27f3..bf5e62b7903 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 @@ -333,7 +333,7 @@ impl DependencyContext { Value::Instruction { .. } | Value::NumericConstant { .. } | Value::Param { .. } - | Value::Global(_) => { + | Value::Global => { panic!( "calling non-function value with ID {func_id} in function {}", function.name() @@ -620,7 +620,7 @@ impl Context { Value::Instruction { .. } | Value::NumericConstant { .. } | Value::Param { .. } - | Value::Global(_) => { + | 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 c9050bb5042..7ffcdac28cd 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -325,8 +325,8 @@ impl DataFlowGraph { id } - pub(crate) fn make_global(&mut self, typ: Type) -> ValueId { - self.values.insert(Value::Global(typ)) + pub(crate) fn make_global(&mut self) -> ValueId { + self.values.insert(Value::Global) } /// Gets or creates a ValueId for the given FunctionId. @@ -595,7 +595,7 @@ impl DataFlowGraph { }, // 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, + Value::Global => false, _ => true, } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 2e4ea576f15..cc15eee32ca 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -53,7 +53,7 @@ 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(_) => { + Value::Global => { format!("@{id}") } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index b3dce24363c..65bb5cb52ca 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -65,7 +65,7 @@ pub(crate) enum Value { /// other than generating different backend operations and being only accessible through Brillig. ForeignFunction(String), - Global(Type), + Global, } impl Value { @@ -77,7 +77,8 @@ impl Value { Value::Function { .. } | Value::Intrinsic { .. } | Value::ForeignFunction { .. } => { Cow::Owned(Type::Function) } - Value::Global(typ) => Cow::Borrowed(typ), + // Value::Global(typ) => Cow::Borrowed(typ), + Value::Global => unreachable!("Global does not have a type associated with it. Fetch the actual value from the global context"), } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 53bbe20989a..59bd460658e 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -382,8 +382,8 @@ impl<'global> InlineContext<'global> { let mut context = PerFunctionContext::new(&mut self, entry_point); 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()); + for _ in ssa.globals.dfg.values_iter() { + context.context.builder.current_function.dfg.make_global(); } // The entry block is already inserted so we have to add it to context.blocks and add @@ -488,7 +488,7 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { Value::ForeignFunction(function) => { self.context.builder.import_foreign_function(function) } - Value::Global(_) => { + Value::Global => { // TODO: Inlining the global into the function is only a temporary measure // until Brillig gen with globals is working end to end let new_id = match &self.context.globals.dfg[id] { 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 8bd2a2a08b9..4408c991a4c 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -72,8 +72,8 @@ impl Context { 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.dfg.values_iter() { - new_function.dfg.make_global(value.get_type().into_owned()); + for _ in globals.dfg.values_iter() { + new_function.dfg.make_global(); } let mut reachable_blocks = PostOrder::with_function(old_function).into_vec(); @@ -196,7 +196,7 @@ impl IdMaps { } Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), - Value::Global(_) => { + 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 b11b4221e43..4f6506a7f0e 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -116,8 +116,8 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); - for (_, value) in shared_context.globals_builder.dfg.values_iter() { - builder.current_function.dfg.make_global(value.get_type().into_owned()); + for _ in shared_context.globals_builder.dfg.values_iter() { + builder.current_function.dfg.make_global(); } let definitions = HashMap::default(); @@ -139,8 +139,8 @@ impl<'a> FunctionContext<'a> { self.builder.new_function(func.name.clone(), id, func.inline_type); } - for (_, value) in self.shared_context.globals_builder.dfg.values_iter() { - self.builder.current_function.dfg.make_global(value.get_type().into_owned()); + for _ in self.shared_context.globals_builder.dfg.values_iter() { + self.builder.current_function.dfg.make_global(); } self.add_parameters_to_scope(&func.parameters); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 73cbd6c95a2..c6f468af23b 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -7,7 +7,6 @@ use serde_with::serde_as; use crate::ssa::ir::{ function::{Function, FunctionId}, - instruction::Instruction, map::AtomicCounter, printer::display_instruction, value::Value, @@ -118,7 +117,7 @@ impl Display for Ssa { Value::Instruction { instruction, .. } => { display_instruction(&self.globals.dfg, *instruction, f)?; } - Value::Global(_) => { + Value::Global => { panic!("Value::Global should only be in the function dfg"); } _ => panic!("Expected only numeric constant or instruction"), From 77441793fac75cc2e891ec1ee59e4224310ed037 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 16:21:37 +0000 Subject: [PATCH 08/19] bring back global type --- compiler/noirc_evaluator/src/acir/mod.rs | 4 ++-- .../src/brillig/brillig_gen/brillig_block.rs | 6 +++--- .../src/brillig/brillig_gen/variable_liveness.rs | 2 +- .../src/ssa/checks/check_for_underconstrained_values.rs | 4 ++-- compiler/noirc_evaluator/src/ssa/ir/dfg.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/ir/printer.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/value.rs | 5 ++--- compiler/noirc_evaluator/src/ssa/opt/inlining.rs | 6 +++--- .../noirc_evaluator/src/ssa/opt/normalize_value_ids.rs | 6 +++--- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 8 ++++---- compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs | 2 +- 11 files changed, 25 insertions(+), 26 deletions(-) diff --git a/compiler/noirc_evaluator/src/acir/mod.rs b/compiler/noirc_evaluator/src/acir/mod.rs index a8e7cae35c3..60fd6bac5a5 100644 --- a/compiler/noirc_evaluator/src/acir/mod.rs +++ b/compiler/noirc_evaluator/src/acir/mod.rs @@ -1892,8 +1892,8 @@ 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"); + Value::Global(_) => { + unreachable!("ICE: All globals should have been inlined"); } }; self.ssa_values.insert(value_id, acir_value.clone()); 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 8ad6b6362df..13a156953fc 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -649,7 +649,7 @@ impl<'block> BrilligBlock<'block> { Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } - | Value::Global => { + | Value::Global(_) => { unreachable!("unsupported function call type {:?}", dfg[*func]) } }, @@ -1560,8 +1560,8 @@ impl<'block> BrilligBlock<'block> { let value = &dfg[value_id]; match value { - Value::Global => { - unreachable!("ICE: all globals should have been inlined"); + Value::Global(_) => { + unreachable!("ICE: All globals should have been inlined"); } Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been 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 04996caccb7..c0248a50412 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -56,7 +56,7 @@ pub(crate) fn collect_variables_of_value( Value::Instruction { .. } | Value::Param { .. } | Value::NumericConstant { .. } - | Value::Global => Some(value_id), + | 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/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index bf5e62b7903..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 @@ -333,7 +333,7 @@ impl DependencyContext { Value::Instruction { .. } | Value::NumericConstant { .. } | Value::Param { .. } - | Value::Global => { + | Value::Global(_) => { panic!( "calling non-function value with ID {func_id} in function {}", function.name() @@ -620,7 +620,7 @@ impl Context { Value::Instruction { .. } | Value::NumericConstant { .. } | Value::Param { .. } - | Value::Global => { + | 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 7ffcdac28cd..c9050bb5042 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -325,8 +325,8 @@ impl DataFlowGraph { id } - pub(crate) fn make_global(&mut self) -> ValueId { - self.values.insert(Value::Global) + 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. @@ -595,7 +595,7 @@ impl DataFlowGraph { }, // 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, + Value::Global(_) => false, _ => true, } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index cc15eee32ca..2e4ea576f15 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -53,7 +53,7 @@ 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 => { + Value::Global(_) => { format!("@{id}") } } diff --git a/compiler/noirc_evaluator/src/ssa/ir/value.rs b/compiler/noirc_evaluator/src/ssa/ir/value.rs index 65bb5cb52ca..b3dce24363c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -65,7 +65,7 @@ pub(crate) enum Value { /// other than generating different backend operations and being only accessible through Brillig. ForeignFunction(String), - Global, + Global(Type), } impl Value { @@ -77,8 +77,7 @@ impl Value { Value::Function { .. } | Value::Intrinsic { .. } | Value::ForeignFunction { .. } => { Cow::Owned(Type::Function) } - // Value::Global(typ) => Cow::Borrowed(typ), - Value::Global => unreachable!("Global does not have a type associated with it. Fetch the actual value from the global context"), + 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 59bd460658e..53bbe20989a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -382,8 +382,8 @@ impl<'global> InlineContext<'global> { let mut context = PerFunctionContext::new(&mut self, entry_point); context.inlining_entry = true; - for _ in ssa.globals.dfg.values_iter() { - context.context.builder.current_function.dfg.make_global(); + 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 @@ -488,7 +488,7 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { Value::ForeignFunction(function) => { self.context.builder.import_foreign_function(function) } - Value::Global => { + Value::Global(_) => { // TODO: Inlining the global into the function is only a temporary measure // until Brillig gen with globals is working end to end let new_id = match &self.context.globals.dfg[id] { 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 4408c991a4c..8bd2a2a08b9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -72,8 +72,8 @@ impl Context { let new_function_id = self.new_ids.function_ids[&old_function.id()]; let new_function = &mut self.functions[new_function_id]; - for _ in globals.dfg.values_iter() { - new_function.dfg.make_global(); + for (_, value) in globals.dfg.values_iter() { + new_function.dfg.make_global(value.get_type().into_owned()); } let mut reachable_blocks = PostOrder::with_function(old_function).into_vec(); @@ -196,7 +196,7 @@ impl IdMaps { } Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), - Value::Global => { + 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 4f6506a7f0e..b11b4221e43 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -116,8 +116,8 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); - for _ in shared_context.globals_builder.dfg.values_iter() { - builder.current_function.dfg.make_global(); + for (_, value) in shared_context.globals_builder.dfg.values_iter() { + builder.current_function.dfg.make_global(value.get_type().into_owned()); } let definitions = HashMap::default(); @@ -139,8 +139,8 @@ impl<'a> FunctionContext<'a> { self.builder.new_function(func.name.clone(), id, func.inline_type); } - for _ in self.shared_context.globals_builder.dfg.values_iter() { - self.builder.current_function.dfg.make_global(); + for (_, value) in self.shared_context.globals_builder.dfg.values_iter() { + self.builder.current_function.dfg.make_global(value.get_type().into_owned()); } self.add_parameters_to_scope(&func.parameters); diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index c6f468af23b..a85abc0becc 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -117,7 +117,7 @@ impl Display for Ssa { Value::Instruction { instruction, .. } => { display_instruction(&self.globals.dfg, *instruction, f)?; } - Value::Global => { + Value::Global(_) => { panic!("Value::Global should only be in the function dfg"); } _ => panic!("Expected only numeric constant or instruction"), From 68ddd14c98bc4944ea5026049f64e427dfe9d2c8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 11:25:47 -0500 Subject: [PATCH 09/19] Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b11b4221e43..64224758569 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1195,8 +1195,6 @@ impl GlobalsContext { let typ = FunctionContext::convert_type(&array.typ).flatten(); Ok(match array.typ { ast::Type::Slice(_) => { - // let slice_length = - // self.builder.length_constant(array.contents.len() as u128); let slice_length = self .dfg .make_constant(array.contents.len().into(), NumericType::length_type()); From 68230198b4c28462ead8e2ccae4d8429d068043a Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 17:11:10 +0000 Subject: [PATCH 10/19] only print globals when printing ssa if globals exist --- compiler/noirc_evaluator/src/ssa.rs | 2 +- .../noirc_evaluator/src/ssa/ir/printer.rs | 9 ++++-- .../noirc_evaluator/src/ssa/opt/inlining.rs | 5 ++-- .../src/ssa/ssa_gen/context.rs | 12 ++++---- .../noirc_evaluator/src/ssa/ssa_gen/mod.rs | 2 +- .../src/ssa/ssa_gen/program.rs | 28 ++++++++++++------- .../global_var_regression_simple/src/main.nr | 2 +- 7 files changed, 35 insertions(+), 25 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 2500b8685a1..30ea6433453 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/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 2e4ea576f15..f731283034c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -35,7 +35,7 @@ pub(crate) fn display_block( 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, true, f)?; } display_terminator(dfg, block.terminator(), f) @@ -113,10 +113,13 @@ pub(crate) fn display_terminator( pub(crate) fn display_instruction( dfg: &DataFlowGraph, instruction: InstructionId, + indent: bool, f: &mut Formatter, ) -> Result { - // instructions are always indented within a function - write!(f, " ")?; + if indent { + // instructions are always indented within a function + write!(f, " ")?; + } let results = dfg.instruction_results(instruction); if !results.is_empty() { diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 53bbe20989a..d4d6f745788 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -491,7 +491,7 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { Value::Global(_) => { // TODO: Inlining the global into the function is only a temporary measure // until Brillig gen with globals is working end to end - let new_id = match &self.context.globals.dfg[id] { + match &self.context.globals.dfg[id] { Value::Instruction { instruction, .. } => { let Instruction::MakeArray { elements, typ } = &self.context.globals.dfg[*instruction] @@ -508,8 +508,7 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { self.context.builder.numeric_constant(*constant, *typ) } _ => panic!("Expected only an instruction or a numeric constant"), - }; - new_id + } } }; diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index b11b4221e43..228bcb7aa0a 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -74,7 +74,7 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, - pub(super) globals_builder: GlobalsContext, + pub(super) globals_context: GlobalsContext, pub(super) globals: BTreeMap, @@ -116,7 +116,7 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); - for (_, value) in shared_context.globals_builder.dfg.values_iter() { + for (_, value) in shared_context.globals_context.dfg.values_iter() { builder.current_function.dfg.make_global(value.get_type().into_owned()); } @@ -139,7 +139,7 @@ impl<'a> FunctionContext<'a> { self.builder.new_function(func.name.clone(), id, func.inline_type); } - for (_, value) in self.shared_context.globals_builder.dfg.values_iter() { + for (_, value) in self.shared_context.globals_context.dfg.values_iter() { self.builder.current_function.dfg.make_global(value.get_type().into_owned()); } @@ -1043,10 +1043,10 @@ 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 mut globals_builder = GlobalsContext::default(); + let mut globals_context = GlobalsContext::default(); let mut globals = BTreeMap::default(); for (id, global) in program.globals.iter() { - let values = globals_builder.codegen_expression(global).unwrap(); + let values = globals_context.codegen_expression(global).unwrap(); globals.insert(*id, values); } @@ -1055,7 +1055,7 @@ impl SharedContext { function_queue: Default::default(), function_counter: Default::default(), program, - globals_builder, + globals_context, globals, } } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 191414c4833..f8ca101829c 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -121,7 +121,7 @@ pub(crate) fn generate_ssa(program: Program) -> Result { } let mut ssa = function_context.builder.finish(); - ssa.globals = context.globals_builder; + ssa.globals = context.globals_context; Ok(ssa) } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index a85abc0becc..5d9f94090dc 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -106,16 +106,29 @@ impl Ssa { } impl Display for Ssa { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if self.globals.dfg.values_iter().len() > 0 { + write!(f, "{}", self.globals)?; + } + + for function in self.functions.values() { + writeln!(f, "{function}")?; + } + Ok(()) + } +} + +impl Display for GlobalsContext { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { writeln!(f, "Globals: ")?; - for (id, value) in self.globals.dfg.values_iter() { - write!(f, "@{} = ", id)?; + for (id, value) in self.dfg.values_iter() { + write!(f, "@")?; match value { Value::NumericConstant { constant, typ } => { - writeln!(f, "{typ} {constant}")?; + writeln!(f, "{id} = {typ} {constant}")?; } Value::Instruction { instruction, .. } => { - display_instruction(&self.globals.dfg, *instruction, f)?; + display_instruction(&self.dfg, *instruction, false, f)?; } Value::Global(_) => { panic!("Value::Global should only be in the function dfg"); @@ -123,12 +136,7 @@ impl Display for Ssa { _ => panic!("Expected only numeric constant or instruction"), }; } - writeln!(f)?; - - for function in self.functions.values() { - writeln!(f, "{function}")?; - } - Ok(()) + writeln!(f) } } 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 index b1bf753a73c..10e3976ef7e 100644 --- a/test_programs/execution_success/global_var_regression_simple/src/main.nr +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -20,6 +20,6 @@ fn dummy_again(x: Field, y: Field) { acc += EXPONENTIATE[i][j]; } } - assert(!acc.lt(x)); + // assert(!acc.lt(x)); assert(x != y); } From cbe377ea6b853dab612a38a7ad328067ebc63079 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 17:28:37 +0000 Subject: [PATCH 11/19] make an eval method --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 36c1dfc236a..e86de838035 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1278,11 +1278,7 @@ impl GlobalsContext { for (element, _) in elements { element.for_each(|element| { - // let element = element.eval(self); - let element = match element { - Value::Normal(value) => value, - _ => panic!("Must have Value::Normal for globals"), - }; + let element = Self::eval(element); array.push_back(element); }); } @@ -1299,4 +1295,11 @@ impl GlobalsContext { self.dfg.make_instruction(Instruction::MakeArray { elements, typ: typ.clone() }, None); self.dfg.instruction_results(id)[0] } + + fn eval(value: Value) -> ValueId { + match value { + Value::Normal(value) => value, + _ => panic!("Must have Value::Normal for globals"), + } + } } From 2d750fb2b5da40d56deb0a7558363ab0f8a7a0ec Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 13:23:02 -0500 Subject: [PATCH 12/19] Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e86de838035..a462fdcdc06 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1236,7 +1236,7 @@ impl GlobalsContext { ast::Literal::Bool(value) => { Ok(self.dfg.make_constant((*value).into(), NumericType::bool()).into()) } - ast::Literal::Unit => todo!(), + ast::Literal::Unit => Ok(Self::unit_value()), ast::Literal::Str(string) => Ok(self.codegen_string(string)), ast::Literal::FmtStr(_, _, _) => todo!(), } From faeb30f2ffa4b91d3ca366f29a191d6fc3eb7bf0 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 13:23:28 -0500 Subject: [PATCH 13/19] Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index a462fdcdc06..384696b0dab 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1238,7 +1238,9 @@ impl GlobalsContext { } ast::Literal::Unit => Ok(Self::unit_value()), ast::Literal::Str(string) => Ok(self.codegen_string(string)), - ast::Literal::FmtStr(_, _, _) => todo!(), + ast::Literal::FmtStr(_, _, _) => { + unreachable!("Format strings are lowered as normal strings as they are already interpolated"); + } } } From 2322cb6971bb2f1ea63fa52c9c6e7ce037bb93c5 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 13:26:15 -0500 Subject: [PATCH 14/19] Update test_programs/execution_success/global_var_regression_simple/src/main.nr --- .../execution_success/global_var_regression_simple/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 10e3976ef7e..b1bf753a73c 100644 --- a/test_programs/execution_success/global_var_regression_simple/src/main.nr +++ b/test_programs/execution_success/global_var_regression_simple/src/main.nr @@ -20,6 +20,6 @@ fn dummy_again(x: Field, y: Field) { acc += EXPONENTIATE[i][j]; } } - // assert(!acc.lt(x)); + assert(!acc.lt(x)); assert(x != y); } From fa6d9cf60849aded08da6f063968498ea5156b1b Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 19:07:50 +0000 Subject: [PATCH 15/19] cargo mft --- compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 384696b0dab..54ba5ec4498 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -1239,7 +1239,9 @@ impl GlobalsContext { ast::Literal::Unit => Ok(Self::unit_value()), ast::Literal::Str(string) => Ok(self.codegen_string(string)), ast::Literal::FmtStr(_, _, _) => { - unreachable!("Format strings are lowered as normal strings as they are already interpolated"); + unreachable!( + "Format strings are lowered as normal strings as they are already interpolated" + ); } } } From bb5a9ea6f5ec88f9335926d07cb09058a38c24ad Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 11:49:22 -0500 Subject: [PATCH 16/19] Update compiler/noirc_frontend/src/hir/comptime/value.rs Co-authored-by: jfecher --- compiler/noirc_frontend/src/hir/comptime/value.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index c82db568a0a..77933ba9361 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -522,7 +522,7 @@ impl Value { } pub(crate) fn is_closure(&self) -> bool { - matches!(self, Value::Closure(_, _, _, _, _)) + matches!(self, Value::Closure(..)) } /// Converts any non-negative `Value` into a `FieldElement`. From 74a73582b85e15ed070455da56b53bb23ec191fd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 21:14:18 +0000 Subject: [PATCH 17/19] do ot duplicate codegen --- .../noirc_evaluator/src/ssa/ir/function.rs | 6 - .../noirc_evaluator/src/ssa/ir/printer.rs | 67 +++-- compiler/noirc_evaluator/src/ssa/ir/value.rs | 18 +- .../noirc_evaluator/src/ssa/opt/inlining.rs | 12 +- .../src/ssa/opt/normalize_value_ids.rs | 7 +- .../src/ssa/ssa_gen/context.rs | 250 ++---------------- .../src/ssa/ssa_gen/program.rs | 46 +--- compiler/noirc_frontend/src/ast/mod.rs | 3 +- .../src/monomorphization/ast.rs | 9 +- .../src/monomorphization/mod.rs | 107 ++++---- 10 files changed, 168 insertions(+), 357 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index 109c2a59781..4d1483913e1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -228,12 +228,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 f731283034c..000ebed3e31 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.values_iter() { + match global_value { + Value::NumericConstant { constant, typ } => { + writeln!(f, "g{} = {typ} {constant}", id.to_u32())?; + } + Value::Instruction { instruction, .. } => { + display_instruction(&self.globals, *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.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, true, f)?; + display_instruction(dfg, *instruction, false, f)?; } display_terminator(dfg, block.terminator(), f) @@ -54,7 +87,7 @@ fn value(dfg: &DataFlowGraph, id: ValueId) -> String { Value::ForeignFunction(function) => function.clone(), Value::Param { .. } | Value::Instruction { .. } => id.to_string(), Value::Global(_) => { - format!("@{id}") + format!("g{}", id.to_u32()) } } } @@ -75,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, @@ -110,20 +143,24 @@ pub(crate) fn display_terminator( } /// Display an arbitrary instruction -pub(crate) fn display_instruction( +fn display_instruction( dfg: &DataFlowGraph, instruction: InstructionId, - indent: bool, + in_global_space: bool, f: &mut Formatter, ) -> Result { - if indent { + 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 b3dce24363c..53f87a260c3 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/value.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/value.rs @@ -27,27 +27,16 @@ pub(crate) enum Value { /// Example, if you add two numbers together, then the resulting /// value would have position `0`, the typ would be the type /// of the operands, and the instruction would map to an add instruction. - Instruction { - instruction: InstructionId, - position: usize, - typ: Type, - }, + Instruction { instruction: InstructionId, position: usize, typ: Type }, /// This Value originates from a block parameter. Since function parameters /// are also represented as block parameters, this includes function parameters as well. /// /// position -- the index of this Value in the block parameters list - Param { - block: BasicBlockId, - position: usize, - typ: Type, - }, + Param { block: BasicBlockId, position: usize, typ: Type }, /// This Value originates from a numeric constant - NumericConstant { - constant: FieldElement, - typ: NumericType, - }, + NumericConstant { constant: FieldElement, typ: NumericType }, /// This Value refers to a function in the IR. /// Functions always have the type Type::Function. @@ -65,6 +54,7 @@ pub(crate) enum Value { /// 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), } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index d4d6f745788..c61ff208b18 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -12,12 +12,12 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, call_stack::CallStackId, - dfg::InsertInstructionResult, + dfg::{DataFlowGraph, InsertInstructionResult}, function::{Function, FunctionId, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, value::{Value, ValueId}, }, - ssa_gen::{context::GlobalsContext, Ssa}, + ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; @@ -99,7 +99,7 @@ struct InlineContext<'global> { // These are the functions of the program that we shouldn't inline. functions_not_to_inline: BTreeSet, - globals: &'global GlobalsContext, + globals: &'global DataFlowGraph, } /// The per-function inlining context contains information that is only valid for one function. @@ -382,7 +382,7 @@ impl<'global> InlineContext<'global> { let mut context = PerFunctionContext::new(&mut self, entry_point); context.inlining_entry = true; - for (_, value) in ssa.globals.dfg.values_iter() { + for (_, value) in ssa.globals.values_iter() { context.context.builder.current_function.dfg.make_global(value.get_type().into_owned()); } @@ -491,10 +491,10 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { 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.context.globals.dfg[id] { + match &self.context.globals[id] { Value::Instruction { instruction, .. } => { let Instruction::MakeArray { elements, typ } = - &self.context.globals.dfg[*instruction] + &self.context.globals[*instruction] else { panic!("Only expect Instruction::MakeArray for a global"); }; 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 8bd2a2a08b9..af0add92799 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -3,12 +3,13 @@ use std::collections::BTreeMap; use crate::ssa::{ ir::{ basic_block::BasicBlockId, + dfg::DataFlowGraph, function::{Function, FunctionId}, map::SparseMap, post_order::PostOrder, value::{Value, ValueId}, }, - ssa_gen::{context::GlobalsContext, Ssa}, + ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; use iter_extended::vecmap; @@ -65,14 +66,14 @@ impl Context { } } - fn normalize_ids(&mut self, old_function: &mut Function, globals: &GlobalsContext) { + 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.dfg.values_iter() { + for (_, value) in globals.values_iter() { new_function.dfg.make_global(value.get_type().into_owned()); } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 54ba5ec4498..af98eb6f93e 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -2,12 +2,11 @@ use std::collections::BTreeMap; use std::sync::{Arc, Mutex, RwLock}; use acvm::{acir::AcirField, FieldElement}; -use iter_extended::{try_vecmap, vecmap}; +use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::ast::{BinaryOpKind, Signedness}; -use noirc_frontend::monomorphization::ast::{self, GlobalId, LocalId, Parameters}; +use noirc_frontend::monomorphization::ast::{self, GlobalId, InlineType, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; -use serde::{Deserialize, Serialize}; use crate::errors::RuntimeError; use crate::ssa::function_builder::FunctionBuilder; @@ -57,6 +56,7 @@ pub(super) struct FunctionContext<'a> { /// SSA can be generated by continuously popping from this function_queue and using /// FunctionContext to generate from the popped function id. Once the queue is empty, /// no other functions are reachable and the SSA generation is finished. +#[derive(Default)] pub(super) struct SharedContext { /// All currently known functions which have already been assigned function ids. /// These functions are all either currently having their SSA generated or are @@ -74,7 +74,7 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, - pub(super) globals_context: GlobalsContext, + pub(super) globals_context: DataFlowGraph, pub(super) globals: BTreeMap, @@ -116,12 +116,9 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); - for (_, value) in shared_context.globals_context.dfg.values_iter() { - builder.current_function.dfg.make_global(value.get_type().into_owned()); - } - 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 } @@ -139,13 +136,17 @@ impl<'a> FunctionContext<'a> { self.builder.new_function(func.name.clone(), id, func.inline_type); } - for (_, value) in self.shared_context.globals_context.dfg.values_iter() { - self.builder.current_function.dfg.make_global(value.get_type().into_owned()); - } + self.add_globals(); self.add_parameters_to_scope(&func.parameters); } + fn add_globals(&mut self) { + for (_, value) in self.shared_context.globals_context.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 @@ -1043,10 +1044,22 @@ 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 mut globals_context = GlobalsContext::default(); + let globals_shared_context = SharedContext::default(); + + 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 = globals_context.codegen_expression(global).unwrap(); + let values = context.codegen_expression(global).unwrap(); globals.insert(*id, values); } @@ -1055,7 +1068,7 @@ impl SharedContext { function_queue: Default::default(), function_counter: Default::default(), program, - globals_context, + globals_context: context.builder.current_function.dfg, globals, } } @@ -1098,212 +1111,3 @@ pub(super) enum LValue { MemberAccess { old_object: Values, index: usize, object_lvalue: Box }, Dereference { reference: Values }, } - -#[derive(Default, Serialize, Deserialize)] -pub(crate) struct GlobalsContext { - pub(crate) dfg: DataFlowGraph, - - #[serde(skip)] - definitions: HashMap, -} - -impl GlobalsContext { - fn codegen_expression(&mut self, expr: &ast::Expression) -> Result { - match expr { - ast::Expression::Ident(ident) => Ok(self.codegen_ident(ident)), - ast::Expression::Literal(literal) => self.codegen_literal(literal), - ast::Expression::Block(block) => self.codegen_block(block), - ast::Expression::Let(let_expr) => self.codegen_let(let_expr), - ast::Expression::Tuple(tuple) => self.codegen_tuple(tuple), - _ => { - panic!("Only expected literals for global expressions but got {expr}") - } - } - } - - /// Looks up the value of a given local variable. Expects the variable to have - /// been previously defined or panics otherwise. - pub(super) fn lookup(&self, id: LocalId) -> Values { - self.definitions.get(&id).expect("lookup: variable not defined").clone() - } - - fn codegen_ident(&mut self, ident: &ast::Ident) -> Values { - match &ident.definition { - ast::Definition::Local(id) => self.lookup(*id), - _ => { - panic!("Expected only Definition::Local but got {ident:#?}"); - } - } - } - - fn unit_value() -> Values { - Values::empty() - } - - fn codegen_block(&mut self, block: &[ast::Expression]) -> Result { - let mut result = Self::unit_value(); - for expr in block { - result = self.codegen_expression(expr)?; - } - Ok(result) - } - - fn codegen_let(&mut self, let_expr: &ast::Let) -> Result { - assert!(!let_expr.mutable, "Expected global let expression to be immutable"); - let mut values = self.codegen_expression(&let_expr.expression)?; - - values = values.map(|value| { - let value = match value { - Value::Normal(value) => value, - _ => panic!("Must have Value::Normal for globals"), - }; - - Tree::Leaf(Value::Normal(value)) - }); - - self.define(let_expr.id, values); - Ok(Self::unit_value()) - } - - fn codegen_tuple(&mut self, tuple: &[ast::Expression]) -> Result { - Ok(Tree::Branch(try_vecmap(tuple, |expr| self.codegen_expression(expr))?)) - } - - /// Define a local variable to be some Values that can later be retrieved - /// by calling self.lookup(id) - pub(super) fn define(&mut self, id: LocalId, value: Values) { - let existing = self.definitions.insert(id, value); - assert!(existing.is_none(), "Variable {id:?} was defined twice in ssa-gen pass"); - } - - fn codegen_literal(&mut self, literal: &ast::Literal) -> Result { - match literal { - ast::Literal::Array(array) => { - let elements = self.codegen_array_elements(&array.contents)?; - - let typ = FunctionContext::convert_type(&array.typ).flatten(); - Ok(match array.typ { - ast::Type::Array(_, _) => { - self.codegen_array_checked(elements, typ[0].clone())? - } - _ => unreachable!("ICE: unexpected array literal type, got {}", array.typ), - }) - } - ast::Literal::Slice(array) => { - let elements = self.codegen_array_elements(&array.contents)?; - - let typ = FunctionContext::convert_type(&array.typ).flatten(); - Ok(match array.typ { - ast::Type::Slice(_) => { - let slice_length = self - .dfg - .make_constant(array.contents.len().into(), NumericType::length_type()); - let slice_contents = - self.codegen_array_checked(elements, typ[1].clone())?; - Tree::Branch(vec![slice_length.into(), slice_contents]) - } - _ => unreachable!("ICE: unexpected slice literal type, got {}", array.typ), - }) - } - ast::Literal::Integer(value, negative, typ, location) => { - let numeric_type = FunctionContext::convert_non_tuple_type(typ).unwrap_numeric(); - - let value = *value; - - if let Some(range) = numeric_type.value_is_outside_limits(value, *negative) { - return Err(RuntimeError::IntegerOutOfBounds { - value: if *negative { -value } else { value }, - typ: numeric_type, - range, - call_stack: vec![*location], - }); - } - - let value = if *negative { - match numeric_type { - NumericType::NativeField => -value, - NumericType::Signed { bit_size } | NumericType::Unsigned { bit_size } => { - let base = 1_u128 << bit_size; - FieldElement::from(base) - value - } - } - } else { - value - }; - - Ok(self.dfg.make_constant(value, numeric_type).into()) - } - ast::Literal::Bool(value) => { - Ok(self.dfg.make_constant((*value).into(), NumericType::bool()).into()) - } - ast::Literal::Unit => Ok(Self::unit_value()), - ast::Literal::Str(string) => Ok(self.codegen_string(string)), - ast::Literal::FmtStr(_, _, _) => { - unreachable!( - "Format strings are lowered as normal strings as they are already interpolated" - ); - } - } - } - - fn codegen_string(&mut self, string: &str) -> Values { - let elements = vecmap(string.as_bytes(), |byte| { - let char = self.dfg.make_constant((*byte as u128).into(), NumericType::char()); - (char.into(), false) - }); - let typ = - FunctionContext::convert_non_tuple_type(&ast::Type::String(elements.len() as u32)); - self.codegen_array(elements, typ) - } - - fn codegen_array_elements( - &mut self, - elements: &[ast::Expression], - ) -> Result, RuntimeError> { - try_vecmap(elements, |element| { - let value = self.codegen_expression(element)?; - Ok((value, element.is_array_or_slice_literal())) - }) - } - - fn codegen_array_checked( - &mut self, - elements: Vec<(Values, bool)>, - typ: Type, - ) -> Result { - if typ.is_nested_slice() { - return Err(RuntimeError::NestedSlice { call_stack: vec![] }); - } - Ok(self.codegen_array(elements, typ)) - } - - fn codegen_array(&mut self, elements: Vec<(Values, bool)>, typ: Type) -> Values { - let mut array = im::Vector::new(); - - for (element, _) in elements { - element.for_each(|element| { - let element = Self::eval(element); - array.push_back(element); - }); - } - - self.insert_make_array(array, typ).into() - } - - /// Insert a `make_array` instruction to create a new array or slice. - /// Returns the new array value. Expects `typ` to be an array or slice type. - fn insert_make_array(&mut self, elements: im::Vector, typ: Type) -> ValueId { - assert!(matches!(typ, Type::Array(..) | Type::Slice(_))); - - let id = - self.dfg.make_instruction(Instruction::MakeArray { elements, typ: typ.clone() }, None); - self.dfg.instruction_results(id)[0] - } - - fn eval(value: Value) -> ValueId { - match value { - Value::Normal(value) => value, - _ => panic!("Must have Value::Normal for globals"), - } - } -} diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 5d9f94090dc..79cbc8cadf2 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; @@ -6,22 +6,19 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use crate::ssa::ir::{ + dfg::DataFlowGraph, function::{Function, FunctionId}, map::AtomicCounter, - printer::display_instruction, - value::Value, }; use noirc_frontend::hir_def::types::Type as HirType; -use super::context::GlobalsContext; - /// Contains the entire SSA representation of the program. #[serde_as] #[derive(Serialize, Deserialize)] pub(crate) struct Ssa { #[serde_as(as = "Vec<(_, _)>")] pub(crate) functions: BTreeMap, - pub(crate) globals: GlobalsContext, + pub(crate) globals: DataFlowGraph, pub(crate) main_id: FunctionId, #[serde(skip)] pub(crate) next_id: AtomicCounter, @@ -58,7 +55,7 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, - globals: GlobalsContext::default(), + globals: DataFlowGraph::default(), } } @@ -105,41 +102,6 @@ impl Ssa { } } -impl Display for Ssa { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if self.globals.dfg.values_iter().len() > 0 { - write!(f, "{}", self.globals)?; - } - - for function in self.functions.values() { - writeln!(f, "{function}")?; - } - Ok(()) - } -} - -impl Display for GlobalsContext { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "Globals: ")?; - for (id, value) in self.dfg.values_iter() { - write!(f, "@")?; - match value { - Value::NumericConstant { constant, typ } => { - writeln!(f, "{id} = {typ} {constant}")?; - } - Value::Instruction { instruction, .. } => { - display_instruction(&self.dfg, *instruction, false, f)?; - } - Value::Global(_) => { - panic!("Value::Global should only be in the function dfg"); - } - _ => panic!("Expected only numeric constant or instruction"), - }; - } - writeln!(f) - } -} - #[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/monomorphization/ast.rs b/compiler/noirc_frontend/src/monomorphization/ast.rs index ee9174dd916..d219e8f7c2d 100644 --- a/compiler/noirc_frontend/src/monomorphization/ast.rs +++ b/compiler/noirc_frontend/src/monomorphization/ast.rs @@ -327,7 +327,7 @@ impl Type { } } -#[derive(Debug, Clone, Hash)] +#[derive(Debug, Clone, Hash, Default)] pub struct Program { pub functions: Vec, pub function_signatures: Vec, @@ -378,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 1fc74230984..b0c8744ea8f 100644 --- a/compiler/noirc_frontend/src/monomorphization/mod.rs +++ b/compiler/noirc_frontend/src/monomorphization/mod.rs @@ -947,52 +947,7 @@ impl<'interner> Monomorphizer<'interner> { } } DefinitionKind::Global(global_id) => { - let global = self.interner.get_global(*global_id); - let id = global.id; - let name = definition.name.clone(); - if let Some(seen_global) = self.globals.get(&id) { - let typ = Self::convert_type(&typ, ident.location)?; - let ident = ast::Ident { - location: Some(ident.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)?; - - 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, ident.location)?; - let ident = ast::Ident { - location: Some(ident.location), - definition: Definition::Global(new_id), - mutable: false, - name, - typ, - }; - ast::Expression::Ident(ident) - } else { - expr - } - } + self.global_ident(*global_id, definition.name.clone(), &typ, ident.location)? } DefinitionKind::Local(_) => match self.lookup_captured_expr(ident.id) { Some(expr) => expr, @@ -1039,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(); From 8ff2934db22e86cde9e3678f9302c9bdc37dda38 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 17:21:53 +0000 Subject: [PATCH 18/19] switch to Function from DataFlowGraph to represent globals --- .../noirc_evaluator/src/ssa/ir/function.rs | 16 ++++++++--- .../noirc_evaluator/src/ssa/ir/printer.rs | 6 ++--- .../noirc_evaluator/src/ssa/opt/inlining.rs | 10 +++---- .../src/ssa/opt/normalize_value_ids.rs | 2 +- .../src/ssa/ssa_gen/context.rs | 27 ++++++++++++++----- .../src/ssa/ssa_gen/program.rs | 7 ++--- 6 files changed, 46 insertions(+), 22 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index 4d1483913e1..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. diff --git a/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/compiler/noirc_evaluator/src/ssa/ir/printer.rs index bdd289a655a..7fe12b83ea9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -20,13 +20,13 @@ use super::{ impl Display for Ssa { fn fmt(&self, f: &mut Formatter<'_>) -> Result { - for (id, global_value) in self.globals.values_iter() { + 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, *instruction, true, f)?; + display_instruction(&self.globals.dfg, *instruction, true, f)?; } Value::Global(_) => { panic!("Value::Global should only be in the function dfg"); @@ -35,7 +35,7 @@ impl Display for Ssa { }; } - if self.globals.values_iter().len() > 0 { + if self.globals.dfg.values_iter().len() > 0 { writeln!(f)?; } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index c61ff208b18..eb3c93823bb 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -12,7 +12,7 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, call_stack::CallStackId, - dfg::{DataFlowGraph, InsertInstructionResult}, + dfg::InsertInstructionResult, function::{Function, FunctionId, RuntimeType}, instruction::{Instruction, InstructionId, TerminatorInstruction}, value::{Value, ValueId}, @@ -99,7 +99,7 @@ struct InlineContext<'global> { // These are the functions of the program that we shouldn't inline. functions_not_to_inline: BTreeSet, - globals: &'global DataFlowGraph, + globals: &'global Function, } /// The per-function inlining context contains information that is only valid for one function. @@ -382,7 +382,7 @@ impl<'global> InlineContext<'global> { let mut context = PerFunctionContext::new(&mut self, entry_point); context.inlining_entry = true; - for (_, value) in ssa.globals.values_iter() { + for (_, value) in ssa.globals.dfg.values_iter() { context.context.builder.current_function.dfg.make_global(value.get_type().into_owned()); } @@ -491,10 +491,10 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { 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.context.globals[id] { + match &self.context.globals.dfg[id] { Value::Instruction { instruction, .. } => { let Instruction::MakeArray { elements, typ } = - &self.context.globals[*instruction] + &self.context.globals.dfg[*instruction] else { panic!("Only expect Instruction::MakeArray for a global"); }; 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 af0add92799..5f21e3816f0 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -26,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, &self.globals); + context.normalize_ids(function, &self.globals.dfg); } self.functions = context.functions.into_btree(); } diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index af98eb6f93e..9aaa23cbd3b 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -11,7 +11,6 @@ use noirc_frontend::monomorphization::ast::{FuncId, Program}; use crate::errors::RuntimeError; use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::basic_block::BasicBlockId; -use crate::ssa::ir::dfg::DataFlowGraph; use crate::ssa::ir::function::FunctionId as IrFunctionId; use crate::ssa::ir::function::{Function, RuntimeType}; use crate::ssa::ir::instruction::BinaryOp; @@ -56,7 +55,6 @@ pub(super) struct FunctionContext<'a> { /// SSA can be generated by continuously popping from this function_queue and using /// FunctionContext to generate from the popped function id. Once the queue is empty, /// no other functions are reachable and the SSA generation is finished. -#[derive(Default)] pub(super) struct SharedContext { /// All currently known functions which have already been assigned function ids. /// These functions are all either currently having their SSA generated or are @@ -74,7 +72,11 @@ pub(super) struct SharedContext { /// Shared counter used to assign the ID of the next function function_counter: AtomicCounter, - pub(super) globals_context: DataFlowGraph, + /// 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, @@ -142,7 +144,7 @@ impl<'a> FunctionContext<'a> { } fn add_globals(&mut self) { - for (_, value) in self.shared_context.globals_context.values_iter() { + for (_, value) in self.shared_context.globals_context.dfg.values_iter() { self.builder.current_function.dfg.make_global(value.get_type().into_owned()); } } @@ -1044,7 +1046,7 @@ 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::default(); + let globals_shared_context = SharedContext::new_for_globals(); let globals_id = Program::global_space_id(); @@ -1068,11 +1070,24 @@ impl SharedContext { function_queue: Default::default(), function_counter: Default::default(), program, - globals_context: context.builder.current_function.dfg, + 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(), + } + } + /// Pops the next function from the shared function queue, returning None if the queue is empty. pub(super) fn pop_next_function_in_queue(&self) -> Option<(FuncId, IrFunctionId)> { self.function_queue.lock().expect("Failed to lock function_queue").pop() diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 79cbc8cadf2..f3474b9f4a5 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -6,7 +6,6 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use crate::ssa::ir::{ - dfg::DataFlowGraph, function::{Function, FunctionId}, map::AtomicCounter, }; @@ -18,7 +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: DataFlowGraph, + pub(crate) globals: Function, pub(crate) main_id: FunctionId, #[serde(skip)] pub(crate) next_id: AtomicCounter, @@ -55,7 +54,9 @@ impl Ssa { next_id: AtomicCounter::starting_after(max_id), entry_point_to_generated_index: BTreeMap::new(), error_selector_to_type: error_types, - globals: DataFlowGraph::default(), + // 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(), } } From aae17defa42e1f2cbb2871b81d81bdf63f578efa Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 17:48:50 +0000 Subject: [PATCH 19/19] remove unnecessary lifetime on InlineContext --- .../noirc_evaluator/src/ssa/opt/inlining.rs | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index eb3c93823bb..7dd8fe57735 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -80,7 +80,7 @@ impl Ssa { /// This works using an internal FunctionBuilder to build a new main function from scratch. /// Doing it this way properly handles importing instructions between functions and lets us /// reuse the existing API at the cost of essentially cloning each of main's instructions. -struct InlineContext<'global> { +struct InlineContext { recursion_level: u32, builder: FunctionBuilder, @@ -98,8 +98,6 @@ struct InlineContext<'global> { // These are the functions of the program that we shouldn't inline. functions_not_to_inline: BTreeSet, - - globals: &'global Function, } /// The per-function inlining context contains information that is only valid for one function. @@ -107,13 +105,13 @@ struct InlineContext<'global> { /// 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, 'global> { +struct PerFunctionContext<'function> { /// The source function is the function we're currently inlining into the function being built. source_function: &'function Function, /// The shared inlining context for all functions. This notably contains the FunctionBuilder used /// to build the function we're inlining into. - context: &'function mut InlineContext<'global>, + context: &'function mut InlineContext, /// Maps ValueIds in the function being inlined to the new ValueIds to use in the function /// being inlined into. This mapping also contains the mapping from parameter values to @@ -131,6 +129,8 @@ struct PerFunctionContext<'function, 'global> { /// 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. @@ -349,14 +349,14 @@ fn compute_function_interface_cost(func: &Function) -> usize { func.parameters().len() + func.returns().len() } -impl<'global> InlineContext<'global> { +impl InlineContext { /// Create a new context object for the function inlining pass. /// This starts off with an empty mapping of instructions for main's parameters. /// The function being inlined into will always be the main function, although it is /// actually a copy that is created in case the original main is still needed from a function /// that could not be inlined calling it. fn new( - ssa: &'global Ssa, + ssa: &Ssa, entry_point: FunctionId, inline_no_predicates_functions: bool, functions_not_to_inline: BTreeSet, @@ -371,7 +371,6 @@ impl<'global> InlineContext<'global> { call_stack: CallStackId::root(), inline_no_predicates_functions, functions_not_to_inline, - globals: &ssa.globals, } } @@ -379,7 +378,8 @@ impl<'global> InlineContext<'global> { 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() { @@ -429,7 +429,7 @@ impl<'global> InlineContext<'global> { ); } - 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()); @@ -444,14 +444,15 @@ impl<'global> InlineContext<'global> { } } -impl<'function, 'global> PerFunctionContext<'function, 'global> { +impl<'function> PerFunctionContext<'function> { /// Create a new PerFunctionContext from the source 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<'global>, + context: &'function mut InlineContext, source_function: &'function Function, + globals: &'function Function, ) -> Self { Self { context, @@ -459,6 +460,7 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { blocks: HashMap::default(), values: HashMap::default(), inlining_entry: false, + globals, } } @@ -491,10 +493,10 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { 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.context.globals.dfg[id] { + match &self.globals.dfg[id] { Value::Instruction { instruction, .. } => { let Instruction::MakeArray { elements, typ } = - &self.context.globals.dfg[*instruction] + &self.globals.dfg[*instruction] else { panic!("Only expect Instruction::MakeArray for a global"); };