From c7d8009072837ee105d8337eb273b88893235949 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 8 Jan 2025 02:46:27 +0000 Subject: [PATCH 01/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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/25] 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 5bbe0c943d00d815104b4d2f402e09d93c763b01 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 15:32:13 +0000 Subject: [PATCH 16/25] working brillig implementation of globals --- acvm-repo/brillig_vm/src/arithmetic.rs | 3 + .../src/brillig/brillig_gen.rs | 10 +- .../src/brillig/brillig_gen/brillig_block.rs | 54 +++- .../brillig_gen/brillig_block_variables.rs | 8 + .../src/brillig/brillig_gen/brillig_fn.rs | 8 +- .../brillig/brillig_gen/brillig_slice_ops.rs | 26 +- .../noirc_evaluator/src/brillig/brillig_ir.rs | 19 +- .../src/brillig/brillig_ir/artifact.rs | 7 + .../src/brillig/brillig_ir/entry_point.rs | 8 +- .../src/brillig/brillig_ir/instructions.rs | 6 + .../src/brillig/brillig_ir/registers.rs | 58 ++++- compiler/noirc_evaluator/src/brillig/mod.rs | 236 +++++++++++++++++- .../noirc_evaluator/src/ssa/opt/inlining.rs | 64 +++-- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 13 +- .../global_var_regression_simple/src/main.nr | 6 + tooling/nargo/src/errors.rs | 1 + tooling/nargo/src/ops/execute.rs | 15 +- 17 files changed, 476 insertions(+), 66 deletions(-) diff --git a/acvm-repo/brillig_vm/src/arithmetic.rs b/acvm-repo/brillig_vm/src/arithmetic.rs index 7cd31cd6443..720598717ca 100644 --- a/acvm-repo/brillig_vm/src/arithmetic.rs +++ b/acvm-repo/brillig_vm/src/arithmetic.rs @@ -69,6 +69,9 @@ pub(crate) fn evaluate_binary_int_op( rhs: MemoryValue, bit_size: IntegerBitSize, ) -> Result, BrilligArithmeticError> { + // dbg!(op); + // dbg!(lhs); + // dbg!(rhs); let lhs = lhs.expect_integer_with_bit_size(bit_size).map_err(|err| match err { MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } => { BrilligArithmeticError::MismatchedLhsBitSize { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index 5a81c79ae0d..d9ab4ef21c7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -7,6 +7,7 @@ mod constant_allocation; mod variable_liveness; use acvm::FieldElement; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; use super::{ @@ -14,7 +15,7 @@ use super::{ artifact::{BrilligArtifact, BrilligParameter, GeneratedBrillig, Label}, BrilligContext, }, - Brillig, + Brillig, BrilligVariable, ValueId, }; use crate::{ errors::InternalError, @@ -25,17 +26,20 @@ use crate::{ pub(crate) fn convert_ssa_function( func: &Function, enable_debug_trace: bool, + globals: &HashMap, ) -> BrilligArtifact { let mut brillig_context = BrilligContext::new(enable_debug_trace); - let mut function_context = FunctionContext::new(func); + let global_values = globals.iter().map(|(value, _)| *value).collect::>(); + + let mut function_context = FunctionContext::new(func, globals); brillig_context.enter_context(Label::function(func.id())); brillig_context.call_check_max_stack_depth_procedure(); for block in function_context.blocks.clone() { - BrilligBlock::compile(&mut function_context, &mut brillig_context, block, &func.dfg); + BrilligBlock::compile(&mut function_context, &mut brillig_context, block, &func.dfg, &global_values); } let mut artifact = brillig_context.artifact(); 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 13a156953fc..fcb3855913e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -32,8 +32,8 @@ use super::brillig_fn::FunctionContext; use super::constant_allocation::InstructionLocation; /// Generate the compilation artifacts for compiling a function into brillig bytecode. -pub(crate) struct BrilligBlock<'block> { - pub(crate) function_context: &'block mut FunctionContext, +pub(crate) struct BrilligBlock<'block, 'global> { + pub(crate) function_context: &'block mut FunctionContext<'global>, /// The basic block that is being converted pub(crate) block_id: BasicBlockId, /// Context for creating brillig opcodes @@ -44,16 +44,37 @@ pub(crate) struct BrilligBlock<'block> { pub(crate) last_uses: HashMap>, } -impl<'block> BrilligBlock<'block> { +impl<'block, 'global> BrilligBlock<'block, 'global> { /// Converts an SSA Basic block into a sequence of Brillig opcodes pub(crate) fn compile( - function_context: &'block mut FunctionContext, + function_context: &'block mut FunctionContext<'global>, brillig_context: &'block mut BrilligContext, block_id: BasicBlockId, dfg: &DataFlowGraph, + globals: &HashSet, ) { let live_in = function_context.liveness.get_live_in(&block_id); - let variables = BlockVariables::new(live_in.clone()); + + // let live_in = live_in.into_iter().filter(|value| { + // !matches!(&dfg[**value], Value::Global(_)) + // }).collect(); + + let mut live_in_no_globals = HashSet::default(); + for value in live_in { + if !matches!(&dfg[*value], Value::Global(_)) { + live_in_no_globals.insert(*value); + } + } + + let variables = BlockVariables::new(live_in_no_globals); + + // let mut live_in_no_globals = HashSet::default(); + // for value in live_in { + // if let Value::Global(_) = &dfg[*value] { + // continue; + // } + // } + brillig_context.set_allocated_registers( variables @@ -849,12 +870,20 @@ impl<'block> BrilligBlock<'block> { .expect("Last uses for instruction should have been computed"); for dead_variable in dead_variables { - self.variables.remove_variable( - dead_variable, - self.function_context, - self.brillig_context, - ); + match &dfg[*dead_variable] { + Value::Global(_) => { + dbg!("got dead global"); + } + _ => { + self.variables.remove_variable( + dead_variable, + self.function_context, + self.brillig_context, + ); + } + } } + self.brillig_context.set_call_stack(CallStack::new()); } @@ -1561,7 +1590,10 @@ impl<'block> BrilligBlock<'block> { match value { Value::Global(_) => { - unreachable!("ICE: All globals should have been inlined"); + dbg!(value_id); + let variable = *self.function_context.globals.get(&value_id).unwrap_or_else(|| panic!("ICE: Global value not found in cache {value_id}")); + dbg!(variable.clone()); + variable } Value::Param { .. } | Value::Instruction { .. } => { // All block parameters and instruction results should have already been diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index bf0a1bc7347..06e65471538 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -133,6 +133,14 @@ pub(crate) fn allocate_value( ) -> BrilligVariable { let typ = dfg.type_of_value(value_id); + allocate_value_with_type(brillig_context, typ) +} + +/// For a given value_id, allocates the necessary registers to hold it. +pub(crate) fn allocate_value_with_type( + brillig_context: &mut BrilligContext, + typ: Type, +) -> BrilligVariable { match typ { Type::Numeric(_) | Type::Reference(_) | Type::Function => { BrilligVariable::SingleAddr(SingleAddrVariable { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 3dea7b3e7f5..b9b9536bc57 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -17,7 +17,7 @@ use fxhash::FxHashMap as HashMap; use super::{constant_allocation::ConstantAllocation, variable_liveness::VariableLiveness}; -pub(crate) struct FunctionContext { +pub(crate) struct FunctionContext<'global> { pub(crate) function_id: FunctionId, /// Map from SSA values its allocation. Since values can be only defined once in SSA form, we insert them here on when we allocate them at their definition. pub(crate) ssa_value_allocations: HashMap, @@ -27,11 +27,12 @@ pub(crate) struct FunctionContext { pub(crate) liveness: VariableLiveness, /// Information on where to allocate constants pub(crate) constant_allocation: ConstantAllocation, + pub(crate) globals: &'global HashMap, } -impl FunctionContext { +impl<'global> FunctionContext<'global> { /// Creates a new function context. It will allocate parameters for all blocks and compute the liveness of every variable. - pub(crate) fn new(function: &Function) -> Self { + pub(crate) fn new(function: &Function, globals: &'global HashMap) -> Self { let id = function.id(); let mut reverse_post_order = Vec::new(); @@ -47,6 +48,7 @@ impl FunctionContext { blocks: reverse_post_order, liveness, constant_allocation: constants, + globals } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index 26c7151bf07..8d9a531f674 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -7,7 +7,7 @@ use crate::brillig::brillig_ir::{ use super::brillig_block::BrilligBlock; -impl<'block> BrilligBlock<'block> { +impl<'block, 'global> BrilligBlock<'block, 'global> { fn write_variables(&mut self, write_pointer: MemoryAddress, variables: &[BrilligVariable]) { for (index, variable) in variables.iter().enumerate() { self.brillig_context.store_instruction(write_pointer, variable.extract_register()); @@ -159,6 +159,7 @@ mod tests { use std::vec; use acvm::FieldElement; + use fxhash::FxHashMap as HashMap; use noirc_frontend::monomorphization::ast::InlineType; use crate::brillig::brillig_gen::brillig_block::BrilligBlock; @@ -173,12 +174,13 @@ mod tests { create_and_run_vm, create_context, create_entry_point_bytecode, }; use crate::brillig::brillig_ir::{BrilligContext, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE}; + use crate::brillig::ValueId; use crate::ssa::function_builder::FunctionBuilder; use crate::ssa::ir::function::RuntimeType; use crate::ssa::ir::map::Id; use crate::ssa::ssa_gen::Ssa; - fn create_test_environment() -> (Ssa, FunctionContext, BrilligContext) { + fn create_test_environment(brillig_globals: &HashMap) -> (Ssa, FunctionContext, BrilligContext) { let mut builder = FunctionBuilder::new("main".to_string(), Id::test_new(0)); builder.set_runtime(RuntimeType::Brillig(InlineType::default())); @@ -186,14 +188,14 @@ mod tests { let mut brillig_context = create_context(ssa.main_id); brillig_context.enter_context(Label::block(ssa.main_id, Id::test_new(0))); - let function_context = FunctionContext::new(ssa.main()); + let function_context = FunctionContext::new(ssa.main(), brillig_globals); (ssa, function_context, brillig_context) } - fn create_brillig_block<'a>( - function_context: &'a mut FunctionContext, + fn create_brillig_block<'a, 'global>( + function_context: &'a mut FunctionContext<'global>, brillig_context: &'a mut BrilligContext, - ) -> BrilligBlock<'a> { + ) -> BrilligBlock<'a, 'global> { let variables = BlockVariables::default(); BrilligBlock { function_context, @@ -230,7 +232,8 @@ mod tests { result_length_with_metadata, )]; - let (_, mut function_context, mut context) = create_test_environment(); + let brillig_globals = HashMap::default(); + let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters let source_vector = BrilligVector { pointer: context.allocate_register() }; @@ -346,7 +349,8 @@ mod tests { ), ]; - let (_, mut function_context, mut context) = create_test_environment(); + let brillig_globals = HashMap::default(); + let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters let source_vector = BrilligVector { pointer: context.allocate_register() }; @@ -448,7 +452,8 @@ mod tests { result_length_with_metadata, )]; - let (_, mut function_context, mut context) = create_test_environment(); + let brillig_globals = HashMap::default(); + let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters let source_vector = BrilligVector { pointer: context.allocate_register() }; @@ -588,7 +593,8 @@ mod tests { ), ]; - let (_, mut function_context, mut context) = create_test_environment(); + let brillig_globals = HashMap::default(); + let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters let source_vector = BrilligVector { pointer: context.allocate_register() }; diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 55e12c993fa..8c2ed0c2c34 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -37,7 +37,7 @@ use acvm::{ }; use debug_show::DebugShow; -use super::ProcedureId; +use super::{GlobalSpace, ProcedureId}; /// The Brillig VM does not apply a limit to the memory address space, /// As a convention, we take use 32 bits. This means that we assume that @@ -213,6 +213,23 @@ impl BrilligContext { } } +/// Special brillig context to codegen global values initialization +impl BrilligContext { + pub(crate) fn new_for_global_init( + enable_debug_trace: bool, + ) -> BrilligContext { + BrilligContext { + obj: BrilligArtifact::default(), + registers: GlobalSpace::new(), + context_label: Label::globals_init(), + current_section: 0, + next_section: 1, + debug_show: DebugShow::new(enable_debug_trace), + can_call_procedures: false, + } + } +} + impl BrilligContext { /// Adds a brillig instruction to the brillig byte code fn push_opcode(&mut self, opcode: BrilligOpcode) { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index 3654a95a03f..dc71fc9b28b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -75,6 +75,8 @@ pub(crate) enum LabelType { Function(FunctionId, Option), /// Labels for intrinsic procedures Procedure(ProcedureId), + /// Label for initialization of globals + GlobalInit, } impl std::fmt::Display for LabelType { @@ -89,6 +91,7 @@ impl std::fmt::Display for LabelType { } LabelType::Entrypoint => write!(f, "Entrypoint"), LabelType::Procedure(procedure_id) => write!(f, "Procedure({:?})", procedure_id), + LabelType::GlobalInit => write!(f, "Globals Initialization"), } } } @@ -123,6 +126,10 @@ impl Label { pub(crate) fn procedure(procedure_id: ProcedureId) -> Self { Label { label_type: LabelType::Procedure(procedure_id), section: None } } + + pub(crate) fn globals_init() -> Self { + Label { label_type: LabelType::GlobalInit, section: None } + } } impl std::fmt::Display for Label { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 2dbee48b277..29ca46964b7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -15,6 +15,7 @@ use acvm::acir::{ pub(crate) const MAX_STACK_SIZE: usize = 16 * MAX_STACK_FRAME_SIZE; pub(crate) const MAX_STACK_FRAME_SIZE: usize = 2048; pub(crate) const MAX_SCRATCH_SPACE: usize = 64; +pub(crate) const MAX_GLOBAL_SPACE: usize = 2048; impl BrilligContext { /// Creates an entry point artifact that will jump to the function label provided. @@ -27,6 +28,8 @@ impl BrilligContext { context.codegen_entry_point(&arguments, &return_parameters); + context.add_globals_init_instruction(); + context.add_external_call_instruction(target_function); context.codegen_exit_point(&arguments, &return_parameters); @@ -34,7 +37,7 @@ impl BrilligContext { } fn calldata_start_offset() -> usize { - ReservedRegisters::len() + MAX_STACK_SIZE + MAX_SCRATCH_SPACE + ReservedRegisters::len() + MAX_STACK_SIZE + MAX_SCRATCH_SPACE + MAX_GLOBAL_SPACE } fn return_data_start_offset(calldata_size: usize) -> usize { @@ -77,7 +80,7 @@ impl BrilligContext { self.copy_and_cast_calldata(arguments); let mut current_calldata_pointer = Self::calldata_start_offset(); - + dbg!(current_calldata_pointer); // Initialize the variables with the calldata for (argument_variable, argument) in argument_variables.iter_mut().zip(arguments) { match (argument_variable, argument) { @@ -331,6 +334,7 @@ impl BrilligContext { // Return data has a reserved space after calldata let return_data_offset = Self::return_data_start_offset(calldata_size); + dbg!(return_data_offset); let mut return_data_index = return_data_offset; for (return_param, returned_variable) in return_parameters.iter().zip(&returned_variables) { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs index 2bf5364414c..8b2b492e880 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs @@ -200,6 +200,12 @@ impl BrilligContext< self.obj.add_unresolved_external_call(BrilligOpcode::Call { location: 0 }, proc_label); } + pub(super) fn add_globals_init_instruction(&mut self) { + let globals_init_label = Label::globals_init(); + self.debug_show.add_external_call_instruction(globals_init_label.to_string()); + self.obj.add_unresolved_external_call(BrilligOpcode::Call { location: 0 }, globals_init_label); + } + /// Adds a unresolved `Jump` instruction to the bytecode. pub(crate) fn jump_instruction(&mut self, target_label: Label) { self.debug_show.jump_instruction(target_label.to_string()); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs index dd7766f40aa..52889590b5e 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs @@ -7,7 +7,7 @@ use crate::brillig::brillig_ir::entry_point::MAX_STACK_SIZE; use super::{ brillig_variable::SingleAddrVariable, - entry_point::{MAX_SCRATCH_SPACE, MAX_STACK_FRAME_SIZE}, + entry_point::{MAX_GLOBAL_SPACE, MAX_SCRATCH_SPACE, MAX_STACK_FRAME_SIZE}, BrilligContext, ReservedRegisters, }; @@ -141,6 +141,62 @@ impl RegisterAllocator for ScratchSpace { } } +/// Globals have a separate memory space +/// This memory space is initialized once at the beginning of a program +/// is read-only. +pub(crate) struct GlobalSpace { + storage: DeallocationListAllocator +} + +impl GlobalSpace { + pub(crate) fn new() -> Self { + Self { storage: DeallocationListAllocator::new(Self::start()) } + } + + fn is_within_bounds(register: MemoryAddress) -> bool { + let index = register.unwrap_direct(); + index >= Self::start() && index < Self::end() + } +} + +impl RegisterAllocator for GlobalSpace { + fn start() -> usize { + ScratchSpace::end() + } + + fn end() -> usize { + Self::start() + MAX_GLOBAL_SPACE + } + + fn allocate_register(&mut self) -> MemoryAddress { + let allocated = MemoryAddress::direct(self.storage.allocate_register()); + assert!(Self::is_within_bounds(allocated), "Scratch space too deep"); + allocated + } + + fn deallocate_register(&mut self, register_index: MemoryAddress) { + self.storage.deallocate_register(register_index.unwrap_direct()); + } + + fn ensure_register_is_allocated(&mut self, register: MemoryAddress) { + assert!(Self::is_within_bounds(register), "Register out of scratch space bounds"); + self.storage.ensure_register_is_allocated(register.unwrap_direct()); + } + + fn from_preallocated_registers(preallocated_registers: Vec) -> Self { + for register in &preallocated_registers { + assert!(Self::is_within_bounds(*register), "Register out of scratch space bounds"); + } + + Self { + storage: DeallocationListAllocator::from_preallocated_registers( + Self::start(), + vecmap(preallocated_registers, |r| r.unwrap_direct()), + ), + } + } +} + struct DeallocationListAllocator { /// A free-list of registers that have been deallocated and can be used again. deallocated_registers: BTreeSet, diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index cb8c35cd8e0..b7f6a4eb714 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -1,8 +1,9 @@ pub(crate) mod brillig_gen; pub(crate) mod brillig_ir; -use acvm::FieldElement; -use brillig_ir::artifact::LabelType; +use acvm::{acir::brillig::MemoryAddress, FieldElement}; +use brillig_gen::brillig_block_variables::allocate_value_with_type; +use brillig_ir::{artifact::LabelType, brillig_variable::{BrilligVariable, SingleAddrVariable}, registers::GlobalSpace, BrilligBinaryOp, BrilligContext, ReservedRegisters}; use self::{ brillig_gen::convert_ssa_function, @@ -12,11 +13,11 @@ use self::{ }, }; use crate::ssa::{ - ir::function::{Function, FunctionId}, + ir::{dfg::DataFlowGraph, function::{Function, FunctionId}, instruction::Instruction, types::Type, value::{Value, ValueId}}, ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; -use std::{borrow::Cow, collections::BTreeSet}; +use std::{borrow::Cow, collections::BTreeSet, sync::Arc}; pub use self::brillig_ir::procedures::ProcedureId; @@ -26,12 +27,13 @@ pub use self::brillig_ir::procedures::ProcedureId; pub struct Brillig { /// Maps SSA function labels to their brillig artifact ssa_function_to_brillig: HashMap>, + globals: BrilligArtifact, } impl Brillig { /// Compiles a function into brillig and store the compilation artifacts - pub(crate) fn compile(&mut self, func: &Function, enable_debug_trace: bool) { - let obj = convert_ssa_function(func, enable_debug_trace); + pub(crate) fn compile(&mut self, func: &Function, enable_debug_trace: bool, globals: &HashMap) { + let obj = convert_ssa_function(func, enable_debug_trace, globals); self.ssa_function_to_brillig.insert(func.id(), obj); } @@ -46,9 +48,220 @@ impl Brillig { } // Procedures are compiled as needed LabelType::Procedure(procedure_id) => Some(Cow::Owned(compile_procedure(procedure_id))), + LabelType::GlobalInit => Some(Cow::Borrowed(&self.globals)), _ => unreachable!("ICE: Expected a function or procedure label"), } } + + pub(crate) fn create_brillig_globals(brillig_context: &mut BrilligContext, globals: &DataFlowGraph) -> HashMap { + let mut brillig_globals = HashMap::default(); + for (id, value) in globals.values_iter() { + match value { + Value::NumericConstant { constant, typ } => { + let new_variable = allocate_value_with_type(brillig_context, Type::Numeric(*typ)); + dbg!(new_variable.clone()); + brillig_context.const_instruction(new_variable.extract_single_addr(), *constant); + + brillig_globals.insert(id, new_variable); + } + Value::Instruction { instruction, .. } => { + let result = globals.instruction_results(*instruction)[0]; + dbg!(result); + let instruction = &globals[*instruction]; + match &instruction { + Instruction::MakeArray { elements: array, typ } => { + let new_variable = allocate_value_with_type(brillig_context, typ.clone()); + // Initialize the variable + match new_variable { + BrilligVariable::BrilligArray(brillig_array) => { + brillig_context.codegen_initialize_array(brillig_array); + } + BrilligVariable::BrilligVector(vector) => { + let size = brillig_context + .make_usize_constant_instruction(array.len().into()); + brillig_context.codegen_initialize_vector(vector, size, None); + brillig_context.deallocate_single_addr(size); + } + _ => unreachable!( + "ICE: Cannot initialize array value created as {new_variable:?}" + ), + }; + + // Write the items + let items_pointer = brillig_context + .codegen_make_array_or_vector_items_pointer(new_variable); + + Self::initialize_constant_array(array, typ, items_pointer, brillig_context, &brillig_globals); + + brillig_context.deallocate_register(items_pointer); + + dbg!(new_variable.clone()); + brillig_globals.insert(result, new_variable); + } + _ => unreachable!("Expected MakeArray instruction but got {instruction:#?}") + } + } + _ => { + panic!("got something other than numeric constant") + } + } + } + brillig_globals + } + + fn initialize_constant_array( + data: &im::Vector, + typ: &Type, + pointer: MemoryAddress, + brillig_context: &mut BrilligContext, + brillig_globals: &HashMap, + ) { + if data.is_empty() { + return; + } + let item_types = typ.clone().element_types(); + + // Find out if we are repeating the same item over and over + let first_item = data.iter().take(item_types.len()).copied().collect(); + let mut is_repeating = true; + + for item_index in (item_types.len()..data.len()).step_by(item_types.len()) { + let item: Vec<_> = (0..item_types.len()).map(|i| data[item_index + i]).collect(); + if first_item != item { + is_repeating = false; + break; + } + } + + // If all the items are single address, and all have the same initial value, we can initialize the array in a runtime loop. + // Since the cost in instructions for a runtime loop is in the order of magnitude of 10, we only do this if the item_count is bigger than that. + let item_count = data.len() / item_types.len(); + + if item_count > 10 + && is_repeating + && item_types.iter().all(|typ| matches!(typ, Type::Numeric(_))) + { + dbg!("initializing runtime"); + Self::initialize_constant_array_runtime( + item_types, first_item, item_count, pointer, brillig_context, &brillig_globals + ); + } else { + dbg!("initializing comptime"); + Self::initialize_constant_array_comptime(data, pointer, brillig_context, &brillig_globals); + } + } + + fn initialize_constant_array_runtime( + item_types: Arc>, + item_to_repeat: Vec, + item_count: usize, + pointer: MemoryAddress, + brillig_context: &mut BrilligContext, + brillig_globals: &HashMap, + ) { + let mut subitem_to_repeat_variables = Vec::with_capacity(item_types.len()); + for subitem_id in item_to_repeat.into_iter() { + subitem_to_repeat_variables.push(*brillig_globals.get(&subitem_id).unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available"))); + } + + // Initialize loop bound with the array length + let end_pointer_variable = brillig_context + .make_usize_constant_instruction((item_count * item_types.len()).into()); + + // Add the pointer to the array length + brillig_context.memory_op_instruction( + end_pointer_variable.address, + pointer, + end_pointer_variable.address, + BrilligBinaryOp::Add, + ); + + // If this is an array with complex subitems, we need a custom step in the loop to write all the subitems while iterating. + if item_types.len() > 1 { + let step_variable = + brillig_context.make_usize_constant_instruction(item_types.len().into()); + + let subitem_pointer = + SingleAddrVariable::new_usize(brillig_context.allocate_register()); + + // Initializes a single subitem + let initializer_fn = + |ctx: &mut BrilligContext<_, _>, subitem_start_pointer: SingleAddrVariable| { + ctx.mov_instruction(subitem_pointer.address, subitem_start_pointer.address); + for (subitem_index, subitem) in + subitem_to_repeat_variables.into_iter().enumerate() + { + ctx.store_instruction(subitem_pointer.address, subitem.extract_register()); + if subitem_index != item_types.len() - 1 { + ctx.memory_op_instruction( + subitem_pointer.address, + ReservedRegisters::usize_one(), + subitem_pointer.address, + BrilligBinaryOp::Add, + ); + } + } + }; + + // for (let subitem_start_pointer = pointer; subitem_start_pointer < pointer + data_length; subitem_start_pointer += step) { initializer_fn(iterator) } + brillig_context.codegen_for_loop( + Some(pointer), + end_pointer_variable.address, + Some(step_variable.address), + initializer_fn, + ); + + brillig_context.deallocate_single_addr(step_variable); + brillig_context.deallocate_single_addr(subitem_pointer); + } else { + let subitem = subitem_to_repeat_variables.into_iter().next().unwrap(); + + let initializer_fn = + |ctx: &mut BrilligContext<_, _>, item_pointer: SingleAddrVariable| { + ctx.store_instruction(item_pointer.address, subitem.extract_register()); + }; + + // for (let item_pointer = pointer; item_pointer < pointer + data_length; item_pointer += 1) { initializer_fn(iterator) } + brillig_context.codegen_for_loop( + Some(pointer), + end_pointer_variable.address, + None, + initializer_fn, + ); + } + brillig_context.deallocate_single_addr(end_pointer_variable); + } + + fn initialize_constant_array_comptime( + data: &im::Vector>, + pointer: MemoryAddress, + brillig_context: &mut BrilligContext, + brillig_globals: &HashMap, + ) { + // Allocate a register for the iterator + let write_pointer_register = brillig_context.allocate_register(); + + brillig_context.mov_instruction(write_pointer_register, pointer); + + for (element_idx, element_id) in data.iter().enumerate() { + let element_variable = *brillig_globals.get(&element_id).unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); + // Store the item in memory + brillig_context + .store_instruction(write_pointer_register, element_variable.extract_register()); + + if element_idx != data.len() - 1 { + // Increment the write_pointer_register + brillig_context.memory_op_instruction( + write_pointer_register, + ReservedRegisters::usize_one(), + write_pointer_register, + BrilligBinaryOp::Add, + ); + } + } + + brillig_context.deallocate_register(write_pointer_register); + } } impl std::ops::Index for Brillig { @@ -71,9 +284,18 @@ impl Ssa { .collect::>(); let mut brillig = Brillig::default(); + + let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace); + brillig_context.enter_context(Label::globals_init()); + let brillig_globals = Brillig::create_brillig_globals(&mut brillig_context, &self.globals.dfg); + brillig_context.return_instruction(); + + let artifact = brillig_context.artifact(); + brillig.globals = artifact; + for brillig_function_id in brillig_reachable_function_ids { let func = &self.functions[&brillig_function_id]; - brillig.compile(func, enable_debug_trace); + brillig.compile(func, enable_debug_trace, &brillig_globals); } brillig diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index d4d6f745788..19d4ffad43b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -475,6 +475,11 @@ impl<'function, 'global> PerFunctionContext<'function, 'global> { let new_value = match &self.source_function.dfg[id] { value @ Value::Instruction { .. } => { + // if self.context.globals.dfg.values_iter().len() > id.to_u32() as usize { + // id + // } else { + // unreachable!("All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value {id} = {value:?}") + // } unreachable!("All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value {id} = {value:?}") } value @ Value::Param { .. } => { @@ -489,26 +494,49 @@ 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 - // until Brillig gen with globals is working end to end - 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) + if self.context.builder.current_function.dfg.runtime().is_acir() { + 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"), } - _ => panic!("Expected only an instruction or a numeric constant"), + } else { + id } + // 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] { + // 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"), + // } + // id } }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index ab4256197b9..d987f3f9c4b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -24,7 +24,7 @@ use acvm::{acir::AcirField, FieldElement}; use im::HashSet; use crate::{ - brillig::brillig_gen::convert_ssa_function, + brillig::{brillig_gen::convert_ssa_function, brillig_ir::{brillig_variable::BrilligVariable, BrilligContext}, Brillig}, errors::RuntimeError, ssa::{ ir::{ @@ -87,8 +87,11 @@ impl Ssa { if has_unrolled { if let Some((orig_function, max_incr_pct)) = orig_func_and_max_incr_pct { - let new_size = brillig_bytecode_size(function); - let orig_size = brillig_bytecode_size(&orig_function); + let mut brillig_context = BrilligContext::new_for_global_init(true); + let brillig_globals = Brillig::create_brillig_globals(&mut brillig_context, &self.globals.dfg); + + let new_size = brillig_bytecode_size(function, &brillig_globals); + let orig_size = brillig_bytecode_size(&orig_function, &brillig_globals); if !is_new_size_ok(orig_size, new_size, max_incr_pct) { *function = orig_function; } @@ -974,7 +977,7 @@ fn simplify_between_unrolls(function: &mut Function) { } /// Convert the function to Brillig bytecode and return the resulting size. -fn brillig_bytecode_size(function: &Function) -> usize { +fn brillig_bytecode_size(function: &Function, globals: &HashMap) -> usize { // We need to do some SSA passes in order for the conversion to be able to go ahead, // otherwise we can hit `unreachable!()` instructions in `convert_ssa_instruction`. // Creating a clone so as not to modify the originals. @@ -986,7 +989,7 @@ fn brillig_bytecode_size(function: &Function) -> usize { // This is to try to prevent hitting ICE. temp.dead_instruction_elimination(false); - convert_ssa_function(&temp, false).byte_code.len() + convert_ssa_function(&temp, false, globals).byte_code.len() } /// Decide if the new bytecode size is acceptable, compared to the original. 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..fa4557e63d9 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,9 +1,14 @@ global EXPONENTIATE: [[Field; 2]; 2] = [[1, 1], [0, 0]]; +// global EXPONENTIATE: [Field; 2] = [1, 1]; +// global EXPONENTIATE: Field = 10; + fn main(x: Field, y: pub Field) { let mut acc: Field = 0; for i in 0..2 { for j in 0..2 { + // let got_i = EXPONENTIATE[i]; + // acc += got_i[j]; acc += EXPONENTIATE[i][j]; } } @@ -18,6 +23,7 @@ fn dummy_again(x: Field, y: Field) { for i in 0..2 { for j in 0..2 { acc += EXPONENTIATE[i][j]; + // acc += EXPONENTIATE[j]; } } assert(!acc.lt(x)); diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index 00c411bf7e4..0b7a02b8af9 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -237,6 +237,7 @@ pub fn try_to_diagnose_runtime_error( } _ => return None, }; + dbg!(source_locations.clone()); // The location of the error itself will be the location at the top // of the call stack (the last item in the Vec). let location = *source_locations.last()?; diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 57116ec2efd..76f44092018 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -137,11 +137,16 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> }; return Err(NargoError::ExecutionError(match assertion_payload { - Some(payload) => ExecutionError::AssertionFailed( - payload, - call_stack.expect("Should have call stack for an assertion failure"), - brillig_function_id, - ), + Some(payload) => { + dbg!(payload.clone()); + dbg!(call_stack.clone()); + dbg!(brillig_function_id); + ExecutionError::AssertionFailed( + payload, + call_stack.expect("Should have call stack for an assertion failure"), + brillig_function_id, + ) + } None => ExecutionError::SolvingError(error, call_stack), })); } From bb5a9ea6f5ec88f9335926d07cb09058a38c24ad Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 11:49:22 -0500 Subject: [PATCH 17/25] 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 fa0122d000f6c759bc9d86dfbf5ef0fa2617e444 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 18:50:06 +0000 Subject: [PATCH 18/25] make separate BrilligsGlobal struct --- .../src/brillig/brillig_gen.rs | 5 +- .../src/brillig/brillig_gen/brillig_block.rs | 15 +- .../brillig/brillig_gen/brillig_globals.rs | 227 ++++++++++++++++++ 3 files changed, 230 insertions(+), 17 deletions(-) create mode 100644 compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index d9ab4ef21c7..d40863b8e80 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -3,6 +3,7 @@ pub(crate) mod brillig_block; pub(crate) mod brillig_block_variables; pub(crate) mod brillig_fn; pub(crate) mod brillig_slice_ops; +pub(crate) mod brillig_globals; mod constant_allocation; mod variable_liveness; @@ -30,8 +31,6 @@ pub(crate) fn convert_ssa_function( ) -> BrilligArtifact { let mut brillig_context = BrilligContext::new(enable_debug_trace); - let global_values = globals.iter().map(|(value, _)| *value).collect::>(); - let mut function_context = FunctionContext::new(func, globals); brillig_context.enter_context(Label::function(func.id())); @@ -39,7 +38,7 @@ pub(crate) fn convert_ssa_function( brillig_context.call_check_max_stack_depth_procedure(); for block in function_context.blocks.clone() { - BrilligBlock::compile(&mut function_context, &mut brillig_context, block, &func.dfg, &global_values); + BrilligBlock::compile(&mut function_context, &mut brillig_context, block, &func.dfg); } let mut artifact = brillig_context.artifact(); 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 fcb3855913e..4601a033ca8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -3,7 +3,7 @@ use crate::brillig::brillig_ir::brillig_variable::{ type_to_heap_value_type, BrilligArray, BrilligVariable, SingleAddrVariable, }; -use crate::brillig::brillig_ir::registers::Stack; +use crate::brillig::brillig_ir::registers::{RegisterAllocator, Stack}; use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; @@ -51,14 +51,9 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { brillig_context: &'block mut BrilligContext, block_id: BasicBlockId, dfg: &DataFlowGraph, - globals: &HashSet, ) { let live_in = function_context.liveness.get_live_in(&block_id); - // let live_in = live_in.into_iter().filter(|value| { - // !matches!(&dfg[**value], Value::Global(_)) - // }).collect(); - let mut live_in_no_globals = HashSet::default(); for value in live_in { if !matches!(&dfg[*value], Value::Global(_)) { @@ -68,14 +63,6 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { let variables = BlockVariables::new(live_in_no_globals); - // let mut live_in_no_globals = HashSet::default(); - // for value in live_in { - // if let Value::Global(_) = &dfg[*value] { - // continue; - // } - // } - - brillig_context.set_allocated_registers( variables .get_available_variables(function_context) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs new file mode 100644 index 00000000000..de608f5d695 --- /dev/null +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -0,0 +1,227 @@ +use std::sync::Arc; + +use acvm::{acir::brillig::MemoryAddress, FieldElement}; +use fxhash::FxHashMap as HashMap; + +use crate::brillig::{allocate_value_with_type, brillig_gen::BrilligBlock, brillig_ir::BrilligContext, BrilligBinaryOp, DataFlowGraph, GlobalSpace, Instruction, ReservedRegisters, SingleAddrVariable, Type, Value}; +use super::{BrilligVariable, ValueId}; + +pub(crate) struct BrilligGlobals<'global> { + pub(crate) brillig_context: &'global mut BrilligContext, + + brillig_globals: HashMap, +} + +impl<'global> BrilligGlobals<'global> { + + pub(crate) fn get_globals(self) -> HashMap { + self.brillig_globals + } + + pub(crate) fn create_brillig_globals(&mut self, globals: &DataFlowGraph) { + for (id, value) in globals.values_iter() { + match value { + Value::NumericConstant { constant, typ } => { + let new_variable = allocate_value_with_type(self.brillig_context, Type::Numeric(*typ)); + self.brillig_context.const_instruction(new_variable.extract_single_addr(), *constant); + + self.brillig_globals.insert(id, new_variable); + } + Value::Instruction { instruction, .. } => { + + let result = globals.instruction_results(*instruction)[0]; + let instruction = &globals[*instruction]; + match &instruction { + Instruction::MakeArray { elements: array, typ } => { + let new_variable = allocate_value_with_type(self.brillig_context, typ.clone()); + // Initialize the variable + match new_variable { + BrilligVariable::BrilligArray(brillig_array) => { + self.brillig_context.codegen_initialize_array(brillig_array); + } + BrilligVariable::BrilligVector(vector) => { + let size = self.brillig_context + .make_usize_constant_instruction(array.len().into()); + self.brillig_context.codegen_initialize_vector(vector, size, None); + self.brillig_context.deallocate_single_addr(size); + } + _ => unreachable!( + "ICE: Cannot initialize array value created as {new_variable:?}" + ), + }; + + // Write the items + let items_pointer = self.brillig_context + .codegen_make_array_or_vector_items_pointer(new_variable); + + self.initialize_constant_array(array, typ, items_pointer); + + self.brillig_context.deallocate_register(items_pointer); + + self.brillig_globals.insert(result, new_variable); + } + _ => unreachable!("Expected MakeArray instruction but got {instruction:#?}") + } + } + _ => { + panic!("got something other than numeric constant") + } + } + } + } + + fn initialize_constant_array( + &mut self, + data: &im::Vector, + typ: &Type, + pointer: MemoryAddress, + // brillig_globals: &HashMap, + // globals: &DataFlowGraph, + ) { + if data.is_empty() { + return; + } + let item_types = typ.clone().element_types(); + + // Find out if we are repeating the same item over and over + let first_item = data.iter().take(item_types.len()).copied().collect(); + let mut is_repeating = true; + + for item_index in (item_types.len()..data.len()).step_by(item_types.len()) { + let item: Vec<_> = (0..item_types.len()).map(|i| data[item_index + i]).collect(); + if first_item != item { + is_repeating = false; + break; + } + } + + // If all the items are single address, and all have the same initial value, we can initialize the array in a runtime loop. + // Since the cost in instructions for a runtime loop is in the order of magnitude of 10, we only do this if the item_count is bigger than that. + let item_count = data.len() / item_types.len(); + + if item_count > 10 + && is_repeating + && item_types.iter().all(|typ| matches!(typ, Type::Numeric(_))) + { + dbg!("initializing runtime"); + self.initialize_constant_array_runtime( + item_types, first_item, item_count, pointer + ); + } else { + dbg!("initializing comptime"); + self.initialize_constant_array_comptime(data, pointer); + } + } + + fn initialize_constant_array_runtime( + &mut self, + item_types: Arc>, + item_to_repeat: Vec, + item_count: usize, + pointer: MemoryAddress, + ) { + let mut subitem_to_repeat_variables = Vec::with_capacity(item_types.len()); + for subitem_id in item_to_repeat.into_iter() { + subitem_to_repeat_variables.push(*self.brillig_globals.get(&subitem_id).unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available"))); + } + + // Initialize loop bound with the array length + let end_pointer_variable = self.brillig_context + .make_usize_constant_instruction((item_count * item_types.len()).into()); + + // Add the pointer to the array length + self.brillig_context.memory_op_instruction( + end_pointer_variable.address, + pointer, + end_pointer_variable.address, + BrilligBinaryOp::Add, + ); + + // If this is an array with complex subitems, we need a custom step in the loop to write all the subitems while iterating. + if item_types.len() > 1 { + let step_variable = + self.brillig_context.make_usize_constant_instruction(item_types.len().into()); + + let subitem_pointer = + SingleAddrVariable::new_usize(self.brillig_context.allocate_register()); + + // Initializes a single subitem + let initializer_fn = + |ctx: &mut BrilligContext<_, _>, subitem_start_pointer: SingleAddrVariable| { + ctx.mov_instruction(subitem_pointer.address, subitem_start_pointer.address); + for (subitem_index, subitem) in + subitem_to_repeat_variables.into_iter().enumerate() + { + ctx.store_instruction(subitem_pointer.address, subitem.extract_register()); + if subitem_index != item_types.len() - 1 { + ctx.memory_op_instruction( + subitem_pointer.address, + ReservedRegisters::usize_one(), + subitem_pointer.address, + BrilligBinaryOp::Add, + ); + } + } + }; + + // for (let subitem_start_pointer = pointer; subitem_start_pointer < pointer + data_length; subitem_start_pointer += step) { initializer_fn(iterator) } + self.brillig_context.codegen_for_loop( + Some(pointer), + end_pointer_variable.address, + Some(step_variable.address), + initializer_fn, + ); + + self.brillig_context.deallocate_single_addr(step_variable); + self.brillig_context.deallocate_single_addr(subitem_pointer); + } else { + let subitem = subitem_to_repeat_variables.into_iter().next().unwrap(); + + let initializer_fn = + |ctx: &mut BrilligContext<_, _>, item_pointer: SingleAddrVariable| { + ctx.store_instruction(item_pointer.address, subitem.extract_register()); + }; + + // for (let item_pointer = pointer; item_pointer < pointer + data_length; item_pointer += 1) { initializer_fn(iterator) } + self.brillig_context.codegen_for_loop( + Some(pointer), + end_pointer_variable.address, + None, + initializer_fn, + ); + } + self.brillig_context.deallocate_single_addr(end_pointer_variable); + } + + fn initialize_constant_array_comptime( + &mut self, + data: &im::Vector>, + pointer: MemoryAddress + ) { + // Allocate a register for the iterator + let write_pointer_register = self.brillig_context.allocate_register(); + + self.brillig_context.mov_instruction(write_pointer_register, pointer); + + for (element_idx, element_id) in data.iter().enumerate() { + let element_variable = *self.brillig_globals.get(&element_id).unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); + // Store the item in memory + self.brillig_context + .store_instruction(write_pointer_register, element_variable.extract_register()); + + if element_idx != data.len() - 1 { + // Increment the write_pointer_register + self.brillig_context.memory_op_instruction( + write_pointer_register, + ReservedRegisters::usize_one(), + write_pointer_register, + BrilligBinaryOp::Add, + ); + } + } + + self.brillig_context.deallocate_register(write_pointer_register); + } + + +} \ No newline at end of file From 74a73582b85e15ed070455da56b53bb23ec191fd Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 9 Jan 2025 21:14:18 +0000 Subject: [PATCH 19/25] 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 91c0c35576b5d40693f21eb07956f54cea49f51e Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 03:40:56 +0000 Subject: [PATCH 20/25] some fixes and fmt after merge --- .../src/brillig/brillig_gen.rs | 2 +- .../src/brillig/brillig_gen/brillig_block.rs | 7 +- .../src/brillig/brillig_gen/brillig_fn.rs | 7 +- .../brillig/brillig_gen/brillig_globals.rs | 57 ++++++++----- .../brillig/brillig_gen/brillig_slice_ops.rs | 12 +-- .../noirc_evaluator/src/brillig/brillig_ir.rs | 4 +- .../src/brillig/brillig_ir/artifact.rs | 2 +- .../src/brillig/brillig_ir/entry_point.rs | 2 +- .../src/brillig/brillig_ir/instructions.rs | 3 +- .../src/brillig/brillig_ir/registers.rs | 4 +- compiler/noirc_evaluator/src/brillig/mod.rs | 80 +++++++++++++++---- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 14 +++- tooling/nargo/src/ops/execute.rs | 3 +- 13 files changed, 137 insertions(+), 60 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index d40863b8e80..b2be4b66ab8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -2,8 +2,8 @@ pub(crate) mod brillig_black_box; pub(crate) mod brillig_block; pub(crate) mod brillig_block_variables; pub(crate) mod brillig_fn; -pub(crate) mod brillig_slice_ops; pub(crate) mod brillig_globals; +pub(crate) mod brillig_slice_ops; mod constant_allocation; mod variable_liveness; 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 4601a033ca8..010e91467f8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -870,7 +870,7 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { } } } - + self.brillig_context.set_call_stack(CallStack::new()); } @@ -1578,7 +1578,10 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { match value { Value::Global(_) => { dbg!(value_id); - let variable = *self.function_context.globals.get(&value_id).unwrap_or_else(|| panic!("ICE: Global value not found in cache {value_id}")); + let variable = + *self.function_context.globals.get(&value_id).unwrap_or_else(|| { + panic!("ICE: Global value not found in cache {value_id}") + }); dbg!(variable.clone()); variable } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index b9b9536bc57..6086eeb8802 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -32,7 +32,10 @@ pub(crate) struct FunctionContext<'global> { impl<'global> FunctionContext<'global> { /// Creates a new function context. It will allocate parameters for all blocks and compute the liveness of every variable. - pub(crate) fn new(function: &Function, globals: &'global HashMap) -> Self { + pub(crate) fn new( + function: &Function, + globals: &'global HashMap, + ) -> Self { let id = function.id(); let mut reverse_post_order = Vec::new(); @@ -48,7 +51,7 @@ impl<'global> FunctionContext<'global> { blocks: reverse_post_order, liveness, constant_allocation: constants, - globals + globals, } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index de608f5d695..340654449c2 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -3,8 +3,12 @@ use std::sync::Arc; use acvm::{acir::brillig::MemoryAddress, FieldElement}; use fxhash::FxHashMap as HashMap; -use crate::brillig::{allocate_value_with_type, brillig_gen::BrilligBlock, brillig_ir::BrilligContext, BrilligBinaryOp, DataFlowGraph, GlobalSpace, Instruction, ReservedRegisters, SingleAddrVariable, Type, Value}; use super::{BrilligVariable, ValueId}; +use crate::brillig::{ + allocate_value_with_type, brillig_gen::BrilligBlock, brillig_ir::BrilligContext, + BrilligBinaryOp, DataFlowGraph, GlobalSpace, Instruction, ReservedRegisters, + SingleAddrVariable, Type, Value, +}; pub(crate) struct BrilligGlobals<'global> { pub(crate) brillig_context: &'global mut BrilligContext, @@ -13,7 +17,6 @@ pub(crate) struct BrilligGlobals<'global> { } impl<'global> BrilligGlobals<'global> { - pub(crate) fn get_globals(self) -> HashMap { self.brillig_globals } @@ -21,28 +24,32 @@ impl<'global> BrilligGlobals<'global> { pub(crate) fn create_brillig_globals(&mut self, globals: &DataFlowGraph) { for (id, value) in globals.values_iter() { match value { - Value::NumericConstant { constant, typ } => { - let new_variable = allocate_value_with_type(self.brillig_context, Type::Numeric(*typ)); - self.brillig_context.const_instruction(new_variable.extract_single_addr(), *constant); + Value::NumericConstant { constant, typ } => { + let new_variable = + allocate_value_with_type(self.brillig_context, Type::Numeric(*typ)); + self.brillig_context + .const_instruction(new_variable.extract_single_addr(), *constant); self.brillig_globals.insert(id, new_variable); } Value::Instruction { instruction, .. } => { - let result = globals.instruction_results(*instruction)[0]; let instruction = &globals[*instruction]; match &instruction { Instruction::MakeArray { elements: array, typ } => { - let new_variable = allocate_value_with_type(self.brillig_context, typ.clone()); + let new_variable = + allocate_value_with_type(self.brillig_context, typ.clone()); // Initialize the variable match new_variable { BrilligVariable::BrilligArray(brillig_array) => { self.brillig_context.codegen_initialize_array(brillig_array); } BrilligVariable::BrilligVector(vector) => { - let size = self.brillig_context + let size = self + .brillig_context .make_usize_constant_instruction(array.len().into()); - self.brillig_context.codegen_initialize_vector(vector, size, None); + self.brillig_context + .codegen_initialize_vector(vector, size, None); self.brillig_context.deallocate_single_addr(size); } _ => unreachable!( @@ -51,7 +58,8 @@ impl<'global> BrilligGlobals<'global> { }; // Write the items - let items_pointer = self.brillig_context + let items_pointer = self + .brillig_context .codegen_make_array_or_vector_items_pointer(new_variable); self.initialize_constant_array(array, typ, items_pointer); @@ -60,7 +68,9 @@ impl<'global> BrilligGlobals<'global> { self.brillig_globals.insert(result, new_variable); } - _ => unreachable!("Expected MakeArray instruction but got {instruction:#?}") + _ => { + unreachable!("Expected MakeArray instruction but got {instruction:#?}") + } } } _ => { @@ -104,9 +114,7 @@ impl<'global> BrilligGlobals<'global> { && item_types.iter().all(|typ| matches!(typ, Type::Numeric(_))) { dbg!("initializing runtime"); - self.initialize_constant_array_runtime( - item_types, first_item, item_count, pointer - ); + self.initialize_constant_array_runtime(item_types, first_item, item_count, pointer); } else { dbg!("initializing comptime"); self.initialize_constant_array_comptime(data, pointer); @@ -122,11 +130,17 @@ impl<'global> BrilligGlobals<'global> { ) { let mut subitem_to_repeat_variables = Vec::with_capacity(item_types.len()); for subitem_id in item_to_repeat.into_iter() { - subitem_to_repeat_variables.push(*self.brillig_globals.get(&subitem_id).unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available"))); + subitem_to_repeat_variables.push( + *self + .brillig_globals + .get(&subitem_id) + .unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available")), + ); } // Initialize loop bound with the array length - let end_pointer_variable = self.brillig_context + let end_pointer_variable = self + .brillig_context .make_usize_constant_instruction((item_count * item_types.len()).into()); // Add the pointer to the array length @@ -196,7 +210,7 @@ impl<'global> BrilligGlobals<'global> { fn initialize_constant_array_comptime( &mut self, data: &im::Vector>, - pointer: MemoryAddress + pointer: MemoryAddress, ) { // Allocate a register for the iterator let write_pointer_register = self.brillig_context.allocate_register(); @@ -204,7 +218,10 @@ impl<'global> BrilligGlobals<'global> { self.brillig_context.mov_instruction(write_pointer_register, pointer); for (element_idx, element_id) in data.iter().enumerate() { - let element_variable = *self.brillig_globals.get(&element_id).unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); + let element_variable = *self + .brillig_globals + .get(&element_id) + .unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); // Store the item in memory self.brillig_context .store_instruction(write_pointer_register, element_variable.extract_register()); @@ -222,6 +239,4 @@ impl<'global> BrilligGlobals<'global> { self.brillig_context.deallocate_register(write_pointer_register); } - - -} \ No newline at end of file +} diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index 8d9a531f674..b6f93d89647 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -180,7 +180,9 @@ mod tests { use crate::ssa::ir::map::Id; use crate::ssa::ssa_gen::Ssa; - fn create_test_environment(brillig_globals: &HashMap) -> (Ssa, FunctionContext, BrilligContext) { + fn create_test_environment( + brillig_globals: &HashMap, + ) -> (Ssa, FunctionContext, BrilligContext) { let mut builder = FunctionBuilder::new("main".to_string(), Id::test_new(0)); builder.set_runtime(RuntimeType::Brillig(InlineType::default())); @@ -232,7 +234,7 @@ mod tests { result_length_with_metadata, )]; - let brillig_globals = HashMap::default(); + let brillig_globals = HashMap::default(); let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters @@ -349,7 +351,7 @@ mod tests { ), ]; - let brillig_globals = HashMap::default(); + let brillig_globals = HashMap::default(); let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters @@ -452,7 +454,7 @@ mod tests { result_length_with_metadata, )]; - let brillig_globals = HashMap::default(); + let brillig_globals = HashMap::default(); let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters @@ -593,7 +595,7 @@ mod tests { ), ]; - let brillig_globals = HashMap::default(); + let brillig_globals = HashMap::default(); let (_, mut function_context, mut context) = create_test_environment(&brillig_globals); // Allocate the parameters diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 8c2ed0c2c34..7e768df055c 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -215,9 +215,7 @@ impl BrilligContext { /// Special brillig context to codegen global values initialization impl BrilligContext { - pub(crate) fn new_for_global_init( - enable_debug_trace: bool, - ) -> BrilligContext { + pub(crate) fn new_for_global_init(enable_debug_trace: bool) -> BrilligContext { BrilligContext { obj: BrilligArtifact::default(), registers: GlobalSpace::new(), diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs index dc71fc9b28b..4c48675d1e7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/artifact.rs @@ -126,7 +126,7 @@ impl Label { pub(crate) fn procedure(procedure_id: ProcedureId) -> Self { Label { label_type: LabelType::Procedure(procedure_id), section: None } } - + pub(crate) fn globals_init() -> Self { Label { label_type: LabelType::GlobalInit, section: None } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 29ca46964b7..8d2a1c8ae10 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -29,7 +29,7 @@ impl BrilligContext { context.codegen_entry_point(&arguments, &return_parameters); context.add_globals_init_instruction(); - + context.add_external_call_instruction(target_function); context.codegen_exit_point(&arguments, &return_parameters); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs index 8b2b492e880..d67da423d44 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/instructions.rs @@ -203,7 +203,8 @@ impl BrilligContext< pub(super) fn add_globals_init_instruction(&mut self) { let globals_init_label = Label::globals_init(); self.debug_show.add_external_call_instruction(globals_init_label.to_string()); - self.obj.add_unresolved_external_call(BrilligOpcode::Call { location: 0 }, globals_init_label); + self.obj + .add_unresolved_external_call(BrilligOpcode::Call { location: 0 }, globals_init_label); } /// Adds a unresolved `Jump` instruction to the bytecode. diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs index 52889590b5e..0937cbbb60a 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs @@ -142,10 +142,10 @@ impl RegisterAllocator for ScratchSpace { } /// Globals have a separate memory space -/// This memory space is initialized once at the beginning of a program +/// This memory space is initialized once at the beginning of a program /// is read-only. pub(crate) struct GlobalSpace { - storage: DeallocationListAllocator + storage: DeallocationListAllocator, } impl GlobalSpace { diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index b7f6a4eb714..75dbbe98cd7 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -3,7 +3,12 @@ pub(crate) mod brillig_ir; use acvm::{acir::brillig::MemoryAddress, FieldElement}; use brillig_gen::brillig_block_variables::allocate_value_with_type; -use brillig_ir::{artifact::LabelType, brillig_variable::{BrilligVariable, SingleAddrVariable}, registers::GlobalSpace, BrilligBinaryOp, BrilligContext, ReservedRegisters}; +use brillig_ir::{ + artifact::LabelType, + brillig_variable::{BrilligVariable, SingleAddrVariable}, + registers::GlobalSpace, + BrilligBinaryOp, BrilligContext, ReservedRegisters, +}; use self::{ brillig_gen::convert_ssa_function, @@ -13,7 +18,13 @@ use self::{ }, }; use crate::ssa::{ - ir::{dfg::DataFlowGraph, function::{Function, FunctionId}, instruction::Instruction, types::Type, value::{Value, ValueId}}, + ir::{ + dfg::DataFlowGraph, + function::{Function, FunctionId}, + instruction::Instruction, + types::Type, + value::{Value, ValueId}, + }, ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; @@ -32,7 +43,12 @@ pub struct Brillig { impl Brillig { /// Compiles a function into brillig and store the compilation artifacts - pub(crate) fn compile(&mut self, func: &Function, enable_debug_trace: bool, globals: &HashMap) { + pub(crate) fn compile( + &mut self, + func: &Function, + enable_debug_trace: bool, + globals: &HashMap, + ) { let obj = convert_ssa_function(func, enable_debug_trace, globals); self.ssa_function_to_brillig.insert(func.id(), obj); } @@ -53,14 +69,19 @@ impl Brillig { } } - pub(crate) fn create_brillig_globals(brillig_context: &mut BrilligContext, globals: &DataFlowGraph) -> HashMap { + pub(crate) fn create_brillig_globals( + brillig_context: &mut BrilligContext, + globals: &DataFlowGraph, + ) -> HashMap { let mut brillig_globals = HashMap::default(); for (id, value) in globals.values_iter() { match value { - Value::NumericConstant { constant, typ } => { - let new_variable = allocate_value_with_type(brillig_context, Type::Numeric(*typ)); + Value::NumericConstant { constant, typ } => { + let new_variable = + allocate_value_with_type(brillig_context, Type::Numeric(*typ)); dbg!(new_variable.clone()); - brillig_context.const_instruction(new_variable.extract_single_addr(), *constant); + brillig_context + .const_instruction(new_variable.extract_single_addr(), *constant); brillig_globals.insert(id, new_variable); } @@ -70,7 +91,8 @@ impl Brillig { let instruction = &globals[*instruction]; match &instruction { Instruction::MakeArray { elements: array, typ } => { - let new_variable = allocate_value_with_type(brillig_context, typ.clone()); + let new_variable = + allocate_value_with_type(brillig_context, typ.clone()); // Initialize the variable match new_variable { BrilligVariable::BrilligArray(brillig_array) => { @@ -91,14 +113,22 @@ impl Brillig { let items_pointer = brillig_context .codegen_make_array_or_vector_items_pointer(new_variable); - Self::initialize_constant_array(array, typ, items_pointer, brillig_context, &brillig_globals); + Self::initialize_constant_array( + array, + typ, + items_pointer, + brillig_context, + &brillig_globals, + ); brillig_context.deallocate_register(items_pointer); dbg!(new_variable.clone()); brillig_globals.insert(result, new_variable); } - _ => unreachable!("Expected MakeArray instruction but got {instruction:#?}") + _ => { + unreachable!("Expected MakeArray instruction but got {instruction:#?}") + } } } _ => { @@ -143,11 +173,21 @@ impl Brillig { { dbg!("initializing runtime"); Self::initialize_constant_array_runtime( - item_types, first_item, item_count, pointer, brillig_context, &brillig_globals + item_types, + first_item, + item_count, + pointer, + brillig_context, + &brillig_globals, ); } else { dbg!("initializing comptime"); - Self::initialize_constant_array_comptime(data, pointer, brillig_context, &brillig_globals); + Self::initialize_constant_array_comptime( + data, + pointer, + brillig_context, + &brillig_globals, + ); } } @@ -161,12 +201,16 @@ impl Brillig { ) { let mut subitem_to_repeat_variables = Vec::with_capacity(item_types.len()); for subitem_id in item_to_repeat.into_iter() { - subitem_to_repeat_variables.push(*brillig_globals.get(&subitem_id).unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available"))); + subitem_to_repeat_variables.push( + *brillig_globals + .get(&subitem_id) + .unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available")), + ); } // Initialize loop bound with the array length - let end_pointer_variable = brillig_context - .make_usize_constant_instruction((item_count * item_types.len()).into()); + let end_pointer_variable = + brillig_context.make_usize_constant_instruction((item_count * item_types.len()).into()); // Add the pointer to the array length brillig_context.memory_op_instruction( @@ -244,7 +288,9 @@ impl Brillig { brillig_context.mov_instruction(write_pointer_register, pointer); for (element_idx, element_id) in data.iter().enumerate() { - let element_variable = *brillig_globals.get(&element_id).unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); + let element_variable = *brillig_globals + .get(&element_id) + .unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); // Store the item in memory brillig_context .store_instruction(write_pointer_register, element_variable.extract_register()); @@ -287,7 +333,7 @@ impl Ssa { let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace); brillig_context.enter_context(Label::globals_init()); - let brillig_globals = Brillig::create_brillig_globals(&mut brillig_context, &self.globals.dfg); + let brillig_globals = Brillig::create_brillig_globals(&mut brillig_context, &self.globals); brillig_context.return_instruction(); let artifact = brillig_context.artifact(); diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index d987f3f9c4b..01fabd0ff5d 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -24,7 +24,11 @@ use acvm::{acir::AcirField, FieldElement}; use im::HashSet; use crate::{ - brillig::{brillig_gen::convert_ssa_function, brillig_ir::{brillig_variable::BrilligVariable, BrilligContext}, Brillig}, + brillig::{ + brillig_gen::convert_ssa_function, + brillig_ir::{brillig_variable::BrilligVariable, BrilligContext}, + Brillig, + }, errors::RuntimeError, ssa::{ ir::{ @@ -88,7 +92,8 @@ impl Ssa { if has_unrolled { if let Some((orig_function, max_incr_pct)) = orig_func_and_max_incr_pct { let mut brillig_context = BrilligContext::new_for_global_init(true); - let brillig_globals = Brillig::create_brillig_globals(&mut brillig_context, &self.globals.dfg); + let brillig_globals = + Brillig::create_brillig_globals(&mut brillig_context, &self.globals); let new_size = brillig_bytecode_size(function, &brillig_globals); let orig_size = brillig_bytecode_size(&orig_function, &brillig_globals); @@ -977,7 +982,10 @@ fn simplify_between_unrolls(function: &mut Function) { } /// Convert the function to Brillig bytecode and return the resulting size. -fn brillig_bytecode_size(function: &Function, globals: &HashMap) -> usize { +fn brillig_bytecode_size( + function: &Function, + globals: &HashMap, +) -> usize { // We need to do some SSA passes in order for the conversion to be able to go ahead, // otherwise we can hit `unreachable!()` instructions in `convert_ssa_instruction`. // Creating a clone so as not to modify the originals. diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 76f44092018..074ae93e129 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -143,7 +143,8 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> dbg!(brillig_function_id); ExecutionError::AssertionFailed( payload, - call_stack.expect("Should have call stack for an assertion failure"), + call_stack + .expect("Should have call stack for an assertion failure"), brillig_function_id, ) } From 94683e6a81fd85d11d1d9380873747e851da8340 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 05:42:17 +0000 Subject: [PATCH 21/25] re-use codegen methods for BrilligBlock --- .../src/brillig/brillig_gen/brillig_block.rs | 64 +++-- .../brillig_gen/brillig_block_variables.rs | 12 +- .../src/brillig/brillig_gen/brillig_fn.rs | 18 ++ .../brillig/brillig_gen/brillig_globals.rs | 235 +--------------- .../brillig/brillig_gen/brillig_slice_ops.rs | 5 +- .../brillig_gen/constant_allocation.rs | 1 + .../brillig/brillig_gen/variable_liveness.rs | 1 + .../noirc_evaluator/src/brillig/brillig_ir.rs | 4 +- .../src/brillig/brillig_ir/codegen_calls.rs | 5 +- .../src/brillig/brillig_ir/entry_point.rs | 2 +- .../src/brillig/brillig_ir/registers.rs | 32 ++- compiler/noirc_evaluator/src/brillig/mod.rs | 262 +++--------------- compiler/noirc_evaluator/src/ssa/ir/cfg.rs | 2 +- compiler/noirc_evaluator/src/ssa/ir/dom.rs | 1 + .../noirc_evaluator/src/ssa/ir/post_order.rs | 1 + 15 files changed, 141 insertions(+), 504 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 010e91467f8..4c88f487cab 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -32,23 +32,25 @@ use super::brillig_fn::FunctionContext; use super::constant_allocation::InstructionLocation; /// Generate the compilation artifacts for compiling a function into brillig bytecode. -pub(crate) struct BrilligBlock<'block, 'global> { +pub(crate) struct BrilligBlock<'block, 'global, Registers: RegisterAllocator> { pub(crate) function_context: &'block mut FunctionContext<'global>, /// The basic block that is being converted pub(crate) block_id: BasicBlockId, /// Context for creating brillig opcodes - pub(crate) brillig_context: &'block mut BrilligContext, + pub(crate) brillig_context: &'block mut BrilligContext, /// Tracks the available variable during the codegen of the block pub(crate) variables: BlockVariables, /// For each instruction, the set of values that are not used anymore after it. pub(crate) last_uses: HashMap>, + + pub(crate) building_globals: bool, } -impl<'block, 'global> BrilligBlock<'block, 'global> { +impl<'block, 'global, Registers: RegisterAllocator> BrilligBlock<'block, 'global, Registers> { /// Converts an SSA Basic block into a sequence of Brillig opcodes pub(crate) fn compile( function_context: &'block mut FunctionContext<'global>, - brillig_context: &'block mut BrilligContext, + brillig_context: &'block mut BrilligContext, block_id: BasicBlockId, dfg: &DataFlowGraph, ) { @@ -72,8 +74,14 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { ); let last_uses = function_context.liveness.get_last_uses(&block_id).clone(); - let mut brillig_block = - BrilligBlock { function_context, block_id, brillig_context, variables, last_uses }; + let mut brillig_block = BrilligBlock { + function_context, + block_id, + brillig_context, + variables, + last_uses, + building_globals: false, + }; brillig_block.convert_block(dfg); } @@ -207,7 +215,11 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { } /// Converts an SSA instruction into a sequence of Brillig opcodes. - fn convert_ssa_instruction(&mut self, instruction_id: InstructionId, dfg: &DataFlowGraph) { + pub(crate) fn convert_ssa_instruction( + &mut self, + instruction_id: InstructionId, + dfg: &DataFlowGraph, + ) { let instruction = &dfg[instruction_id]; self.brillig_context.set_call_stack(dfg.get_instruction_call_stack(instruction_id)); @@ -851,22 +863,24 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { Instruction::Noop => (), }; - let dead_variables = self - .last_uses - .get(&instruction_id) - .expect("Last uses for instruction should have been computed"); + if !self.building_globals { + let dead_variables = self + .last_uses + .get(&instruction_id) + .expect("Last uses for instruction should have been computed"); - for dead_variable in dead_variables { - match &dfg[*dead_variable] { - Value::Global(_) => { - dbg!("got dead global"); - } - _ => { - self.variables.remove_variable( - dead_variable, - self.function_context, - self.brillig_context, - ); + for dead_variable in dead_variables { + match &dfg[*dead_variable] { + Value::Global(_) => { + dbg!("got dead global"); + } + _ => { + self.variables.remove_variable( + dead_variable, + self.function_context, + self.brillig_context, + ); + } } } } @@ -1571,7 +1585,11 @@ impl<'block, 'global> BrilligBlock<'block, 'global> { } /// Converts an SSA `ValueId` into a `RegisterOrMemory`. Initializes if necessary. - fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> BrilligVariable { + pub(crate) fn convert_ssa_value( + &mut self, + value_id: ValueId, + dfg: &DataFlowGraph, + ) -> BrilligVariable { let value_id = dfg.resolve(value_id); let value = &dfg[value_id]; diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index 06e65471538..e19ccdead65 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -48,10 +48,10 @@ impl BlockVariables { } /// For a given SSA value id, define the variable and return the corresponding cached allocation. - pub(crate) fn define_variable( + pub(crate) fn define_variable( &mut self, function_context: &mut FunctionContext, - brillig_context: &mut BrilligContext, + brillig_context: &mut BrilligContext, value_id: ValueId, dfg: &DataFlowGraph, ) -> BrilligVariable { @@ -68,10 +68,10 @@ impl BlockVariables { } /// Defines a variable that fits in a single register and returns the allocated register. - pub(crate) fn define_single_addr_variable( + pub(crate) fn define_single_addr_variable( &mut self, function_context: &mut FunctionContext, - brillig_context: &mut BrilligContext, + brillig_context: &mut BrilligContext, value: ValueId, dfg: &DataFlowGraph, ) -> SingleAddrVariable { @@ -80,11 +80,11 @@ impl BlockVariables { } /// Removes a variable so it's not used anymore within this block. - pub(crate) fn remove_variable( + pub(crate) fn remove_variable( &mut self, value_id: &ValueId, function_context: &mut FunctionContext, - brillig_context: &mut BrilligContext, + brillig_context: &mut BrilligContext, ) { assert!(self.available_variables.remove(value_id), "ICE: Variable is not available"); let variable = function_context diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 6086eeb8802..22780d3e2ec 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -8,6 +8,7 @@ use crate::{ ssa::ir::{ basic_block::BasicBlockId, function::{Function, FunctionId}, + map::Id, post_order::PostOrder, types::Type, value::ValueId, @@ -55,6 +56,23 @@ impl<'global> FunctionContext<'global> { } } + pub(crate) fn new_for_global_init( + function_id: FunctionId, + globals: &'global HashMap, + ) -> Self { + // let mut liveness = VariableLiveness::default(); + // liveness.l + Self { + // It does not matter what ID we have here + function_id, + ssa_value_allocations: Default::default(), + blocks: Default::default(), + liveness: Default::default(), + constant_allocation: Default::default(), + globals, + } + } + pub(crate) fn ssa_type_to_parameter(typ: &Type) -> BrilligParameter { match typ { Type::Numeric(_) | Type::Reference(_) => { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 340654449c2..1388de4da8d 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -1,242 +1,11 @@ -use std::sync::Arc; - -use acvm::{acir::brillig::MemoryAddress, FieldElement}; +use acvm::FieldElement; use fxhash::FxHashMap as HashMap; use super::{BrilligVariable, ValueId}; -use crate::brillig::{ - allocate_value_with_type, brillig_gen::BrilligBlock, brillig_ir::BrilligContext, - BrilligBinaryOp, DataFlowGraph, GlobalSpace, Instruction, ReservedRegisters, - SingleAddrVariable, Type, Value, -}; +use crate::brillig::{brillig_ir::BrilligContext, GlobalSpace}; pub(crate) struct BrilligGlobals<'global> { pub(crate) brillig_context: &'global mut BrilligContext, brillig_globals: HashMap, } - -impl<'global> BrilligGlobals<'global> { - pub(crate) fn get_globals(self) -> HashMap { - self.brillig_globals - } - - pub(crate) fn create_brillig_globals(&mut self, globals: &DataFlowGraph) { - for (id, value) in globals.values_iter() { - match value { - Value::NumericConstant { constant, typ } => { - let new_variable = - allocate_value_with_type(self.brillig_context, Type::Numeric(*typ)); - self.brillig_context - .const_instruction(new_variable.extract_single_addr(), *constant); - - self.brillig_globals.insert(id, new_variable); - } - Value::Instruction { instruction, .. } => { - let result = globals.instruction_results(*instruction)[0]; - let instruction = &globals[*instruction]; - match &instruction { - Instruction::MakeArray { elements: array, typ } => { - let new_variable = - allocate_value_with_type(self.brillig_context, typ.clone()); - // Initialize the variable - match new_variable { - BrilligVariable::BrilligArray(brillig_array) => { - self.brillig_context.codegen_initialize_array(brillig_array); - } - BrilligVariable::BrilligVector(vector) => { - let size = self - .brillig_context - .make_usize_constant_instruction(array.len().into()); - self.brillig_context - .codegen_initialize_vector(vector, size, None); - self.brillig_context.deallocate_single_addr(size); - } - _ => unreachable!( - "ICE: Cannot initialize array value created as {new_variable:?}" - ), - }; - - // Write the items - let items_pointer = self - .brillig_context - .codegen_make_array_or_vector_items_pointer(new_variable); - - self.initialize_constant_array(array, typ, items_pointer); - - self.brillig_context.deallocate_register(items_pointer); - - self.brillig_globals.insert(result, new_variable); - } - _ => { - unreachable!("Expected MakeArray instruction but got {instruction:#?}") - } - } - } - _ => { - panic!("got something other than numeric constant") - } - } - } - } - - fn initialize_constant_array( - &mut self, - data: &im::Vector, - typ: &Type, - pointer: MemoryAddress, - // brillig_globals: &HashMap, - // globals: &DataFlowGraph, - ) { - if data.is_empty() { - return; - } - let item_types = typ.clone().element_types(); - - // Find out if we are repeating the same item over and over - let first_item = data.iter().take(item_types.len()).copied().collect(); - let mut is_repeating = true; - - for item_index in (item_types.len()..data.len()).step_by(item_types.len()) { - let item: Vec<_> = (0..item_types.len()).map(|i| data[item_index + i]).collect(); - if first_item != item { - is_repeating = false; - break; - } - } - - // If all the items are single address, and all have the same initial value, we can initialize the array in a runtime loop. - // Since the cost in instructions for a runtime loop is in the order of magnitude of 10, we only do this if the item_count is bigger than that. - let item_count = data.len() / item_types.len(); - - if item_count > 10 - && is_repeating - && item_types.iter().all(|typ| matches!(typ, Type::Numeric(_))) - { - dbg!("initializing runtime"); - self.initialize_constant_array_runtime(item_types, first_item, item_count, pointer); - } else { - dbg!("initializing comptime"); - self.initialize_constant_array_comptime(data, pointer); - } - } - - fn initialize_constant_array_runtime( - &mut self, - item_types: Arc>, - item_to_repeat: Vec, - item_count: usize, - pointer: MemoryAddress, - ) { - let mut subitem_to_repeat_variables = Vec::with_capacity(item_types.len()); - for subitem_id in item_to_repeat.into_iter() { - subitem_to_repeat_variables.push( - *self - .brillig_globals - .get(&subitem_id) - .unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available")), - ); - } - - // Initialize loop bound with the array length - let end_pointer_variable = self - .brillig_context - .make_usize_constant_instruction((item_count * item_types.len()).into()); - - // Add the pointer to the array length - self.brillig_context.memory_op_instruction( - end_pointer_variable.address, - pointer, - end_pointer_variable.address, - BrilligBinaryOp::Add, - ); - - // If this is an array with complex subitems, we need a custom step in the loop to write all the subitems while iterating. - if item_types.len() > 1 { - let step_variable = - self.brillig_context.make_usize_constant_instruction(item_types.len().into()); - - let subitem_pointer = - SingleAddrVariable::new_usize(self.brillig_context.allocate_register()); - - // Initializes a single subitem - let initializer_fn = - |ctx: &mut BrilligContext<_, _>, subitem_start_pointer: SingleAddrVariable| { - ctx.mov_instruction(subitem_pointer.address, subitem_start_pointer.address); - for (subitem_index, subitem) in - subitem_to_repeat_variables.into_iter().enumerate() - { - ctx.store_instruction(subitem_pointer.address, subitem.extract_register()); - if subitem_index != item_types.len() - 1 { - ctx.memory_op_instruction( - subitem_pointer.address, - ReservedRegisters::usize_one(), - subitem_pointer.address, - BrilligBinaryOp::Add, - ); - } - } - }; - - // for (let subitem_start_pointer = pointer; subitem_start_pointer < pointer + data_length; subitem_start_pointer += step) { initializer_fn(iterator) } - self.brillig_context.codegen_for_loop( - Some(pointer), - end_pointer_variable.address, - Some(step_variable.address), - initializer_fn, - ); - - self.brillig_context.deallocate_single_addr(step_variable); - self.brillig_context.deallocate_single_addr(subitem_pointer); - } else { - let subitem = subitem_to_repeat_variables.into_iter().next().unwrap(); - - let initializer_fn = - |ctx: &mut BrilligContext<_, _>, item_pointer: SingleAddrVariable| { - ctx.store_instruction(item_pointer.address, subitem.extract_register()); - }; - - // for (let item_pointer = pointer; item_pointer < pointer + data_length; item_pointer += 1) { initializer_fn(iterator) } - self.brillig_context.codegen_for_loop( - Some(pointer), - end_pointer_variable.address, - None, - initializer_fn, - ); - } - self.brillig_context.deallocate_single_addr(end_pointer_variable); - } - - fn initialize_constant_array_comptime( - &mut self, - data: &im::Vector>, - pointer: MemoryAddress, - ) { - // Allocate a register for the iterator - let write_pointer_register = self.brillig_context.allocate_register(); - - self.brillig_context.mov_instruction(write_pointer_register, pointer); - - for (element_idx, element_id) in data.iter().enumerate() { - let element_variable = *self - .brillig_globals - .get(&element_id) - .unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); - // Store the item in memory - self.brillig_context - .store_instruction(write_pointer_register, element_variable.extract_register()); - - if element_idx != data.len() - 1 { - // Increment the write_pointer_register - self.brillig_context.memory_op_instruction( - write_pointer_register, - ReservedRegisters::usize_one(), - write_pointer_register, - BrilligBinaryOp::Add, - ); - } - } - - self.brillig_context.deallocate_register(write_pointer_register); - } -} diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index b6f93d89647..c77477676a4 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -2,12 +2,13 @@ use acvm::acir::brillig::MemoryAddress; use crate::brillig::brillig_ir::{ brillig_variable::{BrilligVariable, BrilligVector, SingleAddrVariable}, + registers::RegisterAllocator, BrilligBinaryOp, }; use super::brillig_block::BrilligBlock; -impl<'block, 'global> BrilligBlock<'block, 'global> { +impl<'block, 'global, Registers: RegisterAllocator> BrilligBlock<'block, 'global, Registers> { fn write_variables(&mut self, write_pointer: MemoryAddress, variables: &[BrilligVariable]) { for (index, variable) in variables.iter().enumerate() { self.brillig_context.store_instruction(write_pointer, variable.extract_register()); @@ -197,7 +198,7 @@ mod tests { fn create_brillig_block<'a, 'global>( function_context: &'a mut FunctionContext<'global>, brillig_context: &'a mut BrilligContext, - ) -> BrilligBlock<'a, 'global> { + ) -> BrilligBlock<'a, 'global, Stack> { let variables = BlockVariables::default(); BrilligBlock { function_context, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index 61ca20be2f5..64741393dd7 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -22,6 +22,7 @@ pub(crate) enum InstructionLocation { Terminator, } +#[derive(Default)] pub(crate) struct ConstantAllocation { constant_usage: HashMap>>, allocation_points: HashMap>>, 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..8aa2bec8a66 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -114,6 +114,7 @@ fn compute_used_before_def( type LastUses = HashMap; /// A struct representing the liveness of variables throughout a function. +#[derive(Default)] pub(crate) struct VariableLiveness { cfg: ControlFlowGraph, post_order: PostOrder, diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 7e768df055c..4f3e22505c3 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -82,7 +82,7 @@ impl ReservedRegisters { pub(crate) struct BrilligContext { obj: BrilligArtifact, /// Tracks register allocations - registers: Registers, + pub(crate) registers: Registers, /// Context label, must be unique with respect to the function /// being linked. context_label: Label, @@ -110,7 +110,9 @@ impl BrilligContext { can_call_procedures: true, } } +} +impl BrilligContext { /// Splits a two's complement signed integer in the sign bit and the absolute value. /// For example, -6 i8 (11111010) is split to 00000110 (6, absolute value) and 1 (is_negative). pub(crate) fn absolute_value( diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs index da310873cff..4da3aa4d6d2 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_calls.rs @@ -9,7 +9,8 @@ use super::{ BrilligBinaryOp, BrilligContext, ReservedRegisters, }; -impl BrilligContext { +impl BrilligContext { + // impl BrilligContext { pub(crate) fn codegen_call( &mut self, func_id: FunctionId, @@ -17,7 +18,7 @@ impl BrilligContext { returns: &[BrilligVariable], ) { let stack_size_register = SingleAddrVariable::new_usize(self.allocate_register()); - let previous_stack_pointer = self.registers.empty_stack_start(); + let previous_stack_pointer = self.registers.empty_registers_start(); let stack_size = previous_stack_pointer.unwrap_relative(); // Write the stack size self.const_instruction(stack_size_register, stack_size.into()); diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 8d2a1c8ae10..ce112873fe8 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -334,7 +334,7 @@ impl BrilligContext { // Return data has a reserved space after calldata let return_data_offset = Self::return_data_start_offset(calldata_size); - dbg!(return_data_offset); + // dbg!(return_data_offset); let mut return_data_index = return_data_offset; for (return_param, returned_variable) in return_parameters.iter().zip(&returned_variables) { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs index 0937cbbb60a..315631edab0 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/registers.rs @@ -11,7 +11,9 @@ use super::{ BrilligContext, ReservedRegisters, }; -pub(crate) trait RegisterAllocator { +use std::any::Any; + +pub(crate) trait RegisterAllocator: Any { /// First valid memory address fn start() -> usize; /// Last valid memory address @@ -24,6 +26,8 @@ pub(crate) trait RegisterAllocator { fn ensure_register_is_allocated(&mut self, register: MemoryAddress); /// Creates a new register context from a set of registers allocated previously. fn from_preallocated_registers(preallocated_registers: Vec) -> Self; + /// Finds the first register that is available based upon the deallocation list + fn empty_registers_start(&self) -> MemoryAddress; } /// Every brillig stack frame/call context has its own view of register space. @@ -32,6 +36,16 @@ pub(crate) struct Stack { storage: DeallocationListAllocator, } +// pub(crate) trait StackMarker { +// fn empty_stack_start(&self) -> MemoryAddress; +// } + +// impl StackMarker for Stack { +// fn empty_stack_start(&self) -> MemoryAddress { +// MemoryAddress::relative(self.storage.empty_registers_start(Self::start())) +// } +// } + impl Stack { pub(crate) fn new() -> Self { Self { storage: DeallocationListAllocator::new(Self::start()) } @@ -41,10 +55,6 @@ impl Stack { let offset = register.unwrap_relative(); offset >= Self::start() && offset < Self::end() } - - pub(crate) fn empty_stack_start(&self) -> MemoryAddress { - MemoryAddress::relative(self.storage.empty_registers_start(Self::start())) - } } impl RegisterAllocator for Stack { @@ -83,6 +93,10 @@ impl RegisterAllocator for Stack { ), } } + + fn empty_registers_start(&self) -> MemoryAddress { + MemoryAddress::relative(self.storage.empty_registers_start(Self::start())) + } } /// Procedure arguments and returns are passed through scratch space. @@ -139,6 +153,10 @@ impl RegisterAllocator for ScratchSpace { ), } } + + fn empty_registers_start(&self) -> MemoryAddress { + MemoryAddress::direct(self.storage.empty_registers_start(Self::start())) + } } /// Globals have a separate memory space @@ -195,6 +213,10 @@ impl RegisterAllocator for GlobalSpace { ), } } + + fn empty_registers_start(&self) -> MemoryAddress { + MemoryAddress::direct(self.storage.empty_registers_start(Self::start())) + } } struct DeallocationListAllocator { diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 75dbbe98cd7..0124b2633f2 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -1,8 +1,12 @@ pub(crate) mod brillig_gen; pub(crate) mod brillig_ir; -use acvm::{acir::brillig::MemoryAddress, FieldElement}; -use brillig_gen::brillig_block_variables::allocate_value_with_type; +use acvm::FieldElement; +use brillig_gen::{ + brillig_block::BrilligBlock, + brillig_block_variables::{allocate_value_with_type, BlockVariables}, + brillig_fn::FunctionContext, +}; use brillig_ir::{ artifact::LabelType, brillig_variable::{BrilligVariable, SingleAddrVariable}, @@ -19,6 +23,7 @@ use self::{ }; use crate::ssa::{ ir::{ + basic_block::BasicBlockId, dfg::DataFlowGraph, function::{Function, FunctionId}, instruction::Instruction, @@ -70,243 +75,24 @@ impl Brillig { } pub(crate) fn create_brillig_globals( - brillig_context: &mut BrilligContext, + brillig_context: &mut BrilligBlock<'_, '_, GlobalSpace>, globals: &DataFlowGraph, - ) -> HashMap { - let mut brillig_globals = HashMap::default(); + ) { for (id, value) in globals.values_iter() { match value { - Value::NumericConstant { constant, typ } => { - let new_variable = - allocate_value_with_type(brillig_context, Type::Numeric(*typ)); - dbg!(new_variable.clone()); - brillig_context - .const_instruction(new_variable.extract_single_addr(), *constant); - - brillig_globals.insert(id, new_variable); + Value::NumericConstant { .. } => { + brillig_context.convert_ssa_value(id, globals); } Value::Instruction { instruction, .. } => { - let result = globals.instruction_results(*instruction)[0]; - dbg!(result); - let instruction = &globals[*instruction]; - match &instruction { - Instruction::MakeArray { elements: array, typ } => { - let new_variable = - allocate_value_with_type(brillig_context, typ.clone()); - // Initialize the variable - match new_variable { - BrilligVariable::BrilligArray(brillig_array) => { - brillig_context.codegen_initialize_array(brillig_array); - } - BrilligVariable::BrilligVector(vector) => { - let size = brillig_context - .make_usize_constant_instruction(array.len().into()); - brillig_context.codegen_initialize_vector(vector, size, None); - brillig_context.deallocate_single_addr(size); - } - _ => unreachable!( - "ICE: Cannot initialize array value created as {new_variable:?}" - ), - }; - - // Write the items - let items_pointer = brillig_context - .codegen_make_array_or_vector_items_pointer(new_variable); - - Self::initialize_constant_array( - array, - typ, - items_pointer, - brillig_context, - &brillig_globals, - ); - - brillig_context.deallocate_register(items_pointer); - - dbg!(new_variable.clone()); - brillig_globals.insert(result, new_variable); - } - _ => { - unreachable!("Expected MakeArray instruction but got {instruction:#?}") - } - } + brillig_context.convert_ssa_instruction(*instruction, globals); } _ => { - panic!("got something other than numeric constant") + panic!( + "Expected either an instruction or a numeric constant for a global value" + ) } } } - brillig_globals - } - - fn initialize_constant_array( - data: &im::Vector, - typ: &Type, - pointer: MemoryAddress, - brillig_context: &mut BrilligContext, - brillig_globals: &HashMap, - ) { - if data.is_empty() { - return; - } - let item_types = typ.clone().element_types(); - - // Find out if we are repeating the same item over and over - let first_item = data.iter().take(item_types.len()).copied().collect(); - let mut is_repeating = true; - - for item_index in (item_types.len()..data.len()).step_by(item_types.len()) { - let item: Vec<_> = (0..item_types.len()).map(|i| data[item_index + i]).collect(); - if first_item != item { - is_repeating = false; - break; - } - } - - // If all the items are single address, and all have the same initial value, we can initialize the array in a runtime loop. - // Since the cost in instructions for a runtime loop is in the order of magnitude of 10, we only do this if the item_count is bigger than that. - let item_count = data.len() / item_types.len(); - - if item_count > 10 - && is_repeating - && item_types.iter().all(|typ| matches!(typ, Type::Numeric(_))) - { - dbg!("initializing runtime"); - Self::initialize_constant_array_runtime( - item_types, - first_item, - item_count, - pointer, - brillig_context, - &brillig_globals, - ); - } else { - dbg!("initializing comptime"); - Self::initialize_constant_array_comptime( - data, - pointer, - brillig_context, - &brillig_globals, - ); - } - } - - fn initialize_constant_array_runtime( - item_types: Arc>, - item_to_repeat: Vec, - item_count: usize, - pointer: MemoryAddress, - brillig_context: &mut BrilligContext, - brillig_globals: &HashMap, - ) { - let mut subitem_to_repeat_variables = Vec::with_capacity(item_types.len()); - for subitem_id in item_to_repeat.into_iter() { - subitem_to_repeat_variables.push( - *brillig_globals - .get(&subitem_id) - .unwrap_or_else(|| panic!("ICE: ValueId {subitem_id} is not available")), - ); - } - - // Initialize loop bound with the array length - let end_pointer_variable = - brillig_context.make_usize_constant_instruction((item_count * item_types.len()).into()); - - // Add the pointer to the array length - brillig_context.memory_op_instruction( - end_pointer_variable.address, - pointer, - end_pointer_variable.address, - BrilligBinaryOp::Add, - ); - - // If this is an array with complex subitems, we need a custom step in the loop to write all the subitems while iterating. - if item_types.len() > 1 { - let step_variable = - brillig_context.make_usize_constant_instruction(item_types.len().into()); - - let subitem_pointer = - SingleAddrVariable::new_usize(brillig_context.allocate_register()); - - // Initializes a single subitem - let initializer_fn = - |ctx: &mut BrilligContext<_, _>, subitem_start_pointer: SingleAddrVariable| { - ctx.mov_instruction(subitem_pointer.address, subitem_start_pointer.address); - for (subitem_index, subitem) in - subitem_to_repeat_variables.into_iter().enumerate() - { - ctx.store_instruction(subitem_pointer.address, subitem.extract_register()); - if subitem_index != item_types.len() - 1 { - ctx.memory_op_instruction( - subitem_pointer.address, - ReservedRegisters::usize_one(), - subitem_pointer.address, - BrilligBinaryOp::Add, - ); - } - } - }; - - // for (let subitem_start_pointer = pointer; subitem_start_pointer < pointer + data_length; subitem_start_pointer += step) { initializer_fn(iterator) } - brillig_context.codegen_for_loop( - Some(pointer), - end_pointer_variable.address, - Some(step_variable.address), - initializer_fn, - ); - - brillig_context.deallocate_single_addr(step_variable); - brillig_context.deallocate_single_addr(subitem_pointer); - } else { - let subitem = subitem_to_repeat_variables.into_iter().next().unwrap(); - - let initializer_fn = - |ctx: &mut BrilligContext<_, _>, item_pointer: SingleAddrVariable| { - ctx.store_instruction(item_pointer.address, subitem.extract_register()); - }; - - // for (let item_pointer = pointer; item_pointer < pointer + data_length; item_pointer += 1) { initializer_fn(iterator) } - brillig_context.codegen_for_loop( - Some(pointer), - end_pointer_variable.address, - None, - initializer_fn, - ); - } - brillig_context.deallocate_single_addr(end_pointer_variable); - } - - fn initialize_constant_array_comptime( - data: &im::Vector>, - pointer: MemoryAddress, - brillig_context: &mut BrilligContext, - brillig_globals: &HashMap, - ) { - // Allocate a register for the iterator - let write_pointer_register = brillig_context.allocate_register(); - - brillig_context.mov_instruction(write_pointer_register, pointer); - - for (element_idx, element_id) in data.iter().enumerate() { - let element_variable = *brillig_globals - .get(&element_id) - .unwrap_or_else(|| panic!("ICE: ValueId {element_id} is not available")); - // Store the item in memory - brillig_context - .store_instruction(write_pointer_register, element_variable.extract_register()); - - if element_idx != data.len() - 1 { - // Increment the write_pointer_register - brillig_context.memory_op_instruction( - write_pointer_register, - ReservedRegisters::usize_one(), - write_pointer_register, - BrilligBinaryOp::Add, - ); - } - } - - brillig_context.deallocate_register(write_pointer_register); } } @@ -332,13 +118,29 @@ impl Ssa { let mut brillig = Brillig::default(); let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace); + // We can use any ID here as this context is only going to be used for globals which does not differentiate + // by functions and blocks. The only Label that should be used in the globals context is `Label::globals_init()` + let globals = HashMap::default(); + let mut function_context = FunctionContext::new_for_global_init(self.main_id, &globals); brillig_context.enter_context(Label::globals_init()); - let brillig_globals = Brillig::create_brillig_globals(&mut brillig_context, &self.globals); + + let block_id = DataFlowGraph::default().make_block(); + let mut brillig_block = BrilligBlock { + function_context: &mut function_context, + block_id, + brillig_context: &mut brillig_context, + variables: BlockVariables::default(), + last_uses: HashMap::default(), + building_globals: true, + }; + + Brillig::create_brillig_globals(&mut brillig_block, &self.globals); brillig_context.return_instruction(); let artifact = brillig_context.artifact(); brillig.globals = artifact; + let brillig_globals = function_context.ssa_value_allocations; for brillig_function_id in brillig_reachable_function_ids { let func = &self.functions[&brillig_function_id]; brillig.compile(func, enable_debug_trace, &brillig_globals); diff --git a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs index 788b1a7d302..5253c68c72c 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/cfg.rs @@ -18,7 +18,7 @@ struct CfgNode { pub(crate) successors: BTreeSet, } -#[derive(Clone)] +#[derive(Clone, Default)] /// The Control Flow Graph maintains a mapping of blocks to their predecessors /// and successors where predecessors are basic blocks and successors are /// basic blocks. diff --git a/compiler/noirc_evaluator/src/ssa/ir/dom.rs b/compiler/noirc_evaluator/src/ssa/ir/dom.rs index ff54bf3b6ed..3dde6240e18 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/dom.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/dom.rs @@ -39,6 +39,7 @@ impl DominatorTreeNode { } /// The dominator tree for a single function. +#[derive(Default)] pub(crate) struct DominatorTree { /// The nodes of the dominator tree /// diff --git a/compiler/noirc_evaluator/src/ssa/ir/post_order.rs b/compiler/noirc_evaluator/src/ssa/ir/post_order.rs index 398ce887b96..08f195e53d1 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/post_order.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/post_order.rs @@ -13,6 +13,7 @@ enum Visit { Last, } +#[derive(Default)] pub(crate) struct PostOrder(Vec); impl PostOrder { From f5c5f2562f2757851f2e5de7974b12306f693fc8 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 16:52:37 +0000 Subject: [PATCH 22/25] cleanup and put brillig globals code gen into separate method --- acvm-repo/brillig_vm/src/arithmetic.rs | 3 - .../src/brillig/brillig_gen.rs | 2 +- .../src/brillig/brillig_gen/brillig_block.rs | 39 +++++++---- .../brillig_gen/brillig_block_variables.rs | 2 +- .../src/brillig/brillig_gen/brillig_fn.rs | 5 +- .../brillig/brillig_gen/brillig_globals.rs | 36 ++++++++-- .../brillig/brillig_gen/brillig_slice_ops.rs | 1 + .../src/brillig/brillig_ir/entry_point.rs | 3 +- compiler/noirc_evaluator/src/brillig/mod.rs | 65 ++----------------- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 10 ++- .../global_var_regression_simple/src/main.nr | 6 -- tooling/nargo/src/errors.rs | 2 +- tooling/nargo/src/ops/execute.rs | 16 ++--- 13 files changed, 79 insertions(+), 111 deletions(-) diff --git a/acvm-repo/brillig_vm/src/arithmetic.rs b/acvm-repo/brillig_vm/src/arithmetic.rs index 720598717ca..7cd31cd6443 100644 --- a/acvm-repo/brillig_vm/src/arithmetic.rs +++ b/acvm-repo/brillig_vm/src/arithmetic.rs @@ -69,9 +69,6 @@ pub(crate) fn evaluate_binary_int_op( rhs: MemoryValue, bit_size: IntegerBitSize, ) -> Result, BrilligArithmeticError> { - // dbg!(op); - // dbg!(lhs); - // dbg!(rhs); let lhs = lhs.expect_integer_with_bit_size(bit_size).map_err(|err| match err { MemoryTypeError::MismatchedBitSize { value_bit_size, expected_bit_size } => { BrilligArithmeticError::MismatchedLhsBitSize { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs index b2be4b66ab8..319058e5677 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen.rs @@ -8,7 +8,7 @@ mod constant_allocation; mod variable_liveness; use acvm::FieldElement; -use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; +use fxhash::FxHashMap as HashMap; use self::{brillig_block::BrilligBlock, brillig_fn::FunctionContext}; use super::{ 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 4c88f487cab..7d669ea3de1 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -3,7 +3,7 @@ use crate::brillig::brillig_ir::brillig_variable::{ type_to_heap_value_type, BrilligArray, BrilligVariable, SingleAddrVariable, }; -use crate::brillig::brillig_ir::registers::{RegisterAllocator, Stack}; +use crate::brillig::brillig_ir::registers::RegisterAllocator; use crate::brillig::brillig_ir::{ BrilligBinaryOp, BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, }; @@ -86,6 +86,24 @@ impl<'block, 'global, Registers: RegisterAllocator> BrilligBlock<'block, 'global brillig_block.convert_block(dfg); } + pub(crate) fn compile_globals(&mut self, globals: &DataFlowGraph) { + for (id, value) in globals.values_iter() { + match value { + Value::NumericConstant { .. } => { + self.convert_ssa_value(id, globals); + } + Value::Instruction { instruction, .. } => { + self.convert_ssa_instruction(*instruction, globals); + } + _ => { + panic!( + "Expected either an instruction or a numeric constant for a global value" + ) + } + } + } + } + fn convert_block(&mut self, dfg: &DataFlowGraph) { // Add a label for this block let block_label = self.create_block_label_for_current_function(self.block_id); @@ -871,9 +889,8 @@ impl<'block, 'global, Registers: RegisterAllocator> BrilligBlock<'block, 'global for dead_variable in dead_variables { match &dfg[*dead_variable] { - Value::Global(_) => { - dbg!("got dead global"); - } + // Globals are reserved throughout the entirety of the program + Value::Global(_) => {} _ => { self.variables.remove_variable( dead_variable, @@ -1594,15 +1611,11 @@ impl<'block, 'global, Registers: RegisterAllocator> BrilligBlock<'block, 'global let value = &dfg[value_id]; match value { - Value::Global(_) => { - dbg!(value_id); - let variable = - *self.function_context.globals.get(&value_id).unwrap_or_else(|| { - panic!("ICE: Global value not found in cache {value_id}") - }); - dbg!(variable.clone()); - variable - } + Value::Global(_) => *self + .function_context + .globals + .get(&value_id) + .unwrap_or_else(|| panic!("ICE: Global value not found in cache {value_id}")), 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/brillig_block_variables.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs index e19ccdead65..4cf8e921483 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block_variables.rs @@ -7,7 +7,7 @@ use crate::{ get_bit_size_from_ssa_type, BrilligArray, BrilligVariable, BrilligVector, SingleAddrVariable, }, - registers::{RegisterAllocator, Stack}, + registers::RegisterAllocator, BrilligContext, }, ssa::ir::{ diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 22780d3e2ec..345de4300f4 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -8,7 +8,6 @@ use crate::{ ssa::ir::{ basic_block::BasicBlockId, function::{Function, FunctionId}, - map::Id, post_order::PostOrder, types::Type, value::ValueId, @@ -60,10 +59,10 @@ impl<'global> FunctionContext<'global> { function_id: FunctionId, globals: &'global HashMap, ) -> Self { - // let mut liveness = VariableLiveness::default(); - // liveness.l Self { // It does not matter what ID we have here + // This ID is only used for creating block labels and globals should + // have their own entirely separate label. function_id, ssa_value_allocations: Default::default(), blocks: Default::default(), diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 1388de4da8d..55e6259bd17 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -1,11 +1,37 @@ use acvm::FieldElement; use fxhash::FxHashMap as HashMap; -use super::{BrilligVariable, ValueId}; -use crate::brillig::{brillig_ir::BrilligContext, GlobalSpace}; +use super::{BrilligArtifact, BrilligBlock, BrilligVariable, FunctionContext, Label, ValueId}; +use crate::brillig::{brillig_ir::BrilligContext, DataFlowGraph, FunctionId}; -pub(crate) struct BrilligGlobals<'global> { - pub(crate) brillig_context: &'global mut BrilligContext, +pub(crate) fn convert_ssa_globals( + enable_debug_trace: bool, + dummy_function_id: FunctionId, + globals: &DataFlowGraph, +) -> (BrilligArtifact, HashMap) { + let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace); + // The global space does not have globals itself + let empty_globals = HashMap::default(); + // We can use any ID here as this context is only going to be used for globals which does not differentiate + // by functions and blocks. The only Label that should be used in the globals context is `Label::globals_init()` + let mut function_context = + FunctionContext::new_for_global_init(dummy_function_id, &empty_globals); + brillig_context.enter_context(Label::globals_init()); - brillig_globals: HashMap, + let block_id = DataFlowGraph::default().make_block(); + let mut brillig_block = BrilligBlock { + function_context: &mut function_context, + block_id, + brillig_context: &mut brillig_context, + variables: Default::default(), + last_uses: HashMap::default(), + building_globals: true, + }; + + brillig_block.compile_globals(globals); + + brillig_context.return_instruction(); + + let artifact = brillig_context.artifact(); + (artifact, function_context.ssa_value_allocations) } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index c77477676a4..51b1f11e4af 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -206,6 +206,7 @@ mod tests { brillig_context, variables, last_uses: Default::default(), + building_globals: false, } } diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index ce112873fe8..dc32eed7ea5 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -80,7 +80,7 @@ impl BrilligContext { self.copy_and_cast_calldata(arguments); let mut current_calldata_pointer = Self::calldata_start_offset(); - dbg!(current_calldata_pointer); + // Initialize the variables with the calldata for (argument_variable, argument) in argument_variables.iter_mut().zip(arguments) { match (argument_variable, argument) { @@ -334,7 +334,6 @@ impl BrilligContext { // Return data has a reserved space after calldata let return_data_offset = Self::return_data_start_offset(calldata_size); - // dbg!(return_data_offset); let mut return_data_index = return_data_offset; for (return_param, returned_variable) in return_parameters.iter().zip(&returned_variables) { diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index 0124b2633f2..20ab0dd4056 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -2,17 +2,8 @@ pub(crate) mod brillig_gen; pub(crate) mod brillig_ir; use acvm::FieldElement; -use brillig_gen::{ - brillig_block::BrilligBlock, - brillig_block_variables::{allocate_value_with_type, BlockVariables}, - brillig_fn::FunctionContext, -}; -use brillig_ir::{ - artifact::LabelType, - brillig_variable::{BrilligVariable, SingleAddrVariable}, - registers::GlobalSpace, - BrilligBinaryOp, BrilligContext, ReservedRegisters, -}; +use brillig_gen::brillig_globals::convert_ssa_globals; +use brillig_ir::{artifact::LabelType, brillig_variable::BrilligVariable, registers::GlobalSpace}; use self::{ brillig_gen::convert_ssa_function, @@ -23,17 +14,14 @@ use self::{ }; use crate::ssa::{ ir::{ - basic_block::BasicBlockId, dfg::DataFlowGraph, function::{Function, FunctionId}, - instruction::Instruction, - types::Type, - value::{Value, ValueId}, + value::ValueId, }, ssa_gen::Ssa, }; use fxhash::FxHashMap as HashMap; -use std::{borrow::Cow, collections::BTreeSet, sync::Arc}; +use std::{borrow::Cow, collections::BTreeSet}; pub use self::brillig_ir::procedures::ProcedureId; @@ -73,27 +61,6 @@ impl Brillig { _ => unreachable!("ICE: Expected a function or procedure label"), } } - - pub(crate) fn create_brillig_globals( - brillig_context: &mut BrilligBlock<'_, '_, GlobalSpace>, - globals: &DataFlowGraph, - ) { - for (id, value) in globals.values_iter() { - match value { - Value::NumericConstant { .. } => { - brillig_context.convert_ssa_value(id, globals); - } - Value::Instruction { instruction, .. } => { - brillig_context.convert_ssa_instruction(*instruction, globals); - } - _ => { - panic!( - "Expected either an instruction or a numeric constant for a global value" - ) - } - } - } - } } impl std::ops::Index for Brillig { @@ -117,30 +84,10 @@ impl Ssa { let mut brillig = Brillig::default(); - let mut brillig_context = BrilligContext::new_for_global_init(enable_debug_trace); - // We can use any ID here as this context is only going to be used for globals which does not differentiate - // by functions and blocks. The only Label that should be used in the globals context is `Label::globals_init()` - let globals = HashMap::default(); - let mut function_context = FunctionContext::new_for_global_init(self.main_id, &globals); - brillig_context.enter_context(Label::globals_init()); - - let block_id = DataFlowGraph::default().make_block(); - let mut brillig_block = BrilligBlock { - function_context: &mut function_context, - block_id, - brillig_context: &mut brillig_context, - variables: BlockVariables::default(), - last_uses: HashMap::default(), - building_globals: true, - }; - - Brillig::create_brillig_globals(&mut brillig_block, &self.globals); - brillig_context.return_instruction(); - - let artifact = brillig_context.artifact(); + let (artifact, brillig_globals) = + convert_ssa_globals(enable_debug_trace, self.main_id, &self.globals); brillig.globals = artifact; - let brillig_globals = function_context.ssa_value_allocations; for brillig_function_id in brillig_reachable_function_ids { let func = &self.functions[&brillig_function_id]; brillig.compile(func, enable_debug_trace, &brillig_globals); diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 01fabd0ff5d..a0d4b15d1c4 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -25,9 +25,8 @@ use im::HashSet; use crate::{ brillig::{ - brillig_gen::convert_ssa_function, - brillig_ir::{brillig_variable::BrilligVariable, BrilligContext}, - Brillig, + brillig_gen::{brillig_globals::convert_ssa_globals, convert_ssa_function}, + brillig_ir::brillig_variable::BrilligVariable, }, errors::RuntimeError, ssa::{ @@ -91,9 +90,8 @@ impl Ssa { if has_unrolled { if let Some((orig_function, max_incr_pct)) = orig_func_and_max_incr_pct { - let mut brillig_context = BrilligContext::new_for_global_init(true); - let brillig_globals = - Brillig::create_brillig_globals(&mut brillig_context, &self.globals); + let (_, brillig_globals) = + convert_ssa_globals(false, self.main_id, &self.globals); let new_size = brillig_bytecode_size(function, &brillig_globals); let orig_size = brillig_bytecode_size(&orig_function, &brillig_globals); 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 fa4557e63d9..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,14 +1,9 @@ global EXPONENTIATE: [[Field; 2]; 2] = [[1, 1], [0, 0]]; -// global EXPONENTIATE: [Field; 2] = [1, 1]; -// global EXPONENTIATE: Field = 10; - fn main(x: Field, y: pub Field) { let mut acc: Field = 0; for i in 0..2 { for j in 0..2 { - // let got_i = EXPONENTIATE[i]; - // acc += got_i[j]; acc += EXPONENTIATE[i][j]; } } @@ -23,7 +18,6 @@ fn dummy_again(x: Field, y: Field) { for i in 0..2 { for j in 0..2 { acc += EXPONENTIATE[i][j]; - // acc += EXPONENTIATE[j]; } } assert(!acc.lt(x)); diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index 0b7a02b8af9..700b9e61d5f 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -237,7 +237,7 @@ pub fn try_to_diagnose_runtime_error( } _ => return None, }; - dbg!(source_locations.clone()); + // The location of the error itself will be the location at the top // of the call stack (the last item in the Vec). let location = *source_locations.last()?; diff --git a/tooling/nargo/src/ops/execute.rs b/tooling/nargo/src/ops/execute.rs index 074ae93e129..57116ec2efd 100644 --- a/tooling/nargo/src/ops/execute.rs +++ b/tooling/nargo/src/ops/execute.rs @@ -137,17 +137,11 @@ impl<'a, F: AcirField, B: BlackBoxFunctionSolver, E: ForeignCallExecutor> }; return Err(NargoError::ExecutionError(match assertion_payload { - Some(payload) => { - dbg!(payload.clone()); - dbg!(call_stack.clone()); - dbg!(brillig_function_id); - ExecutionError::AssertionFailed( - payload, - call_stack - .expect("Should have call stack for an assertion failure"), - brillig_function_id, - ) - } + Some(payload) => ExecutionError::AssertionFailed( + payload, + call_stack.expect("Should have call stack for an assertion failure"), + brillig_function_id, + ), None => ExecutionError::SolvingError(error, call_stack), })); } From 8809385b59f096b637a9c3314b640d8cce5f95bc Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 16:57:24 +0000 Subject: [PATCH 23/25] remove pub(crate) on registers field from debugging --- compiler/noirc_evaluator/src/brillig/brillig_ir.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs index 4f3e22505c3..6016a9c3a9b 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_ir.rs @@ -82,7 +82,7 @@ impl ReservedRegisters { pub(crate) struct BrilligContext { obj: BrilligArtifact, /// Tracks register allocations - pub(crate) registers: Registers, + registers: Registers, /// Context label, must be unique with respect to the function /// being linked. context_label: Label, From 8ff2934db22e86cde9e3678f9302c9bdc37dda38 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 17:21:53 +0000 Subject: [PATCH 24/25] 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 4c390c72e5e7fbae2ef5f2a765ed69a29b32ce88 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Fri, 10 Jan 2025 17:38:07 +0000 Subject: [PATCH 25/25] fmt and clippy --- .../src/brillig/brillig_gen/brillig_fn.rs | 17 ----------------- .../src/brillig/brillig_gen/brillig_globals.rs | 9 +++++---- compiler/noirc_evaluator/src/brillig/mod.rs | 3 +-- .../noirc_evaluator/src/ssa/opt/unrolling.rs | 3 +-- 4 files changed, 7 insertions(+), 25 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 345de4300f4..6086eeb8802 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -55,23 +55,6 @@ impl<'global> FunctionContext<'global> { } } - pub(crate) fn new_for_global_init( - function_id: FunctionId, - globals: &'global HashMap, - ) -> Self { - Self { - // It does not matter what ID we have here - // This ID is only used for creating block labels and globals should - // have their own entirely separate label. - function_id, - ssa_value_allocations: Default::default(), - blocks: Default::default(), - liveness: Default::default(), - constant_allocation: Default::default(), - globals, - } - } - pub(crate) fn ssa_type_to_parameter(typ: &Type) -> BrilligParameter { match typ { Type::Numeric(_) | Type::Reference(_) => { diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs index 66cf14a8c21..ceba481c342 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_globals.rs @@ -1,8 +1,10 @@ use acvm::FieldElement; use fxhash::FxHashMap as HashMap; -use super::{BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId}; -use crate::brillig::{brillig_ir::BrilligContext, DataFlowGraph, FunctionId}; +use super::{ + BrilligArtifact, BrilligBlock, BrilligVariable, Function, FunctionContext, Label, ValueId, +}; +use crate::brillig::{brillig_ir::BrilligContext, DataFlowGraph}; pub(crate) fn convert_ssa_globals( enable_debug_trace: bool, @@ -13,8 +15,7 @@ pub(crate) fn convert_ssa_globals( let empty_globals = HashMap::default(); // We can use any ID here as this context is only going to be used for globals which does not differentiate // by functions and blocks. The only Label that should be used in the globals context is `Label::globals_init()` - let mut function_context = - FunctionContext::new(globals, &empty_globals); + let mut function_context = FunctionContext::new(globals, &empty_globals); brillig_context.enter_context(Label::globals_init()); let block_id = DataFlowGraph::default().make_block(); diff --git a/compiler/noirc_evaluator/src/brillig/mod.rs b/compiler/noirc_evaluator/src/brillig/mod.rs index c02da01bc29..4a804ef6426 100644 --- a/compiler/noirc_evaluator/src/brillig/mod.rs +++ b/compiler/noirc_evaluator/src/brillig/mod.rs @@ -84,8 +84,7 @@ impl Ssa { let mut brillig = Brillig::default(); - let (artifact, brillig_globals) = - convert_ssa_globals(enable_debug_trace, &self.globals); + let (artifact, brillig_globals) = convert_ssa_globals(enable_debug_trace, &self.globals); brillig.globals = artifact; for brillig_function_id in brillig_reachable_function_ids { diff --git a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 5ea3cbba6ee..00f0071353f 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -90,8 +90,7 @@ impl Ssa { if has_unrolled { if let Some((orig_function, max_incr_pct)) = orig_func_and_max_incr_pct { - let (_, brillig_globals) = - convert_ssa_globals(false, &self.globals); + let (_, brillig_globals) = convert_ssa_globals(false, &self.globals); let new_size = brillig_bytecode_size(function, &brillig_globals); let orig_size = brillig_bytecode_size(&orig_function, &brillig_globals);