From 6d5d9dea48517aa3fad0c78995ce364f5aeb99a9 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 21 Mar 2023 15:32:35 +0000 Subject: [PATCH 1/4] Add storage slots to globals --- crates/noirc_driver/src/lib.rs | 9 +++---- .../src/hir/def_collector/dc_crate.rs | 16 ++++++------- crates/noirc_frontend/src/hir/mod.rs | 24 +++++++++++++++++++ .../src/hir/resolution/resolver.rs | 12 +++++++--- .../src/monomorphization/mod.rs | 2 +- crates/noirc_frontend/src/node_interner.rs | 7 +++--- 6 files changed, 48 insertions(+), 22 deletions(-) diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index 4529d2224a3..131a78095e7 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -203,8 +203,8 @@ impl Driver { ) -> Result { let functions = try_btree_map(&contract.functions, |function| { let function_name = self.function_name(*function).to_owned(); - - self.compile_no_check(options, *function).map(|program| (function_name, program)) + let program = self.compile_no_check(options, *function)?; + Ok((function_name, program)) })?; Ok(CompiledContract { name: contract.name, functions }) @@ -225,10 +225,7 @@ impl Driver { }; // All Binaries should have a main function - match local_crate.main_function() { - Some(func_id) => Ok(func_id), - None => Err(ReportedError), - } + local_crate.main_function().ok_or(ReportedError) } /// Compile the current crate. Assumes self.check_crate is called beforehand! diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index 1133b4c052f..d62cba68d88 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -258,11 +258,10 @@ fn resolve_globals( globals: Vec, crate_id: CrateId, ) -> Vec<(FileId, StmtId)> { - let mut global_ids = Vec::new(); - - for global in globals { - let path_resolver = - StandardPathResolver::new(ModuleId { local_id: global.module_id, krate: crate_id }); + vecmap(globals, |global| { + let module_id = ModuleId { local_id: global.module_id, krate: crate_id }; + let path_resolver = StandardPathResolver::new(module_id); + let storage_slot = context.next_storage_slot(module_id); let mut resolver = Resolver::new( &mut context.def_interner, @@ -273,15 +272,14 @@ fn resolve_globals( let name = global.stmt_def.pattern.name_ident().clone(); - let hir_stmt = resolver.resolve_global_let(global.stmt_def); + let hir_stmt = resolver.resolve_global_let(global.stmt_def, storage_slot); context.def_interner.update_global(global.stmt_id, hir_stmt); context.def_interner.push_global(global.stmt_id, name.clone(), global.module_id); - global_ids.push((global.file_id, global.stmt_id)); - } - global_ids + (global.file_id, global.stmt_id) + }) } fn type_check_globals( diff --git a/crates/noirc_frontend/src/hir/mod.rs b/crates/noirc_frontend/src/hir/mod.rs index 984231545be..d52278421a2 100644 --- a/crates/noirc_frontend/src/hir/mod.rs +++ b/crates/noirc_frontend/src/hir/mod.rs @@ -20,8 +20,14 @@ pub struct Context { pub crate_graph: CrateGraph, pub(crate) def_maps: HashMap, pub file_manager: FileManager, + + /// Maps a given (contract) module id to the next available storage slot + /// for that contract. + pub storage_slots: HashMap, } +pub type StorageSlot = u32; + impl Context { pub fn new(file_manager: FileManager, crate_graph: CrateGraph, language: Language) -> Context { let mut ctx = Context { @@ -29,10 +35,12 @@ impl Context { def_maps: HashMap::new(), crate_graph, file_manager, + storage_slots: HashMap::new(), }; ctx.def_interner.set_language(&language); ctx } + /// Returns the CrateDefMap for a given CrateId. /// It is perfectly valid for the compiler to look /// up a CrateDefMap and it is not available. @@ -46,4 +54,20 @@ impl Context { pub fn crates(&self) -> impl Iterator + '_ { self.crate_graph.iter_keys() } + + fn module(&self, module_id: def_map::ModuleId) -> &def_map::ModuleData { + &self.def_maps[&module_id.krate].modules()[module_id.local_id.0] + } + + /// Returns the next available storage slot in the given module. + /// Returns None if the given module is not a contract module. + fn next_storage_slot(&mut self, module_id: def_map::ModuleId) -> Option { + let module = self.module(module_id); + + module.is_contract.then(|| { + let next_slot = self.storage_slots.entry(module_id).or_insert(0); + *next_slot += 1; + *next_slot + }) + } } diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index a6da1b3df1b..92f41dfedbf 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -1,3 +1,4 @@ +use crate::hir::StorageSlot; // Fix usage of intern and resolve // In some places, we do intern, however in others we are resolving and interning // Ideally, I want to separate the interning and resolving abstractly @@ -567,7 +568,7 @@ impl<'a> Resolver<'a> { for (stmt_id, global_info) in self.interner.get_all_globals() { if global_info.local_id == self.path_resolver.local_module_id() { let global_stmt = self.interner.let_statement(&stmt_id); - let definition = DefinitionKind::Global(global_stmt.expression); + let definition = DefinitionKind::Global(global_stmt.expression, None); // TODO self.add_global_variable_decl(global_info.ident, definition); } } @@ -722,9 +723,14 @@ impl<'a> Resolver<'a> { } } - pub fn resolve_global_let(&mut self, let_stmt: crate::LetStatement) -> HirStatement { + pub fn resolve_global_let( + &mut self, + let_stmt: crate::LetStatement, + storage_slot: Option, + ) -> HirStatement { let expression = self.resolve_expression(let_stmt.expression); - let definition = DefinitionKind::Global(expression); + let definition = DefinitionKind::Global(expression, storage_slot); + HirStatement::Let(HirLetStatement { pattern: self.resolve_pattern(let_stmt.pattern, definition), r#type: self.resolve_type(let_stmt.r#type), diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index d852db9ac24..1c9e1abe78d 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -504,7 +504,7 @@ impl<'interner> Monomorphizer<'interner> { let ident = ast::Ident { location, mutable, definition, name, typ }; ast::Expression::Ident(ident) } - DefinitionKind::Global(expr_id) => self.expr_infer(*expr_id), + DefinitionKind::Global(expr_id, _slot) => self.expr_infer(*expr_id), DefinitionKind::Local(_) => { let ident = self.local_ident(&ident).unwrap(); ast::Expression::Ident(ident) diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index 5516f12d3e4..c1abe6b19a7 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -11,6 +11,7 @@ use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::UnresolvedStruct; use crate::hir::def_map::{LocalModuleId, ModuleId}; use crate::hir::type_check::TypeCheckError; +use crate::hir::StorageSlot; use crate::hir_def::stmt::HirLetStatement; use crate::hir_def::types::{StructType, Type}; use crate::hir_def::{ @@ -202,7 +203,7 @@ impl DefinitionInfo { #[derive(Debug, Clone, Eq, PartialEq)] pub enum DefinitionKind { Function(FuncId), - Global(ExprId), + Global(ExprId, Option), /// Locals may be defined in let statements or parameters, /// in which case they will not have an associated ExprId @@ -217,13 +218,13 @@ impl DefinitionKind { /// True if this definition is for a global variable. /// Note that this returns false for top-level functions. pub fn is_global(&self) -> bool { - matches!(self, DefinitionKind::Global(_)) + matches!(self, DefinitionKind::Global(..)) } pub fn get_rhs(self) -> Option { match self { DefinitionKind::Function(_) => None, - DefinitionKind::Global(id) => Some(id), + DefinitionKind::Global(id, ..) => Some(id), DefinitionKind::Local(id) => id, DefinitionKind::GenericType(_) => None, } From 4a15a23e2724d280b6d535128fbb993758498a26 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 21 Mar 2023 15:41:45 +0000 Subject: [PATCH 2/4] Fix clippy lint --- crates/noirc_frontend/src/hir/def_collector/dc_crate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index d62cba68d88..c5d982f0e6e 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -276,7 +276,7 @@ fn resolve_globals( context.def_interner.update_global(global.stmt_id, hir_stmt); - context.def_interner.push_global(global.stmt_id, name.clone(), global.module_id); + context.def_interner.push_global(global.stmt_id, name, global.module_id); (global.file_id, global.stmt_id) }) From aab92947428340b20a668d93e557f8f7e45fe3e4 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 21 Mar 2023 16:40:34 +0000 Subject: [PATCH 3/4] Add doc comment --- crates/noirc_frontend/src/node_interner.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index c1abe6b19a7..229936e6568 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -203,6 +203,9 @@ impl DefinitionInfo { #[derive(Debug, Clone, Eq, PartialEq)] pub enum DefinitionKind { Function(FuncId), + + /// Global definitions have an associated storage slot if they are defined within + /// a contract. If they're defined elsewhere, this value is None. Global(ExprId, Option), /// Locals may be defined in let statements or parameters, From 86422acc4b4c44e6c47dbbfb506777577bfc3d59 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Tue, 21 Mar 2023 16:45:42 +0000 Subject: [PATCH 4/4] Reorganize storage slot code to avoid needing to store it on each global id --- .../src/hir/def_collector/dc_crate.rs | 4 ++-- .../src/hir/resolution/resolver.rs | 11 +++------- .../src/monomorphization/mod.rs | 2 +- crates/noirc_frontend/src/node_interner.rs | 20 +++++++++++++------ 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs index c5d982f0e6e..89dbc6b1e7f 100644 --- a/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/crates/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -272,11 +272,11 @@ fn resolve_globals( let name = global.stmt_def.pattern.name_ident().clone(); - let hir_stmt = resolver.resolve_global_let(global.stmt_def, storage_slot); + let hir_stmt = resolver.resolve_global_let(global.stmt_def); context.def_interner.update_global(global.stmt_id, hir_stmt); - context.def_interner.push_global(global.stmt_id, name, global.module_id); + context.def_interner.push_global(global.stmt_id, name, global.module_id, storage_slot); (global.file_id, global.stmt_id) }) diff --git a/crates/noirc_frontend/src/hir/resolution/resolver.rs b/crates/noirc_frontend/src/hir/resolution/resolver.rs index 92f41dfedbf..709751e8176 100644 --- a/crates/noirc_frontend/src/hir/resolution/resolver.rs +++ b/crates/noirc_frontend/src/hir/resolution/resolver.rs @@ -1,4 +1,3 @@ -use crate::hir::StorageSlot; // Fix usage of intern and resolve // In some places, we do intern, however in others we are resolving and interning // Ideally, I want to separate the interning and resolving abstractly @@ -568,7 +567,7 @@ impl<'a> Resolver<'a> { for (stmt_id, global_info) in self.interner.get_all_globals() { if global_info.local_id == self.path_resolver.local_module_id() { let global_stmt = self.interner.let_statement(&stmt_id); - let definition = DefinitionKind::Global(global_stmt.expression, None); // TODO + let definition = DefinitionKind::Global(global_stmt.expression); self.add_global_variable_decl(global_info.ident, definition); } } @@ -723,13 +722,9 @@ impl<'a> Resolver<'a> { } } - pub fn resolve_global_let( - &mut self, - let_stmt: crate::LetStatement, - storage_slot: Option, - ) -> HirStatement { + pub fn resolve_global_let(&mut self, let_stmt: crate::LetStatement) -> HirStatement { let expression = self.resolve_expression(let_stmt.expression); - let definition = DefinitionKind::Global(expression, storage_slot); + let definition = DefinitionKind::Global(expression); HirStatement::Let(HirLetStatement { pattern: self.resolve_pattern(let_stmt.pattern, definition), diff --git a/crates/noirc_frontend/src/monomorphization/mod.rs b/crates/noirc_frontend/src/monomorphization/mod.rs index 1c9e1abe78d..d852db9ac24 100644 --- a/crates/noirc_frontend/src/monomorphization/mod.rs +++ b/crates/noirc_frontend/src/monomorphization/mod.rs @@ -504,7 +504,7 @@ impl<'interner> Monomorphizer<'interner> { let ident = ast::Ident { location, mutable, definition, name, typ }; ast::Expression::Ident(ident) } - DefinitionKind::Global(expr_id, _slot) => self.expr_infer(*expr_id), + DefinitionKind::Global(expr_id) => self.expr_infer(*expr_id), DefinitionKind::Local(_) => { let ident = self.local_ident(&ident).unwrap(); ast::Expression::Ident(ident) diff --git a/crates/noirc_frontend/src/node_interner.rs b/crates/noirc_frontend/src/node_interner.rs index 229936e6568..2489c4aba3c 100644 --- a/crates/noirc_frontend/src/node_interner.rs +++ b/crates/noirc_frontend/src/node_interner.rs @@ -204,9 +204,7 @@ impl DefinitionInfo { pub enum DefinitionKind { Function(FuncId), - /// Global definitions have an associated storage slot if they are defined within - /// a contract. If they're defined elsewhere, this value is None. - Global(ExprId, Option), + Global(ExprId), /// Locals may be defined in let statements or parameters, /// in which case they will not have an associated ExprId @@ -227,7 +225,7 @@ impl DefinitionKind { pub fn get_rhs(self) -> Option { match self { DefinitionKind::Function(_) => None, - DefinitionKind::Global(id, ..) => Some(id), + DefinitionKind::Global(id) => Some(id), DefinitionKind::Local(id) => id, DefinitionKind::GenericType(_) => None, } @@ -238,6 +236,10 @@ impl DefinitionKind { pub struct GlobalInfo { pub ident: Ident, pub local_id: LocalModuleId, + + /// Global definitions have an associated storage slot if they are defined within + /// a contract. If they're defined elsewhere, this value is None. + pub storage_slot: Option, } impl Default for NodeInterner { @@ -335,8 +337,14 @@ impl NodeInterner { self.id_to_type.insert(definition_id.into(), typ); } - pub fn push_global(&mut self, stmt_id: StmtId, ident: Ident, local_id: LocalModuleId) { - self.globals.insert(stmt_id, GlobalInfo { ident, local_id }); + pub fn push_global( + &mut self, + stmt_id: StmtId, + ident: Ident, + local_id: LocalModuleId, + storage_slot: Option, + ) { + self.globals.insert(stmt_id, GlobalInfo { ident, local_id, storage_slot }); } /// Intern an empty global stmt. Used for collecting globals