From 6523811c53e1a6266109c3a02d66af3008e078e8 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 27 Aug 2024 15:53:05 -0400 Subject: [PATCH 01/57] `Assembler::invoke`: no longer returns an `Option` --- assembly/src/assembler/instruction/mod.rs | 12 +++++++++--- assembly/src/assembler/instruction/procedures.rs | 6 +++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/assembly/src/assembler/instruction/mod.rs b/assembly/src/assembler/instruction/mod.rs index 9465f380c..22b1c11d4 100644 --- a/assembly/src/assembler/instruction/mod.rs +++ b/assembly/src/assembler/instruction/mod.rs @@ -433,13 +433,19 @@ impl Assembler { // ----- exec/call instructions ------------------------------------------------------- Instruction::Exec(ref callee) => { - return self.invoke(InvokeKind::Exec, callee, proc_ctx, mast_forest_builder) + return self + .invoke(InvokeKind::Exec, callee, proc_ctx, mast_forest_builder) + .map(Into::into); }, Instruction::Call(ref callee) => { - return self.invoke(InvokeKind::Call, callee, proc_ctx, mast_forest_builder) + return self + .invoke(InvokeKind::Call, callee, proc_ctx, mast_forest_builder) + .map(Into::into); }, Instruction::SysCall(ref callee) => { - return self.invoke(InvokeKind::SysCall, callee, proc_ctx, mast_forest_builder) + return self + .invoke(InvokeKind::SysCall, callee, proc_ctx, mast_forest_builder) + .map(Into::into); }, Instruction::DynExec => return self.dynexec(mast_forest_builder), Instruction::DynCall => return self.dyncall(mast_forest_builder), diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index c751109a8..09d1acc4b 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -16,7 +16,7 @@ impl Assembler { callee: &InvocationTarget, proc_ctx: &mut ProcedureContext, mast_forest_builder: &mut MastForestBuilder, - ) -> Result, AssemblyError> { + ) -> Result { let span = callee.span(); let digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; self.invoke_mast_root(kind, span, digest, mast_forest_builder) @@ -28,7 +28,7 @@ impl Assembler { span: SourceSpan, mast_root: RpoDigest, mast_forest_builder: &mut MastForestBuilder, - ) -> Result, AssemblyError> { + ) -> Result { // Get the procedure from the assembler let current_source_file = self.source_manager.get(span.source_id()).ok(); @@ -116,7 +116,7 @@ impl Assembler { } }; - Ok(Some(mast_root_node_id)) + Ok(mast_root_node_id) } /// Creates a new DYN block for the dynamic code execution and return. From 4c77063389a72e766f0e4a2256d409ebfdd3e417 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 27 Aug 2024 16:29:37 -0400 Subject: [PATCH 02/57] `MastForestBuilder`: `ensure_node` -> `add_node` --- .../src/assembler/instruction/procedures.rs | 79 ++++++++++--------- assembly/src/assembler/mast_forest_builder.rs | 55 +++++-------- assembly/src/assembler/tests.rs | 15 ++-- assembly/src/library/tests.rs | 4 +- 4 files changed, 69 insertions(+), 84 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 09d1acc4b..0e846ff17 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -32,46 +32,47 @@ impl Assembler { // Get the procedure from the assembler let current_source_file = self.source_manager.get(span.source_id()).ok(); + // TODO(plafer): bring back // If the procedure is cached and is a system call, ensure that the call is valid. - match mast_forest_builder.find_procedure(&mast_root) { - Some(proc) if matches!(kind, InvokeKind::SysCall) => { - // Verify if this is a syscall, that the callee is a kernel procedure - // - // NOTE: The assembler is expected to know the full set of all kernel - // procedures at this point, so if we can't identify the callee as a - // kernel procedure, it is a definite error. - if !proc.visibility().is_syscall() { - return Err(AssemblyError::InvalidSysCallTarget { - span, - source_file: current_source_file, - callee: proc.fully_qualified_name().clone(), - }); - } - let maybe_kernel_path = proc.path(); - self.module_graph - .find_module(maybe_kernel_path) - .ok_or_else(|| AssemblyError::InvalidSysCallTarget { - span, - source_file: current_source_file.clone(), - callee: proc.fully_qualified_name().clone(), - }) - .and_then(|module| { - // Note: this module is guaranteed to be of AST variant, since we have the - // AST of a procedure contained in it (i.e. `proc`). Hence, it must be that - // the entire module is in AST representation as well. - if module.unwrap_ast().is_kernel() { - Ok(()) - } else { - Err(AssemblyError::InvalidSysCallTarget { - span, - source_file: current_source_file.clone(), - callee: proc.fully_qualified_name().clone(), - }) - } - })?; - }, - Some(_) | None => (), - } + // match mast_forest_builder.find_procedure(&mast_root) { + // Some(proc) if matches!(kind, InvokeKind::SysCall) => { + // // Verify if this is a syscall, that the callee is a kernel procedure + // // + // // NOTE: The assembler is expected to know the full set of all kernel + // // procedures at this point, so if we can't identify the callee as a + // // kernel procedure, it is a definite error. + // if !proc.visibility().is_syscall() { + // return Err(AssemblyError::InvalidSysCallTarget { + // span, + // source_file: current_source_file, + // callee: proc.fully_qualified_name().clone(), + // }); + // } + // let maybe_kernel_path = proc.path(); + // self.module_graph + // .find_module(maybe_kernel_path) + // .ok_or_else(|| AssemblyError::InvalidSysCallTarget { + // span, + // source_file: current_source_file.clone(), + // callee: proc.fully_qualified_name().clone(), + // }) + // .and_then(|module| { + // // Note: this module is guaranteed to be of AST variant, since we have the + // // AST of a procedure contained in it (i.e. `proc`). Hence, it must be that + // // the entire module is in AST representation as well. + // if module.unwrap_ast().is_kernel() { + // Ok(()) + // } else { + // Err(AssemblyError::InvalidSysCallTarget { + // span, + // source_file: current_source_file.clone(), + // callee: proc.fully_qualified_name().clone(), + // }) + // } + // })?; + // }, + // Some(_) | None => (), + // } let mast_root_node_id = { match kind { diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 88615e6df..6e695941a 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -25,8 +25,6 @@ const PROCEDURE_INLINING_THRESHOLD: usize = 32; /// /// The purpose of the builder is to ensure that the underlying MAST forest contains as little /// information as possible needed to adequately describe the logical MAST forest. Specifically: -/// - The builder ensures that only one copy of a given node exists in the MAST forest (i.e., no two -/// nodes have the same hash). /// - The builder tries to merge adjacent basic blocks and eliminate the source block whenever this /// does not have an impact on other nodes in the forest. #[derive(Clone, Debug, Default)] @@ -35,9 +33,6 @@ pub struct MastForestBuilder { /// nodes added to the MAST forest builder are also immediately added to the underlying MAST /// forest. mast_forest: MastForest, - /// A map of MAST node digests to their corresponding positions in the MAST forest. It is - /// guaranteed that a given digests maps to exactly one node in the MAST forest. - node_id_by_hash: BTreeMap, /// A map of all procedures added to the MAST forest indexed by their global procedure ID. /// This includes all local, exported, and re-exported procedures. In case multiple procedures /// with the same digest are added to the MAST forest builder, only the first procedure is @@ -46,7 +41,8 @@ pub struct MastForestBuilder { /// A map from procedure MAST root to its global procedure index. Similar to the `procedures` /// map, this map contains only the first inserted procedure for procedures with the same MAST /// root. - proc_gid_by_hash: BTreeMap, + /// TODO(plafer): fix docs + proc_gid_by_node_id: BTreeMap, /// A set of IDs for basic blocks which have been merged into a bigger basic blocks. This is /// used as a candidate set of nodes that may be eliminated if the are not referenced by any /// other node in the forest and are not a root of any procedure. @@ -142,8 +138,8 @@ impl MastForestBuilder { /// Returns a reference to the procedure with the specified MAST root, or None /// if such a procedure is not present in this MAST forest builder. #[inline(always)] - pub fn find_procedure(&self, mast_root: &RpoDigest) -> Option<&Procedure> { - self.proc_gid_by_hash.get(mast_root).and_then(|gid| self.get_procedure(*gid)) + pub fn find_procedure(&self, node_id: MastNodeId) -> Option<&Procedure> { + self.proc_gid_by_node_id.get(&node_id).and_then(|gid| self.get_procedure(*gid)) } /// Returns the [`MastNodeId`] of the procedure associated with a given MAST root, or None @@ -172,8 +168,6 @@ impl MastForestBuilder { gid: GlobalProcedureIndex, procedure: Procedure, ) -> Result<(), AssemblyError> { - let proc_root = self.mast_forest[procedure.body_node_id()].digest(); - // Check if an entry is already in this cache slot. // // If there is already a cache entry, but it conflicts with what we're trying to cache, @@ -191,7 +185,7 @@ impl MastForestBuilder { // We don't have a cache entry yet, but we do want to make sure we don't have a conflicting // cache entry with the same MAST root: - if let Some(cached) = self.find_procedure(&proc_root) { + if let Some(cached) = self.find_procedure(procedure.body_node_id()) { // Handle the case where a procedure with no locals is lowered to a MastForest // consisting only of an `External` node to another procedure which has one or more // locals. This will result in the calling procedure having the same digest as the @@ -213,7 +207,7 @@ impl MastForestBuilder { } self.mast_forest.make_root(procedure.body_node_id()); - self.proc_gid_by_hash.insert(proc_root, gid); + self.proc_gid_by_node_id.insert(procedure.body_node_id(), gid); self.procedures.insert(gid, procedure); Ok(()) @@ -346,22 +340,11 @@ impl MastForestBuilder { /// Node inserters impl MastForestBuilder { /// Adds a node to the forest, and returns the [`MastNodeId`] associated with it. - /// - /// If a [`MastNode`] which is equal to the current node was previously added, the previously - /// returned [`MastNodeId`] will be returned. This enforces this invariant that equal - /// [`MastNode`]s have equal [`MastNodeId`]s. - fn ensure_node(&mut self, node: MastNode) -> Result { - let node_digest = node.digest(); - - if let Some(node_id) = self.node_id_by_hash.get(&node_digest) { - // node already exists in the forest; return previously assigned id - Ok(*node_id) - } else { - let new_node_id = self.mast_forest.add_node(node)?; - self.node_id_by_hash.insert(node_digest, new_node_id); - - Ok(new_node_id) - } + /// + /// Note adding the same [`MastNode`] twice will result in two different [`MastNodeId`]s being + /// returned. + fn add_node(&mut self, node: MastNode) -> Result { + Ok(self.mast_forest.add_node(node)?) } /// Adds a basic block node to the forest, and returns the [`MastNodeId`] associated with it. @@ -371,7 +354,7 @@ impl MastForestBuilder { decorators: Option, ) -> Result { let block = MastNode::new_basic_block(operations, decorators)?; - self.ensure_node(block) + self.add_node(block) } /// Adds a join node to the forest, and returns the [`MastNodeId`] associated with it. @@ -381,7 +364,7 @@ impl MastForestBuilder { right_child: MastNodeId, ) -> Result { let join = MastNode::new_join(left_child, right_child, &self.mast_forest)?; - self.ensure_node(join) + self.add_node(join) } /// Adds a split node to the forest, and returns the [`MastNodeId`] associated with it. @@ -391,35 +374,35 @@ impl MastForestBuilder { else_branch: MastNodeId, ) -> Result { let split = MastNode::new_split(if_branch, else_branch, &self.mast_forest)?; - self.ensure_node(split) + self.add_node(split) } /// Adds a loop node to the forest, and returns the [`MastNodeId`] associated with it. pub fn ensure_loop(&mut self, body: MastNodeId) -> Result { let loop_node = MastNode::new_loop(body, &self.mast_forest)?; - self.ensure_node(loop_node) + self.add_node(loop_node) } /// Adds a call node to the forest, and returns the [`MastNodeId`] associated with it. pub fn ensure_call(&mut self, callee: MastNodeId) -> Result { let call = MastNode::new_call(callee, &self.mast_forest)?; - self.ensure_node(call) + self.add_node(call) } /// Adds a syscall node to the forest, and returns the [`MastNodeId`] associated with it. pub fn ensure_syscall(&mut self, callee: MastNodeId) -> Result { let syscall = MastNode::new_syscall(callee, &self.mast_forest)?; - self.ensure_node(syscall) + self.add_node(syscall) } /// Adds a dyn node to the forest, and returns the [`MastNodeId`] associated with it. pub fn ensure_dyn(&mut self) -> Result { - self.ensure_node(MastNode::new_dyn()) + self.add_node(MastNode::new_dyn()) } /// Adds an external node to the forest, and returns the [`MastNodeId`] associated with it. pub fn ensure_external(&mut self, mast_root: RpoDigest) -> Result { - self.ensure_node(MastNode::new_external(mast_root)) + self.add_node(MastNode::new_external(mast_root)) } } diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index 74059a1ba..851c40f82 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -154,8 +154,8 @@ fn nested_blocks() -> Result<(), Report> { Ok(()) } -/// Ensures that a single copy of procedures with the same MAST root are added only once to the MAST -/// forest. +/// Ensures that adding procedures with the same MAST root results in 2 different procedures in the +/// assembled program. #[test] fn duplicate_procedure() { let context = TestContext::new(); @@ -179,7 +179,7 @@ fn duplicate_procedure() { "#; let program = context.assemble(program_source).unwrap(); - assert_eq!(program.num_procedures(), 2); + assert_eq!(program.num_procedures(), 3); } /// Ensures that equal MAST nodes don't get added twice to a MAST forest @@ -202,17 +202,18 @@ fn duplicate_nodes() { let mut expected_mast_forest = MastForest::new(); // basic block: mul - let mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); + let first_mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); - // basic block: add + // basic block: add and mul let add_basic_block_id = expected_mast_forest.add_block(vec![Operation::Add], None).unwrap(); + let second_mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); // inner split: `if.true add else mul end` let inner_split_id = - expected_mast_forest.add_split(add_basic_block_id, mul_basic_block_id).unwrap(); + expected_mast_forest.add_split(add_basic_block_id, second_mul_basic_block_id).unwrap(); // root: outer split - let root_id = expected_mast_forest.add_split(mul_basic_block_id, inner_split_id).unwrap(); + let root_id = expected_mast_forest.add_split(first_mul_basic_block_id, inner_split_id).unwrap(); expected_mast_forest.make_root(root_id); diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index f4f32341f..22c0b822a 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -80,11 +80,11 @@ fn library_exports() -> Result<(), Report> { end "#; let bar = parse_module!(&context, "lib2::bar", bar); - let modules = [foo, bar]; + let lib2_modules = [foo, bar]; let lib2 = Assembler::new(context.source_manager()) .with_library(lib1)? - .assemble_library(modules.iter().cloned())?; + .assemble_library(lib2_modules.iter().cloned())?; let foo2 = QualifiedProcedureName::from_str("lib2::foo::foo2").unwrap(); let foo3 = QualifiedProcedureName::from_str("lib2::foo::foo3").unwrap(); From bc5cb7479761668292126a431c5e038c12a7fd29 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 27 Aug 2024 17:04:00 -0400 Subject: [PATCH 03/57] `Library`: use `MastNodeId` as unique identifier for procedures --- assembly/src/assembler/mod.rs | 44 ++++++++++++++++++++++++----------- assembly/src/library/error.rs | 2 -- assembly/src/library/mod.rs | 17 +++----------- assembly/src/library/tests.rs | 4 ---- 4 files changed, 33 insertions(+), 34 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index ba20fd764..e806f2c68 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -277,7 +277,7 @@ impl Assembler { let mut mast_forest_builder = MastForestBuilder::default(); - let exports = { + let mut exports = { let mut exports = BTreeMap::new(); for module_idx in ast_module_indices { @@ -289,10 +289,11 @@ impl Assembler { let gid = module_idx + proc_idx; self.compile_subgraph(gid, &mut mast_forest_builder)?; - let proc_hash = mast_forest_builder - .get_procedure_hash(gid) - .expect("compilation succeeded but root not found in cache"); - exports.insert(fqn, proc_hash); + let proc_root_node_id = mast_forest_builder + .get_procedure(gid) + .expect("compilation succeeded but root not found in cache") + .body_node_id(); + exports.insert(fqn, proc_root_node_id); } } @@ -300,7 +301,15 @@ impl Assembler { }; // TODO: show a warning if library exports are empty? - let (mast_forest, _) = mast_forest_builder.build(); + let (mast_forest, id_remappings) = mast_forest_builder.build(); + if let Some(id_remappings) = id_remappings { + for (_proc_name, node_id) in exports.iter_mut() { + if let Some(&new_node_id) = id_remappings.get(&node_id) { + *node_id = new_node_id; + } + } + } + Ok(Library::new(mast_forest.into(), exports)?) } @@ -327,22 +336,29 @@ impl Assembler { // AST (we just added them to the module graph) let ast_module = self.module_graph[module_idx].unwrap_ast().clone(); - let exports = ast_module + let mut exports = ast_module .exported_procedures() .map(|(proc_idx, fqn)| { let gid = module_idx + proc_idx; self.compile_subgraph(gid, &mut mast_forest_builder)?; - let proc_hash = mast_forest_builder - .get_procedure_hash(gid) - .expect("compilation succeeded but root not found in cache"); - Ok((fqn, proc_hash)) + let proc_root_node_id = mast_forest_builder + .get_procedure(gid) + .expect("compilation succeeded but root not found in cache") + .body_node_id(); + Ok((fqn, proc_root_node_id)) }) - .collect::, Report>>()?; + .collect::, Report>>()?; // TODO: show a warning if library exports are empty? - - let (mast_forest, _) = mast_forest_builder.build(); + let (mast_forest, id_remappings) = mast_forest_builder.build(); + if let Some(id_remappings) = id_remappings { + for (_proc_name, node_id) in exports.iter_mut() { + if let Some(&new_node_id) = id_remappings.get(&node_id) { + *node_id = new_node_id; + } + } + } let library = Library::new(mast_forest.into(), exports)?; Ok(library.try_into()?) } diff --git a/assembly/src/library/error.rs b/assembly/src/library/error.rs index 3df795ca3..c8e31f79d 100644 --- a/assembly/src/library/error.rs +++ b/assembly/src/library/error.rs @@ -11,6 +11,4 @@ pub enum LibraryError { InvalidKernelExport { procedure_path: QualifiedProcedureName }, #[error(transparent)] Kernel(#[from] KernelError), - #[error("invalid export: no procedure root for {procedure_path} procedure")] - NoProcedureRootForExport { procedure_path: QualifiedProcedureName }, } diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index c7656bb97..65e84eeda 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -69,24 +69,13 @@ impl Library { /// in the provided MAST forest. pub fn new( mast_forest: Arc, - exports: BTreeMap, + exports: BTreeMap, ) -> Result { - let mut fqn_to_export = BTreeMap::new(); - - // convert fqn |-> mast_root map into fqn |-> mast_node_id map - for (fqn, mast_root) in exports.into_iter() { - if let Some(proc_node_id) = mast_forest.find_procedure_root(mast_root) { - fqn_to_export.insert(fqn, proc_node_id); - } else { - return Err(LibraryError::NoProcedureRootForExport { procedure_path: fqn }); - } - } - - let digest = compute_content_hash(&fqn_to_export, &mast_forest); + let digest = compute_content_hash(&exports, &mast_forest); Ok(Self { digest, - exports: fqn_to_export, + exports, mast_forest, }) } diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index 22c0b822a..80d3ad904 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -98,10 +98,6 @@ fn library_exports() -> Result<(), Report> { let actual_exports: BTreeSet<_> = lib2.exports().collect(); assert_eq!(expected_exports, actual_exports); - // make sure foo2, bar2, and bar3 map to the same MastNode - assert_eq!(lib2.get_export_node_id(&foo2), lib2.get_export_node_id(&bar2)); - assert_eq!(lib2.get_export_node_id(&foo2), lib2.get_export_node_id(&bar3)); - // make sure there are 6 roots in the MAST (foo1, foo2, foo3, bar1, bar4, and bar5) assert_eq!(lib2.mast_forest.num_procedures(), 6); From 7272a559ed62ff3e712b093885a9d6ceb56f5c51 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 08:59:42 -0400 Subject: [PATCH 04/57] fix tests --- assembly/src/library/tests.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index 80d3ad904..3242d8eec 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -98,14 +98,16 @@ fn library_exports() -> Result<(), Report> { let actual_exports: BTreeSet<_> = lib2.exports().collect(); assert_eq!(expected_exports, actual_exports); - // make sure there are 6 roots in the MAST (foo1, foo2, foo3, bar1, bar4, and bar5) - assert_eq!(lib2.mast_forest.num_procedures(), 6); + // make sure there are 7 roots in the MAST (foo1, foo2, foo3, bar1, bar2, bar4, and bar5) + // Note that bar3 compiles to the same procedure as foo2, since `exec` returns the root node + // that is being exec'd. + assert_eq!(lib2.mast_forest.num_procedures(), 7); - // bar1 should be the only re-export + // bar1 and bar2 should be the only re-exports assert!(!lib2.is_reexport(&foo2)); assert!(!lib2.is_reexport(&foo3)); assert!(lib2.is_reexport(&bar1)); - assert!(!lib2.is_reexport(&bar2)); + assert!(lib2.is_reexport(&bar2)); assert!(!lib2.is_reexport(&bar3)); assert!(!lib2.is_reexport(&bar5)); @@ -150,18 +152,15 @@ fn library_procedure_collision() -> Result<(), Report> { .with_library(lib1)? .assemble_library([bar])?; - let bar1 = QualifiedProcedureName::from_str("lib2::bar::bar1").unwrap(); - let bar2 = QualifiedProcedureName::from_str("lib2::bar::bar2").unwrap(); + let lib2_bar_bar1 = QualifiedProcedureName::from_str("lib2::bar::bar1").unwrap(); + let lib2_bar_bar2 = QualifiedProcedureName::from_str("lib2::bar::bar2").unwrap(); // make sure lib2 has the expected exports (i.e., bar1 and bar2) assert_eq!(lib2.num_exports(), 2); - assert_eq!(lib2.get_export_node_id(&bar1), lib2.get_export_node_id(&bar2)); - - // make sure only one node was added to the forest - // NOTE: the MAST forest should actually have only 1 node (external node for the re-exported - // procedure), because nodes for the local procedure nodes should be pruned from the forest, - // but this is not implemented yet - assert_eq!(lib2.mast_forest().num_nodes(), 5); + + // make sure that bar1 and bar2 are different nodes in the MAST forest (since they could differ + // in their use of decorators) + assert_ne!(lib2.get_export_node_id(&lib2_bar_bar1), lib2.get_export_node_id(&lib2_bar_bar2)); Ok(()) } From 725eff5440286b73d7d2e1d65c6dbb884e184514 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 09:58:01 -0400 Subject: [PATCH 05/57] add failing test --- assembly/src/tests.rs | 51 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index 8b005f351..d237435cb 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1,4 +1,5 @@ use alloc::string::ToString; +use vm_core::mast::{MastNode, MastNodeId}; use crate::{ assert_diagnostic_lines, @@ -1307,6 +1308,56 @@ end"; // PROGRAMS WITH PROCEDURES // ================================================================================================ +/// If the program has 2 procedures with the same MAST root (but possibly different decorators), the +/// correct procedure is chosen on exec +#[test] +fn ensure_correct_procedure_selection_on_collision() -> TestResult { + let context = TestContext::default(); + + // if with else + let source = source_file!( + &context, + " + proc.f + add + end + + proc.g + trace.2 + add + end + + begin + if.true + exec.f + else + exec.g + end + end" + ); + let program = context.assemble(source)?; + + let expected_f_node_id = + MastNodeId::from_u32_safe(0_u32, program.mast_forest().as_ref()).unwrap(); + let expected_g_node_id = + MastNodeId::from_u32_safe(1_u32, program.mast_forest().as_ref()).unwrap(); + + let (f_node_id, g_node_id) = { + let split_node_id = program.entrypoint(); + let split_node = match &program.mast_forest()[split_node_id] { + MastNode::Split(split_node) => split_node, + _ => panic!("expected split node"), + }; + + (split_node.on_true(), split_node.on_false()) + }; + + assert_eq!(expected_f_node_id, f_node_id); + assert_eq!(expected_g_node_id, g_node_id); + + Ok(()) +} + #[test] fn program_with_one_procedure() -> TestResult { let context = TestContext::default(); From 62b4c522e8c592196290d67d649720d8c13da9d4 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 11:08:15 -0400 Subject: [PATCH 06/57] use `MastNodeId` to identify procedures in ModuleGraph --- assembly/src/assembler/mod.rs | 6 +-- assembly/src/assembler/module_graph/mod.rs | 42 +++++++++++++------ .../assembler/module_graph/name_resolver.rs | 2 + assembly/src/ast/procedure/resolver.rs | 1 + 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index e806f2c68..f5f257d61 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -455,7 +455,7 @@ impl Assembler { while let Some(procedure_gid) = worklist.pop() { // If we have already compiled this procedure, do not recompile if let Some(proc) = mast_forest_builder.get_procedure(procedure_gid) { - self.module_graph.register_mast_root(procedure_gid, proc.mast_root())?; + self.module_graph.register_procedure_root(procedure_gid, proc.mast_root())?; continue; } // Fetch procedure metadata from the graph @@ -494,7 +494,7 @@ impl Assembler { // be added to the forest. // Cache the compiled procedure - self.module_graph.register_mast_root(procedure_gid, procedure.mast_root())?; + self.module_graph.register_procedure_root(procedure_gid, procedure.mast_root())?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, Export::Alias(proc_alias) => { @@ -526,7 +526,7 @@ impl Assembler { let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); // Make the MAST root available to all dependents - self.module_graph.register_mast_root(procedure_gid, proc_mast_root)?; + self.module_graph.register_procedure_root(procedure_gid, proc_mast_root)?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, } diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index d4a2fff6a..08d419bc5 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -8,7 +8,7 @@ use alloc::{boxed::Box, collections::BTreeMap, sync::Arc, vec::Vec}; use core::ops::Index; use smallvec::{smallvec, SmallVec}; -use vm_core::Kernel; +use vm_core::{crypto::hash::RpoDigest, mast::MastNodeId, Kernel}; use self::{analysis::MaybeRewriteCheck, name_resolver::NameResolver, rewrites::ModuleRewriter}; pub use self::{ @@ -22,7 +22,7 @@ use crate::{ ResolvedProcedure, }, library::{ModuleInfo, ProcedureInfo}, - AssemblyError, LibraryNamespace, LibraryPath, RpoDigest, SourceManager, Spanned, + AssemblyError, LibraryNamespace, LibraryPath, SourceManager, Spanned, }; // WRAPPER STRUCTS @@ -158,9 +158,11 @@ pub struct ModuleGraph { /// The global call graph of calls, not counting those that are performed directly via MAST /// root. callgraph: CallGraph, - /// The set of MAST roots which have procedure definitions in this graph. There can be + /// The set of MAST node ids which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. - roots: BTreeMap>, + procedure_root_digests: BTreeMap>, + /// The set of MAST node ids which have procedure definitions in this graph. + procedure_root_ids: BTreeMap, kernel_index: Option, kernel: Kernel, source_manager: Arc, @@ -175,7 +177,8 @@ impl ModuleGraph { modules: Default::default(), pending: Default::default(), callgraph: Default::default(), - roots: Default::default(), + procedure_root_digests: Default::default(), + procedure_root_ids: Default::default(), kernel_index: None, kernel: Default::default(), source_manager, @@ -198,7 +201,7 @@ impl ModuleGraph { for &module_index in module_indices.iter() { for (proc_index, proc) in self[module_index].unwrap_info().clone().procedures() { let gid = module_index + proc_index; - self.register_mast_root(gid, proc.digest)?; + self.register_procedure_root(gid, proc.digest)?; } } @@ -520,11 +523,25 @@ impl ModuleGraph { } } + pub fn get_procedure_index_by_node_id( + &self, + node_id: MastNodeId, + ) -> Option { + self.procedure_root_ids.get(&node_id).copied() + } + + /// Returns a procedure index which corresponds to the provided procedure digest. + /// + /// Note that there can be many procedures with the same digest - due to having the same code, + /// and/or using different decorators which don't affect the MAST root. This method returns an + /// arbitrary one. pub fn get_procedure_index_by_digest( &self, - digest: &RpoDigest, + procedure_digest: &RpoDigest, ) -> Option { - self.roots.get(digest).map(|indices| indices[0]) + // TODO(plafer): emit warning if more than one? + // TODO(plafer): return the whole vec? + self.procedure_root_digests.get(procedure_digest).map(|indices| indices[0]) } /// Resolves `target` from the perspective of `caller`. @@ -537,7 +554,7 @@ impl ModuleGraph { resolver.resolve_target(caller, target) } - /// Registers a [RpoDigest] as corresponding to a given [GlobalProcedureIndex]. + /// Registers a [MastNodeId] as corresponding to a given [GlobalProcedureIndex]. /// /// # SAFETY /// @@ -545,13 +562,14 @@ impl ModuleGraph { /// procedure. It is fine if there are multiple procedures with the same digest, but it _must_ /// be the case that if a given digest is specified, it can be used as if it was the definition /// of the referenced procedure, i.e. they are referentially transparent. - pub(crate) fn register_mast_root( + pub(crate) fn register_procedure_root( &mut self, id: GlobalProcedureIndex, - digest: RpoDigest, + // TODO(plafer): all `procedure_root_id` field, and add to `self.procedure_root_ids` + procedure_root_digest: RpoDigest, ) -> Result<(), AssemblyError> { use alloc::collections::btree_map::Entry; - match self.roots.entry(digest) { + match self.procedure_root_digests.entry(procedure_root_digest) { Entry::Occupied(ref mut entry) => { let prev_id = entry.get()[0]; if prev_id != id { diff --git a/assembly/src/assembler/module_graph/name_resolver.rs b/assembly/src/assembler/module_graph/name_resolver.rs index f802f40c9..20e71ddad 100644 --- a/assembly/src/assembler/module_graph/name_resolver.rs +++ b/assembly/src/assembler/module_graph/name_resolver.rs @@ -214,6 +214,7 @@ impl<'a> NameResolver<'a> { }) }, Some(ResolvedProcedure::MastRoot(ref digest)) => { + // TODO(plafer): use `get_procedure_by_node_id()` once we switch `ResolvedProcedure::MastRoot` to being a `MastNodeId` match self.graph.get_procedure_index_by_digest(digest) { Some(gid) => Ok(ResolvedTarget::Exact { gid }), None => Ok(ResolvedTarget::Phantom(*digest)), @@ -351,6 +352,7 @@ impl<'a> NameResolver<'a> { current_callee = Cow::Owned(fqn); }, Some(ResolvedProcedure::MastRoot(ref digest)) => { + // TODO(plafer): use `get_procedure_by_node_id()` once we switch `ResolvedProcedure::MastRoot` to being a `MastNodeId` if let Some(id) = self.graph.get_procedure_index_by_digest(digest) { break Ok(id); } diff --git a/assembly/src/ast/procedure/resolver.rs b/assembly/src/ast/procedure/resolver.rs index 1a75df081..90d3b7a48 100644 --- a/assembly/src/ast/procedure/resolver.rs +++ b/assembly/src/ast/procedure/resolver.rs @@ -14,6 +14,7 @@ pub enum ResolvedProcedure { /// The name was resolved to a procedure exported from another module External(QualifiedProcedureName), /// The name was resolved to a procedure with a known MAST root + /// TODO(plafer): I think this needs to be a `MastNodeId` MastRoot(RpoDigest), } From 72a709370c638e35aeea807e6e1f802ec2ac8daf Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 11:14:45 -0400 Subject: [PATCH 07/57] ProcedureInfo: store `body_node_id` --- assembly/src/assembler/mod.rs | 18 ++++++++++-- assembly/src/assembler/module_graph/mod.rs | 32 +++++++++++++++------- assembly/src/errors.rs | 1 + assembly/src/library/mod.rs | 20 ++++++++------ assembly/src/library/module.rs | 6 ++-- 5 files changed, 54 insertions(+), 23 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index f5f257d61..b0ec3d697 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -455,7 +455,11 @@ impl Assembler { while let Some(procedure_gid) = worklist.pop() { // If we have already compiled this procedure, do not recompile if let Some(proc) = mast_forest_builder.get_procedure(procedure_gid) { - self.module_graph.register_procedure_root(procedure_gid, proc.mast_root())?; + self.module_graph.register_procedure_root( + procedure_gid, + proc.body_node_id(), + proc.mast_root(), + )?; continue; } // Fetch procedure metadata from the graph @@ -494,7 +498,11 @@ impl Assembler { // be added to the forest. // Cache the compiled procedure - self.module_graph.register_procedure_root(procedure_gid, procedure.mast_root())?; + self.module_graph.register_procedure_root( + procedure_gid, + procedure.body_node_id(), + procedure.mast_root(), + )?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, Export::Alias(proc_alias) => { @@ -526,7 +534,11 @@ impl Assembler { let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); // Make the MAST root available to all dependents - self.module_graph.register_procedure_root(procedure_gid, proc_mast_root)?; + self.module_graph.register_procedure_root( + procedure_gid, + proc_node_id, + proc_mast_root, + )?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, } diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index 08d419bc5..ef444f1f5 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -4,7 +4,12 @@ mod debug; mod name_resolver; mod rewrites; -use alloc::{boxed::Box, collections::BTreeMap, sync::Arc, vec::Vec}; +use alloc::{ + boxed::Box, + collections::{btree_map::Entry, BTreeMap}, + sync::Arc, + vec::Vec, +}; use core::ops::Index; use smallvec::{smallvec, SmallVec}; @@ -161,7 +166,7 @@ pub struct ModuleGraph { /// The set of MAST node ids which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. procedure_root_digests: BTreeMap>, - /// The set of MAST node ids which have procedure definitions in this graph. + /// The set of MAST node ids which have procedure definitions in this graph. procedure_root_ids: BTreeMap, kernel_index: Option, kernel: Kernel, @@ -201,7 +206,7 @@ impl ModuleGraph { for &module_index in module_indices.iter() { for (proc_index, proc) in self[module_index].unwrap_info().clone().procedures() { let gid = module_index + proc_index; - self.register_procedure_root(gid, proc.digest)?; + self.register_procedure_root(gid, proc.body_node_id, proc.digest)?; } } @@ -531,7 +536,7 @@ impl ModuleGraph { } /// Returns a procedure index which corresponds to the provided procedure digest. - /// + /// /// Note that there can be many procedures with the same digest - due to having the same code, /// and/or using different decorators which don't affect the MAST root. This method returns an /// arbitrary one. @@ -554,6 +559,7 @@ impl ModuleGraph { resolver.resolve_target(caller, target) } + // TODO(plafer): fix docs /// Registers a [MastNodeId] as corresponding to a given [GlobalProcedureIndex]. /// /// # SAFETY @@ -564,21 +570,27 @@ impl ModuleGraph { /// of the referenced procedure, i.e. they are referentially transparent. pub(crate) fn register_procedure_root( &mut self, - id: GlobalProcedureIndex, - // TODO(plafer): all `procedure_root_id` field, and add to `self.procedure_root_ids` + gid: GlobalProcedureIndex, + procedure_root_id: MastNodeId, procedure_root_digest: RpoDigest, ) -> Result<(), AssemblyError> { - use alloc::collections::btree_map::Entry; + if let Some(old_gid) = self.procedure_root_ids.insert(procedure_root_id, gid) { + assert!( + old_gid == gid, + "internal error: procedure root id {procedure_root_id:?} is associated with two procedure indices: {old_gid:?} and {gid:?}" + ); + } + match self.procedure_root_digests.entry(procedure_root_digest) { Entry::Occupied(ref mut entry) => { let prev_id = entry.get()[0]; - if prev_id != id { + if prev_id != gid { // Multiple procedures with the same root, but compatible - entry.get_mut().push(id); + entry.get_mut().push(gid); } }, Entry::Vacant(entry) => { - entry.insert(smallvec![id]); + entry.insert(smallvec![gid]); }, } diff --git a/assembly/src/errors.rs b/assembly/src/errors.rs index 8c4a3d22c..02814fd60 100644 --- a/assembly/src/errors.rs +++ b/assembly/src/errors.rs @@ -28,6 +28,7 @@ pub enum AssemblyError { #[error("found a cycle in the call graph, involving these procedures: {}", nodes.as_slice().join(", "))] #[diagnostic()] Cycle { nodes: Vec }, + // TODO(plafer): remove this error? #[error("two procedures found with same mast root, but conflicting definitions ('{first}' and '{second}')")] #[diagnostic()] ConflictingDefinitions { diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 65e84eeda..a739a734b 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -73,11 +73,7 @@ impl Library { ) -> Result { let digest = compute_content_hash(&exports, &mast_forest); - Ok(Self { - digest, - exports, - mast_forest, - }) + Ok(Self { digest, exports, mast_forest }) } } @@ -132,13 +128,21 @@ impl Library { .entry(proc_name.module.clone()) .and_modify(|compiled_module| { let proc_digest = self.mast_forest[proc_root_node_id].digest(); - compiled_module.add_procedure(proc_name.name.clone(), proc_digest); + compiled_module.add_procedure( + proc_name.name.clone(), + proc_root_node_id, + proc_digest, + ); }) .or_insert_with(|| { let mut module_info = ModuleInfo::new(proc_name.module.clone()); let proc_digest = self.mast_forest[proc_root_node_id].digest(); - module_info.add_procedure(proc_name.name.clone(), proc_digest); + module_info.add_procedure( + proc_name.name.clone(), + proc_root_node_id, + proc_digest, + ); module_info }); @@ -357,7 +361,7 @@ impl TryFrom for KernelLibrary { let proc_digest = library.mast_forest[proc_node_id].digest(); proc_digests.push(proc_digest); - kernel_module.add_procedure(proc_path.name.clone(), proc_digest); + kernel_module.add_procedure(proc_path.name.clone(), proc_node_id, proc_digest); } let kernel = Kernel::new(&proc_digests)?; diff --git a/assembly/src/library/module.rs b/assembly/src/library/module.rs index ed010d0e2..74b94b589 100644 --- a/assembly/src/library/module.rs +++ b/assembly/src/library/module.rs @@ -1,4 +1,5 @@ use alloc::vec::Vec; +use vm_core::mast::MastNodeId; use super::LibraryPath; use crate::{ @@ -22,8 +23,8 @@ impl ModuleInfo { } /// Adds a procedure to the module. - pub fn add_procedure(&mut self, name: ProcedureName, digest: RpoDigest) { - self.procedures.push(ProcedureInfo { name, digest }); + pub fn add_procedure(&mut self, name: ProcedureName, body_node_id: MastNodeId, digest: RpoDigest) { + self.procedures.push(ProcedureInfo { name, body_node_id, digest }); } /// Returns the module's library path. @@ -71,5 +72,6 @@ impl ModuleInfo { #[derive(Debug, Clone, PartialEq, Eq)] pub struct ProcedureInfo { pub name: ProcedureName, + pub body_node_id: MastNodeId, pub digest: RpoDigest, } From 30d2933ab67bd865b405e1807bd3f517ecfd2a96 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 12:57:47 -0400 Subject: [PATCH 08/57] `ResolvedProcedure`: store body_id when present --- assembly/src/assembler/module_graph/mod.rs | 9 ++++++++- .../src/assembler/module_graph/name_resolver.rs | 16 ++++++++++++++-- assembly/src/ast/procedure/resolver.rs | 8 +++++++- assembly/src/library/module.rs | 10 ++++++++++ 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index ef444f1f5..a5c2bd967 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -122,7 +122,14 @@ impl WrappedModule { match self { WrappedModule::Ast(module) => module.resolve(name), WrappedModule::Info(module) => { - module.get_procedure_digest_by_name(name).map(ResolvedProcedure::MastRoot) + let body_id = module.get_procedure_body_id_by_name(name); + let mast_root = module.get_procedure_digest_by_name(name); + + if let (Some(body_id), Some(mast_root)) = (body_id, mast_root) { + Some(ResolvedProcedure::BodyNodeId { mast_root, body_id }) + } else { + None + } }, } } diff --git a/assembly/src/assembler/module_graph/name_resolver.rs b/assembly/src/assembler/module_graph/name_resolver.rs index 20e71ddad..2c641ffeb 100644 --- a/assembly/src/assembler/module_graph/name_resolver.rs +++ b/assembly/src/assembler/module_graph/name_resolver.rs @@ -214,12 +214,17 @@ impl<'a> NameResolver<'a> { }) }, Some(ResolvedProcedure::MastRoot(ref digest)) => { - // TODO(plafer): use `get_procedure_by_node_id()` once we switch `ResolvedProcedure::MastRoot` to being a `MastNodeId` match self.graph.get_procedure_index_by_digest(digest) { Some(gid) => Ok(ResolvedTarget::Exact { gid }), None => Ok(ResolvedTarget::Phantom(*digest)), } }, + Some(ResolvedProcedure::BodyNodeId { mast_root, body_id }) => { + match self.graph.get_procedure_index_by_node_id(body_id) { + Some(gid) => Ok(ResolvedTarget::Exact { gid }), + None => Ok(ResolvedTarget::Phantom(mast_root)), + } + }, None => Err(AssemblyError::Failed { labels: vec![RelatedLabel::error("undefined procedure") .with_source_file(self.graph.source_manager.get(caller.span.source_id()).ok()) @@ -352,7 +357,6 @@ impl<'a> NameResolver<'a> { current_callee = Cow::Owned(fqn); }, Some(ResolvedProcedure::MastRoot(ref digest)) => { - // TODO(plafer): use `get_procedure_by_node_id()` once we switch `ResolvedProcedure::MastRoot` to being a `MastNodeId` if let Some(id) = self.graph.get_procedure_index_by_digest(digest) { break Ok(id); } @@ -382,6 +386,14 @@ impl<'a> NameResolver<'a> { ], }); }, + Some(ResolvedProcedure::BodyNodeId { mast_root, body_id }) => { + break Ok(self + .graph + .get_procedure_index_by_node_id(body_id) + .unwrap_or_else( || + panic!("internal error: resolved procedure with MAST root {mast_root} and body_id {body_id} not found in module graph") + )); + }, None if matches!(current_caller.kind, InvokeKind::SysCall) => { if self.graph.has_nonempty_kernel() { // No kernel, so this invoke is invalid anyway diff --git a/assembly/src/ast/procedure/resolver.rs b/assembly/src/ast/procedure/resolver.rs index 90d3b7a48..76609003a 100644 --- a/assembly/src/ast/procedure/resolver.rs +++ b/assembly/src/ast/procedure/resolver.rs @@ -1,4 +1,5 @@ use alloc::{collections::BTreeMap, vec::Vec}; +use vm_core::mast::MastNodeId; use super::{ProcedureIndex, ProcedureName, QualifiedProcedureName}; use crate::{ast::Ident, LibraryPath, RpoDigest, SourceSpan, Span, Spanned}; @@ -14,8 +15,12 @@ pub enum ResolvedProcedure { /// The name was resolved to a procedure exported from another module External(QualifiedProcedureName), /// The name was resolved to a procedure with a known MAST root - /// TODO(plafer): I think this needs to be a `MastNodeId` MastRoot(RpoDigest), + // TODO(plafer): rename and document + BodyNodeId { + mast_root: RpoDigest, + body_id: MastNodeId, + }, } impl Spanned for ResolvedProcedure { @@ -24,6 +29,7 @@ impl Spanned for ResolvedProcedure { ResolvedProcedure::Local(p) => p.span(), ResolvedProcedure::External(p) => p.span(), ResolvedProcedure::MastRoot(_) => SourceSpan::default(), + ResolvedProcedure::BodyNodeId { mast_root: _, body_id: _ } => SourceSpan::default(), } } } diff --git a/assembly/src/library/module.rs b/assembly/src/library/module.rs index 74b94b589..42129e77d 100644 --- a/assembly/src/library/module.rs +++ b/assembly/src/library/module.rs @@ -52,6 +52,16 @@ impl ModuleInfo { } }) } + /// Returns the digest of the procedure with the provided name, if any. + pub fn get_procedure_body_id_by_name(&self, name: &ProcedureName) -> Option { + self.procedures.iter().find_map(|proc_info| { + if &proc_info.name == name { + Some(proc_info.body_node_id) + } else { + None + } + }) + } /// Returns an iterator over the procedure infos in the module with their corresponding /// procedure index in the module. From c678944d541b1101dece5281b54a0186d043118f Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 13:22:55 -0400 Subject: [PATCH 09/57] create a new root node on exec --- .../src/assembler/instruction/procedures.rs | 28 +++++++++++++++---- assembly/src/assembler/mast_forest_builder.rs | 2 +- assembly/src/tests.rs | 16 +++++------ 3 files changed, 32 insertions(+), 14 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 0e846ff17..751f2b4d7 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -75,12 +75,12 @@ impl Assembler { // } let mast_root_node_id = { + // Note that here we rely on the fact that we topologically sorted the + // procedures, such that when we assemble a procedure, all + // procedures that it calls will have been assembled, and + // hence be present in the `MastForest`. match kind { - InvokeKind::Exec | InvokeKind::ProcRef => { - // Note that here we rely on the fact that we topologically sorted the - // procedures, such that when we assemble a procedure, all - // procedures that it calls will have been assembled, and - // hence be present in the `MastForest`. + InvokeKind::ProcRef => { match mast_forest_builder.find_procedure_node_id(mast_root) { Some(root) => root, None => { @@ -90,6 +90,24 @@ impl Assembler { }, } }, + InvokeKind::Exec => { + match mast_forest_builder.find_procedure_node_id(mast_root) { + Some(root) => { + // We make sure to copy the root node so that the `exec` is associated + // with a different `MastNodeId` than the procedure it is referencing. + // Currently the only purpose of this is so that simple procedures that + // only have an `exec` have a different body node id than the procedure + // they're executing. + let root_node = mast_forest_builder.get_mast_node(root).unwrap(); + mast_forest_builder.add_node(root_node.clone())? + }, + None => { + // If the MAST root called isn't known to us, make it an external + // reference. + mast_forest_builder.ensure_external(mast_root)? + }, + } + }, InvokeKind::Call => { let callee_id = match mast_forest_builder.find_procedure_node_id(mast_root) { Some(callee_id) => callee_id, diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 6e695941a..146f82115 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -343,7 +343,7 @@ impl MastForestBuilder { /// /// Note adding the same [`MastNode`] twice will result in two different [`MastNodeId`]s being /// returned. - fn add_node(&mut self, node: MastNode) -> Result { + pub fn add_node(&mut self, node: MastNode) -> Result { Ok(self.mast_forest.add_node(node)?) } diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index d237435cb..3923650c0 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -2546,10 +2546,10 @@ fn test_reexported_proc_with_same_name_as_local_proc_diff_locals() { let source = source_file!( &context, "export.foo.2 - push.1 - drop -end -" + push.1 + drop + end + " ); mod_parser.parse(LibraryPath::new("test::mod1").unwrap(), source).unwrap() }; @@ -2558,10 +2558,10 @@ end let source = source_file!( &context, "use.test::mod1 -export.foo - exec.mod1::foo -end -" + export.foo + exec.mod1::foo + end + " ); mod_parser.parse(LibraryPath::new("test::mod2").unwrap(), source).unwrap() }; From a352fa2709803a826b3afeeda076328fa3f6b172 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 13:45:25 -0400 Subject: [PATCH 10/57] Revert "ProcedureInfo: store `body_node_id`" This reverts commit 72a709370c638e35aeea807e6e1f802ec2ac8daf. --- assembly/src/assembler/mod.rs | 18 ++---------- assembly/src/assembler/module_graph/mod.rs | 32 +++++++--------------- assembly/src/errors.rs | 1 - assembly/src/library/mod.rs | 20 ++++++-------- assembly/src/library/module.rs | 6 ++-- 5 files changed, 23 insertions(+), 54 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index b0ec3d697..f5f257d61 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -455,11 +455,7 @@ impl Assembler { while let Some(procedure_gid) = worklist.pop() { // If we have already compiled this procedure, do not recompile if let Some(proc) = mast_forest_builder.get_procedure(procedure_gid) { - self.module_graph.register_procedure_root( - procedure_gid, - proc.body_node_id(), - proc.mast_root(), - )?; + self.module_graph.register_procedure_root(procedure_gid, proc.mast_root())?; continue; } // Fetch procedure metadata from the graph @@ -498,11 +494,7 @@ impl Assembler { // be added to the forest. // Cache the compiled procedure - self.module_graph.register_procedure_root( - procedure_gid, - procedure.body_node_id(), - procedure.mast_root(), - )?; + self.module_graph.register_procedure_root(procedure_gid, procedure.mast_root())?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, Export::Alias(proc_alias) => { @@ -534,11 +526,7 @@ impl Assembler { let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); // Make the MAST root available to all dependents - self.module_graph.register_procedure_root( - procedure_gid, - proc_node_id, - proc_mast_root, - )?; + self.module_graph.register_procedure_root(procedure_gid, proc_mast_root)?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, } diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index a5c2bd967..c1df7e5f0 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -4,12 +4,7 @@ mod debug; mod name_resolver; mod rewrites; -use alloc::{ - boxed::Box, - collections::{btree_map::Entry, BTreeMap}, - sync::Arc, - vec::Vec, -}; +use alloc::{boxed::Box, collections::BTreeMap, sync::Arc, vec::Vec}; use core::ops::Index; use smallvec::{smallvec, SmallVec}; @@ -173,7 +168,7 @@ pub struct ModuleGraph { /// The set of MAST node ids which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. procedure_root_digests: BTreeMap>, - /// The set of MAST node ids which have procedure definitions in this graph. + /// The set of MAST node ids which have procedure definitions in this graph. procedure_root_ids: BTreeMap, kernel_index: Option, kernel: Kernel, @@ -213,7 +208,7 @@ impl ModuleGraph { for &module_index in module_indices.iter() { for (proc_index, proc) in self[module_index].unwrap_info().clone().procedures() { let gid = module_index + proc_index; - self.register_procedure_root(gid, proc.body_node_id, proc.digest)?; + self.register_procedure_root(gid, proc.digest)?; } } @@ -543,7 +538,7 @@ impl ModuleGraph { } /// Returns a procedure index which corresponds to the provided procedure digest. - /// + /// /// Note that there can be many procedures with the same digest - due to having the same code, /// and/or using different decorators which don't affect the MAST root. This method returns an /// arbitrary one. @@ -566,7 +561,6 @@ impl ModuleGraph { resolver.resolve_target(caller, target) } - // TODO(plafer): fix docs /// Registers a [MastNodeId] as corresponding to a given [GlobalProcedureIndex]. /// /// # SAFETY @@ -577,27 +571,21 @@ impl ModuleGraph { /// of the referenced procedure, i.e. they are referentially transparent. pub(crate) fn register_procedure_root( &mut self, - gid: GlobalProcedureIndex, - procedure_root_id: MastNodeId, + id: GlobalProcedureIndex, + // TODO(plafer): all `procedure_root_id` field, and add to `self.procedure_root_ids` procedure_root_digest: RpoDigest, ) -> Result<(), AssemblyError> { - if let Some(old_gid) = self.procedure_root_ids.insert(procedure_root_id, gid) { - assert!( - old_gid == gid, - "internal error: procedure root id {procedure_root_id:?} is associated with two procedure indices: {old_gid:?} and {gid:?}" - ); - } - + use alloc::collections::btree_map::Entry; match self.procedure_root_digests.entry(procedure_root_digest) { Entry::Occupied(ref mut entry) => { let prev_id = entry.get()[0]; - if prev_id != gid { + if prev_id != id { // Multiple procedures with the same root, but compatible - entry.get_mut().push(gid); + entry.get_mut().push(id); } }, Entry::Vacant(entry) => { - entry.insert(smallvec![gid]); + entry.insert(smallvec![id]); }, } diff --git a/assembly/src/errors.rs b/assembly/src/errors.rs index 02814fd60..8c4a3d22c 100644 --- a/assembly/src/errors.rs +++ b/assembly/src/errors.rs @@ -28,7 +28,6 @@ pub enum AssemblyError { #[error("found a cycle in the call graph, involving these procedures: {}", nodes.as_slice().join(", "))] #[diagnostic()] Cycle { nodes: Vec }, - // TODO(plafer): remove this error? #[error("two procedures found with same mast root, but conflicting definitions ('{first}' and '{second}')")] #[diagnostic()] ConflictingDefinitions { diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index a739a734b..65e84eeda 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -73,7 +73,11 @@ impl Library { ) -> Result { let digest = compute_content_hash(&exports, &mast_forest); - Ok(Self { digest, exports, mast_forest }) + Ok(Self { + digest, + exports, + mast_forest, + }) } } @@ -128,21 +132,13 @@ impl Library { .entry(proc_name.module.clone()) .and_modify(|compiled_module| { let proc_digest = self.mast_forest[proc_root_node_id].digest(); - compiled_module.add_procedure( - proc_name.name.clone(), - proc_root_node_id, - proc_digest, - ); + compiled_module.add_procedure(proc_name.name.clone(), proc_digest); }) .or_insert_with(|| { let mut module_info = ModuleInfo::new(proc_name.module.clone()); let proc_digest = self.mast_forest[proc_root_node_id].digest(); - module_info.add_procedure( - proc_name.name.clone(), - proc_root_node_id, - proc_digest, - ); + module_info.add_procedure(proc_name.name.clone(), proc_digest); module_info }); @@ -361,7 +357,7 @@ impl TryFrom for KernelLibrary { let proc_digest = library.mast_forest[proc_node_id].digest(); proc_digests.push(proc_digest); - kernel_module.add_procedure(proc_path.name.clone(), proc_node_id, proc_digest); + kernel_module.add_procedure(proc_path.name.clone(), proc_digest); } let kernel = Kernel::new(&proc_digests)?; diff --git a/assembly/src/library/module.rs b/assembly/src/library/module.rs index 42129e77d..c85c32879 100644 --- a/assembly/src/library/module.rs +++ b/assembly/src/library/module.rs @@ -1,5 +1,4 @@ use alloc::vec::Vec; -use vm_core::mast::MastNodeId; use super::LibraryPath; use crate::{ @@ -23,8 +22,8 @@ impl ModuleInfo { } /// Adds a procedure to the module. - pub fn add_procedure(&mut self, name: ProcedureName, body_node_id: MastNodeId, digest: RpoDigest) { - self.procedures.push(ProcedureInfo { name, body_node_id, digest }); + pub fn add_procedure(&mut self, name: ProcedureName, digest: RpoDigest) { + self.procedures.push(ProcedureInfo { name, digest }); } /// Returns the module's library path. @@ -82,6 +81,5 @@ impl ModuleInfo { #[derive(Debug, Clone, PartialEq, Eq)] pub struct ProcedureInfo { pub name: ProcedureName, - pub body_node_id: MastNodeId, pub digest: RpoDigest, } From 5ae4fd5c40e73cd7a46def89e5c7022c62ea594c Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 13:49:16 -0400 Subject: [PATCH 11/57] fix build after `ProcedureInfo` revert --- assembly/src/assembler/module_graph/mod.rs | 13 +++---------- assembly/src/library/module.rs | 10 ---------- assembly/src/library/tests.rs | 6 ++---- 3 files changed, 5 insertions(+), 24 deletions(-) diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index c1df7e5f0..ab80b839c 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -117,14 +117,7 @@ impl WrappedModule { match self { WrappedModule::Ast(module) => module.resolve(name), WrappedModule::Info(module) => { - let body_id = module.get_procedure_body_id_by_name(name); - let mast_root = module.get_procedure_digest_by_name(name); - - if let (Some(body_id), Some(mast_root)) = (body_id, mast_root) { - Some(ResolvedProcedure::BodyNodeId { mast_root, body_id }) - } else { - None - } + module.get_procedure_digest_by_name(name).map(ResolvedProcedure::MastRoot) }, } } @@ -168,7 +161,7 @@ pub struct ModuleGraph { /// The set of MAST node ids which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. procedure_root_digests: BTreeMap>, - /// The set of MAST node ids which have procedure definitions in this graph. + /// The set of MAST node ids which have procedure definitions in this graph. procedure_root_ids: BTreeMap, kernel_index: Option, kernel: Kernel, @@ -538,7 +531,7 @@ impl ModuleGraph { } /// Returns a procedure index which corresponds to the provided procedure digest. - /// + /// /// Note that there can be many procedures with the same digest - due to having the same code, /// and/or using different decorators which don't affect the MAST root. This method returns an /// arbitrary one. diff --git a/assembly/src/library/module.rs b/assembly/src/library/module.rs index c85c32879..ed010d0e2 100644 --- a/assembly/src/library/module.rs +++ b/assembly/src/library/module.rs @@ -51,16 +51,6 @@ impl ModuleInfo { } }) } - /// Returns the digest of the procedure with the provided name, if any. - pub fn get_procedure_body_id_by_name(&self, name: &ProcedureName) -> Option { - self.procedures.iter().find_map(|proc_info| { - if &proc_info.name == name { - Some(proc_info.body_node_id) - } else { - None - } - }) - } /// Returns an iterator over the procedure infos in the module with their corresponding /// procedure index in the module. diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index 3242d8eec..3e845a55d 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -98,10 +98,8 @@ fn library_exports() -> Result<(), Report> { let actual_exports: BTreeSet<_> = lib2.exports().collect(); assert_eq!(expected_exports, actual_exports); - // make sure there are 7 roots in the MAST (foo1, foo2, foo3, bar1, bar2, bar4, and bar5) - // Note that bar3 compiles to the same procedure as foo2, since `exec` returns the root node - // that is being exec'd. - assert_eq!(lib2.mast_forest.num_procedures(), 7); + // make sure there are 8 roots in the MAST (foo1, foo2, foo3, bar1, bar2, bar3, bar4, and bar5) + assert_eq!(lib2.mast_forest.num_procedures(), 8); // bar1 and bar2 should be the only re-exports assert!(!lib2.is_reexport(&foo2)); From de51e2c94ab976b8d6fc42b05903e1c738d85fae Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 14:02:00 -0400 Subject: [PATCH 12/57] Remove `ResolvedProcedure::BodyNodeId` --- .../src/assembler/module_graph/name_resolver.rs | 14 -------------- assembly/src/ast/procedure/resolver.rs | 7 ------- 2 files changed, 21 deletions(-) diff --git a/assembly/src/assembler/module_graph/name_resolver.rs b/assembly/src/assembler/module_graph/name_resolver.rs index 2c641ffeb..f802f40c9 100644 --- a/assembly/src/assembler/module_graph/name_resolver.rs +++ b/assembly/src/assembler/module_graph/name_resolver.rs @@ -219,12 +219,6 @@ impl<'a> NameResolver<'a> { None => Ok(ResolvedTarget::Phantom(*digest)), } }, - Some(ResolvedProcedure::BodyNodeId { mast_root, body_id }) => { - match self.graph.get_procedure_index_by_node_id(body_id) { - Some(gid) => Ok(ResolvedTarget::Exact { gid }), - None => Ok(ResolvedTarget::Phantom(mast_root)), - } - }, None => Err(AssemblyError::Failed { labels: vec![RelatedLabel::error("undefined procedure") .with_source_file(self.graph.source_manager.get(caller.span.source_id()).ok()) @@ -386,14 +380,6 @@ impl<'a> NameResolver<'a> { ], }); }, - Some(ResolvedProcedure::BodyNodeId { mast_root, body_id }) => { - break Ok(self - .graph - .get_procedure_index_by_node_id(body_id) - .unwrap_or_else( || - panic!("internal error: resolved procedure with MAST root {mast_root} and body_id {body_id} not found in module graph") - )); - }, None if matches!(current_caller.kind, InvokeKind::SysCall) => { if self.graph.has_nonempty_kernel() { // No kernel, so this invoke is invalid anyway diff --git a/assembly/src/ast/procedure/resolver.rs b/assembly/src/ast/procedure/resolver.rs index 76609003a..1a75df081 100644 --- a/assembly/src/ast/procedure/resolver.rs +++ b/assembly/src/ast/procedure/resolver.rs @@ -1,5 +1,4 @@ use alloc::{collections::BTreeMap, vec::Vec}; -use vm_core::mast::MastNodeId; use super::{ProcedureIndex, ProcedureName, QualifiedProcedureName}; use crate::{ast::Ident, LibraryPath, RpoDigest, SourceSpan, Span, Spanned}; @@ -16,11 +15,6 @@ pub enum ResolvedProcedure { External(QualifiedProcedureName), /// The name was resolved to a procedure with a known MAST root MastRoot(RpoDigest), - // TODO(plafer): rename and document - BodyNodeId { - mast_root: RpoDigest, - body_id: MastNodeId, - }, } impl Spanned for ResolvedProcedure { @@ -29,7 +23,6 @@ impl Spanned for ResolvedProcedure { ResolvedProcedure::Local(p) => p.span(), ResolvedProcedure::External(p) => p.span(), ResolvedProcedure::MastRoot(_) => SourceSpan::default(), - ResolvedProcedure::BodyNodeId { mast_root: _, body_id: _ } => SourceSpan::default(), } } } From 03471eb225cc8067c2704d9a5a2770703c584b33 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 15:24:22 -0400 Subject: [PATCH 13/57] `Assembler::resolve_target` returns `Either` --- .../src/assembler/instruction/procedures.rs | 20 +++++++++---- assembly/src/assembler/mod.rs | 30 ++++++++++++------- core/src/utils/mod.rs | 11 +++++++ 3 files changed, 45 insertions(+), 16 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 751f2b4d7..0ff5bc4f4 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -1,5 +1,5 @@ use smallvec::SmallVec; -use vm_core::mast::MastNodeId; +use vm_core::{mast::MastNodeId, utils::Either}; use super::{Assembler, BasicBlockBuilder, Operation}; use crate::{ @@ -18,8 +18,11 @@ impl Assembler { mast_forest_builder: &mut MastForestBuilder, ) -> Result { let span = callee.span(); - let digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; - self.invoke_mast_root(kind, span, digest, mast_forest_builder) + let node_id_or_digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; + match node_id_or_digest { + Either::Left(node_id) => Ok(node_id), + Either::Right(digest) => self.invoke_mast_root(kind, span, digest, mast_forest_builder), + } } fn invoke_mast_root( @@ -168,8 +171,15 @@ impl Assembler { basic_block_builder: &mut BasicBlockBuilder, mast_forest_builder: &MastForestBuilder, ) -> Result<(), AssemblyError> { - let digest = - self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?; + let digest = match self.resolve_target( + InvokeKind::ProcRef, + callee, + proc_ctx, + mast_forest_builder, + )? { + Either::Left(node_id) => mast_forest_builder.get_mast_node(node_id).unwrap().digest(), + Either::Right(digest) => digest, + }; self.procref_mast_root(digest, basic_block_builder) } diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index f5f257d61..2df2da334 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -2,7 +2,7 @@ use alloc::{collections::BTreeMap, sync::Arc, vec::Vec}; use mast_forest_builder::MastForestBuilder; use module_graph::{ProcedureWrapper, WrappedModule}; -use vm_core::{mast::MastNodeId, DecoratorList, Felt, Kernel, Operation, Program}; +use vm_core::{mast::MastNodeId, utils::Either, DecoratorList, Felt, Kernel, Operation, Program}; use crate::{ ast::{self, Export, InvocationTarget, InvokeKind, ModuleKind, QualifiedProcedureName}, @@ -494,7 +494,8 @@ impl Assembler { // be added to the forest. // Cache the compiled procedure - self.module_graph.register_procedure_root(procedure_gid, procedure.mast_root())?; + self.module_graph + .register_procedure_root(procedure_gid, procedure.mast_root())?; mast_forest_builder.insert_procedure(procedure_gid, procedure)?; }, Export::Alias(proc_alias) => { @@ -512,12 +513,17 @@ impl Assembler { ) .with_span(proc_alias.span()); - let proc_mast_root = self.resolve_target( + let proc_mast_root = match self.resolve_target( InvokeKind::ProcRef, &proc_alias.target().into(), &pctx, mast_forest_builder, - )?; + )? { + Either::Left(node_id) => { + mast_forest_builder.get_mast_node(node_id).unwrap().digest() + }, + Either::Right(digest) => digest, + }; // insert external node into the MAST forest for this procedure; if a procedure // with the same MAST rood had been previously added to the builder, this will @@ -668,7 +674,7 @@ impl Assembler { target: &InvocationTarget, proc_ctx: &ProcedureContext, mast_forest_builder: &MastForestBuilder, - ) -> Result { + ) -> Result, AssemblyError> { let caller = CallerInfo { span: target.span(), module: proc_ctx.id().module, @@ -676,16 +682,18 @@ impl Assembler { }; let resolved = self.module_graph.resolve_target(&caller, target)?; match resolved { - ResolvedTarget::Phantom(digest) => Ok(digest), + ResolvedTarget::Phantom(digest) => Ok(Either::Right(digest)), ResolvedTarget::Exact { gid } | ResolvedTarget::Resolved { gid, .. } => { - match mast_forest_builder.get_procedure_hash(gid) { - Some(proc_hash) => Ok(proc_hash), + match mast_forest_builder.get_procedure(gid) { + Some(proc) => Ok(Either::Left(proc.body_node_id())), + // We didn't find the procedure in our current MAST forest. We still need to + // check if it exists in one of a library dependency. None => match self.module_graph.get_procedure_unsafe(gid) { - ProcedureWrapper::Info(p) => Ok(p.digest), - ProcedureWrapper::Ast(_) => panic!("Did not find procedure {gid:?} neither in module graph nor procedure cache"), + ProcedureWrapper::Info(p) => Ok(Either::Right(p.digest)), + ProcedureWrapper::Ast(_) => panic!("AST procedure {gid:?} exits in the module graph but not in the MastForestBuilder"), }, } - } + }, } } } diff --git a/core/src/utils/mod.rs b/core/src/utils/mod.rs index f610ed7f8..5147ea85f 100644 --- a/core/src/utils/mod.rs +++ b/core/src/utils/mod.rs @@ -130,3 +130,14 @@ fn debug_assert_is_checked() { // ================================================================================================ pub use miden_formatting::hex::{to_hex, DisplayHex, ToHex}; + +// EITHER +// ================================================================================================ + +/// Generic container for a choice between two types. +#[derive(Debug, Clone, Copy)] +pub enum Either { + Left(L), + Right(R) +} + From bc045b64d94e1f50577e502b5aad77d7cc24e9e9 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 15:34:20 -0400 Subject: [PATCH 14/57] fix failing test --- assembly/src/tests.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index 3923650c0..0cb3ceeff 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1321,7 +1321,7 @@ fn ensure_correct_procedure_selection_on_collision() -> TestResult { proc.f add end - + proc.g trace.2 add @@ -1337,10 +1337,12 @@ fn ensure_correct_procedure_selection_on_collision() -> TestResult { ); let program = context.assemble(source)?; + // Note: those values were taken from adding prints to the assembler at the time of writing. It + // is possible that this test starts failing if we end up ordering procedures differently. let expected_f_node_id = - MastNodeId::from_u32_safe(0_u32, program.mast_forest().as_ref()).unwrap(); - let expected_g_node_id = MastNodeId::from_u32_safe(1_u32, program.mast_forest().as_ref()).unwrap(); + let expected_g_node_id = + MastNodeId::from_u32_safe(0_u32, program.mast_forest().as_ref()).unwrap(); let (f_node_id, g_node_id) = { let split_node_id = program.entrypoint(); From 67a061a5d35e937f5a6bb6481675b5df9000220c Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 15:37:18 -0400 Subject: [PATCH 15/57] fmt --- assembly/src/assembler/instruction/procedures.rs | 8 ++++---- assembly/src/assembler/mast_forest_builder.rs | 2 +- assembly/src/assembler/tests.rs | 15 ++++++++++----- assembly/src/library/mod.rs | 6 +----- assembly/src/library/tests.rs | 2 +- assembly/src/tests.rs | 1 + core/src/utils/mod.rs | 3 +-- 7 files changed, 19 insertions(+), 18 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 0ff5bc4f4..a1bc11ad7 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -60,10 +60,10 @@ impl Assembler { // callee: proc.fully_qualified_name().clone(), // }) // .and_then(|module| { - // // Note: this module is guaranteed to be of AST variant, since we have the - // // AST of a procedure contained in it (i.e. `proc`). Hence, it must be that - // // the entire module is in AST representation as well. - // if module.unwrap_ast().is_kernel() { + // // Note: this module is guaranteed to be of AST variant, since we have + // the // AST of a procedure contained in it (i.e. `proc`). Hence, + // it must be that // the entire module is in AST representation as + // well. if module.unwrap_ast().is_kernel() { // Ok(()) // } else { // Err(AssemblyError::InvalidSysCallTarget { diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 146f82115..871cf2019 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -340,7 +340,7 @@ impl MastForestBuilder { /// Node inserters impl MastForestBuilder { /// Adds a node to the forest, and returns the [`MastNodeId`] associated with it. - /// + /// /// Note adding the same [`MastNode`] twice will result in two different [`MastNodeId`]s being /// returned. pub fn add_node(&mut self, node: MastNode) -> Result { diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index 851c40f82..e2d4885f2 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -202,18 +202,23 @@ fn duplicate_nodes() { let mut expected_mast_forest = MastForest::new(); // basic block: mul - let first_mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); + let first_mul_basic_block_id = + expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); // basic block: add and mul let add_basic_block_id = expected_mast_forest.add_block(vec![Operation::Add], None).unwrap(); - let second_mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); + let second_mul_basic_block_id = + expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); // inner split: `if.true add else mul end` - let inner_split_id = - expected_mast_forest.add_split(add_basic_block_id, second_mul_basic_block_id).unwrap(); + let inner_split_id = expected_mast_forest + .add_split(add_basic_block_id, second_mul_basic_block_id) + .unwrap(); // root: outer split - let root_id = expected_mast_forest.add_split(first_mul_basic_block_id, inner_split_id).unwrap(); + let root_id = expected_mast_forest + .add_split(first_mul_basic_block_id, inner_split_id) + .unwrap(); expected_mast_forest.make_root(root_id); diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 65e84eeda..925275e44 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -73,11 +73,7 @@ impl Library { ) -> Result { let digest = compute_content_hash(&exports, &mast_forest); - Ok(Self { - digest, - exports, - mast_forest, - }) + Ok(Self { digest, exports, mast_forest }) } } diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index 3e845a55d..27d120957 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -155,7 +155,7 @@ fn library_procedure_collision() -> Result<(), Report> { // make sure lib2 has the expected exports (i.e., bar1 and bar2) assert_eq!(lib2.num_exports(), 2); - + // make sure that bar1 and bar2 are different nodes in the MAST forest (since they could differ // in their use of decorators) assert_ne!(lib2.get_export_node_id(&lib2_bar_bar1), lib2.get_export_node_id(&lib2_bar_bar2)); diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index 0cb3ceeff..11794f8fe 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1,4 +1,5 @@ use alloc::string::ToString; + use vm_core::mast::{MastNode, MastNodeId}; use crate::{ diff --git a/core/src/utils/mod.rs b/core/src/utils/mod.rs index 5147ea85f..c4f564d97 100644 --- a/core/src/utils/mod.rs +++ b/core/src/utils/mod.rs @@ -138,6 +138,5 @@ pub use miden_formatting::hex::{to_hex, DisplayHex, ToHex}; #[derive(Debug, Clone, Copy)] pub enum Either { Left(L), - Right(R) + Right(R), } - From a3c5d8c9f5cfbcee23c62958548188cab36c71b8 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 15:49:59 -0400 Subject: [PATCH 16/57] add back map digest -> GID to mast forest builder --- .../src/assembler/instruction/procedures.rs | 79 +++++++++---------- assembly/src/assembler/mast_forest_builder.rs | 19 +++-- assembly/src/assembler/mod.rs | 4 +- assembly/src/assembler/module_graph/mod.rs | 12 +-- 4 files changed, 54 insertions(+), 60 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index a1bc11ad7..f0db884de 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -35,47 +35,46 @@ impl Assembler { // Get the procedure from the assembler let current_source_file = self.source_manager.get(span.source_id()).ok(); - // TODO(plafer): bring back // If the procedure is cached and is a system call, ensure that the call is valid. - // match mast_forest_builder.find_procedure(&mast_root) { - // Some(proc) if matches!(kind, InvokeKind::SysCall) => { - // // Verify if this is a syscall, that the callee is a kernel procedure - // // - // // NOTE: The assembler is expected to know the full set of all kernel - // // procedures at this point, so if we can't identify the callee as a - // // kernel procedure, it is a definite error. - // if !proc.visibility().is_syscall() { - // return Err(AssemblyError::InvalidSysCallTarget { - // span, - // source_file: current_source_file, - // callee: proc.fully_qualified_name().clone(), - // }); - // } - // let maybe_kernel_path = proc.path(); - // self.module_graph - // .find_module(maybe_kernel_path) - // .ok_or_else(|| AssemblyError::InvalidSysCallTarget { - // span, - // source_file: current_source_file.clone(), - // callee: proc.fully_qualified_name().clone(), - // }) - // .and_then(|module| { - // // Note: this module is guaranteed to be of AST variant, since we have - // the // AST of a procedure contained in it (i.e. `proc`). Hence, - // it must be that // the entire module is in AST representation as - // well. if module.unwrap_ast().is_kernel() { - // Ok(()) - // } else { - // Err(AssemblyError::InvalidSysCallTarget { - // span, - // source_file: current_source_file.clone(), - // callee: proc.fully_qualified_name().clone(), - // }) - // } - // })?; - // }, - // Some(_) | None => (), - // } + match mast_forest_builder.find_procedure_by_digest(&mast_root) { + Some(proc) if matches!(kind, InvokeKind::SysCall) => { + // Verify if this is a syscall, that the callee is a kernel procedure + // + // NOTE: The assembler is expected to know the full set of all kernel + // procedures at this point, so if we can't identify the callee as a + // kernel procedure, it is a definite error. + if !proc.visibility().is_syscall() { + return Err(AssemblyError::InvalidSysCallTarget { + span, + source_file: current_source_file, + callee: proc.fully_qualified_name().clone(), + }); + } + let maybe_kernel_path = proc.path(); + self.module_graph + .find_module(maybe_kernel_path) + .ok_or_else(|| AssemblyError::InvalidSysCallTarget { + span, + source_file: current_source_file.clone(), + callee: proc.fully_qualified_name().clone(), + }) + .and_then(|module| { + // Note: this module is guaranteed to be of AST variant, since we have the + // AST of a procedure contained in it (i.e. `proc`). Hence, it must be + // thatthe entire module is in AST representation as well. + if module.unwrap_ast().is_kernel() { + Ok(()) + } else { + Err(AssemblyError::InvalidSysCallTarget { + span, + source_file: current_source_file.clone(), + callee: proc.fully_qualified_name().clone(), + }) + } + })?; + }, + Some(_) | None => (), + } let mast_root_node_id = { // Note that here we rely on the fact that we topologically sorted the diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 871cf2019..8deab0598 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -43,6 +43,10 @@ pub struct MastForestBuilder { /// root. /// TODO(plafer): fix docs proc_gid_by_node_id: BTreeMap, + /// A map from procedure MAST root to its global procedure index. Similar to the `procedures` + /// map, this map contains only the first inserted procedure for procedures with the same MAST + /// root. + proc_gid_by_digest: BTreeMap, /// A set of IDs for basic blocks which have been merged into a bigger basic blocks. This is /// used as a candidate set of nodes that may be eliminated if the are not referenced by any /// other node in the forest and are not a root of any procedure. @@ -128,18 +132,18 @@ impl MastForestBuilder { self.procedures.get(&gid) } - /// Returns the hash of the procedure with the specified [`GlobalProcedureIndex`], or None if - /// such a procedure is not present in this MAST forest builder. + /// Returns a reference to the procedure with the specified [`MastNodeId`], or None + /// if such a procedure is not present in this MAST forest builder. #[inline(always)] - pub fn get_procedure_hash(&self, gid: GlobalProcedureIndex) -> Option { - self.procedures.get(&gid).map(|proc| proc.mast_root()) + pub fn find_procedure_by_id(&self, node_id: MastNodeId) -> Option<&Procedure> { + self.proc_gid_by_node_id.get(&node_id).and_then(|gid| self.get_procedure(*gid)) } /// Returns a reference to the procedure with the specified MAST root, or None /// if such a procedure is not present in this MAST forest builder. #[inline(always)] - pub fn find_procedure(&self, node_id: MastNodeId) -> Option<&Procedure> { - self.proc_gid_by_node_id.get(&node_id).and_then(|gid| self.get_procedure(*gid)) + pub fn find_procedure_by_digest(&self, digest: &RpoDigest) -> Option<&Procedure> { + self.proc_gid_by_digest.get(digest).and_then(|gid| self.get_procedure(*gid)) } /// Returns the [`MastNodeId`] of the procedure associated with a given MAST root, or None @@ -185,7 +189,7 @@ impl MastForestBuilder { // We don't have a cache entry yet, but we do want to make sure we don't have a conflicting // cache entry with the same MAST root: - if let Some(cached) = self.find_procedure(procedure.body_node_id()) { + if let Some(cached) = self.find_procedure_by_id(procedure.body_node_id()) { // Handle the case where a procedure with no locals is lowered to a MastForest // consisting only of an `External` node to another procedure which has one or more // locals. This will result in the calling procedure having the same digest as the @@ -208,6 +212,7 @@ impl MastForestBuilder { self.mast_forest.make_root(procedure.body_node_id()); self.proc_gid_by_node_id.insert(procedure.body_node_id(), gid); + self.proc_gid_by_digest.insert(procedure.mast_root(), gid); self.procedures.insert(gid, procedure); Ok(()) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 2df2da334..ea153d1a3 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -304,7 +304,7 @@ impl Assembler { let (mast_forest, id_remappings) = mast_forest_builder.build(); if let Some(id_remappings) = id_remappings { for (_proc_name, node_id) in exports.iter_mut() { - if let Some(&new_node_id) = id_remappings.get(&node_id) { + if let Some(&new_node_id) = id_remappings.get(node_id) { *node_id = new_node_id; } } @@ -354,7 +354,7 @@ impl Assembler { let (mast_forest, id_remappings) = mast_forest_builder.build(); if let Some(id_remappings) = id_remappings { for (_proc_name, node_id) in exports.iter_mut() { - if let Some(&new_node_id) = id_remappings.get(&node_id) { + if let Some(&new_node_id) = id_remappings.get(node_id) { *node_id = new_node_id; } } diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index ab80b839c..420cfc661 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -8,7 +8,7 @@ use alloc::{boxed::Box, collections::BTreeMap, sync::Arc, vec::Vec}; use core::ops::Index; use smallvec::{smallvec, SmallVec}; -use vm_core::{crypto::hash::RpoDigest, mast::MastNodeId, Kernel}; +use vm_core::{crypto::hash::RpoDigest, Kernel}; use self::{analysis::MaybeRewriteCheck, name_resolver::NameResolver, rewrites::ModuleRewriter}; pub use self::{ @@ -161,8 +161,6 @@ pub struct ModuleGraph { /// The set of MAST node ids which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. procedure_root_digests: BTreeMap>, - /// The set of MAST node ids which have procedure definitions in this graph. - procedure_root_ids: BTreeMap, kernel_index: Option, kernel: Kernel, source_manager: Arc, @@ -178,7 +176,6 @@ impl ModuleGraph { pending: Default::default(), callgraph: Default::default(), procedure_root_digests: Default::default(), - procedure_root_ids: Default::default(), kernel_index: None, kernel: Default::default(), source_manager, @@ -523,13 +520,6 @@ impl ModuleGraph { } } - pub fn get_procedure_index_by_node_id( - &self, - node_id: MastNodeId, - ) -> Option { - self.procedure_root_ids.get(&node_id).copied() - } - /// Returns a procedure index which corresponds to the provided procedure digest. /// /// Note that there can be many procedures with the same digest - due to having the same code, From b45d45913456f79f4c3b008b89b1bba3b7096ac2 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 15:52:35 -0400 Subject: [PATCH 17/57] `MastForestBuilder`: change all `ensure_*` to `add_*` --- assembly/src/assembler/basic_block_builder.rs | 2 +- .../src/assembler/instruction/procedures.rs | 18 +++++------ assembly/src/assembler/mast_forest_builder.rs | 22 ++++++------- assembly/src/assembler/mod.rs | 8 ++--- assembly/src/assembler/tests.rs | 32 +++++++++---------- 5 files changed, 40 insertions(+), 42 deletions(-) diff --git a/assembly/src/assembler/basic_block_builder.rs b/assembly/src/assembler/basic_block_builder.rs index 5a6a05cab..c535115a9 100644 --- a/assembly/src/assembler/basic_block_builder.rs +++ b/assembly/src/assembler/basic_block_builder.rs @@ -139,7 +139,7 @@ impl BasicBlockBuilder { let ops = self.ops.drain(..).collect(); let decorators = self.decorators.drain(..).collect(); - let basic_block_node_id = mast_forest_builder.ensure_block(ops, Some(decorators))?; + let basic_block_node_id = mast_forest_builder.add_block(ops, Some(decorators))?; Ok(Some(basic_block_node_id)) } else if !self.decorators.is_empty() { diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index f0db884de..708cc8d78 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -88,7 +88,7 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.ensure_external(mast_root)? + mast_forest_builder.add_external(mast_root)? }, } }, @@ -106,7 +106,7 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.ensure_external(mast_root)? + mast_forest_builder.add_external(mast_root)? }, } }, @@ -116,11 +116,11 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.ensure_external(mast_root)? + mast_forest_builder.add_external(mast_root)? }, }; - mast_forest_builder.ensure_call(callee_id)? + mast_forest_builder.add_call(callee_id)? }, InvokeKind::SysCall => { let callee_id = match mast_forest_builder.find_procedure_node_id(mast_root) { @@ -128,11 +128,11 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.ensure_external(mast_root)? + mast_forest_builder.add_external(mast_root)? }, }; - mast_forest_builder.ensure_syscall(callee_id)? + mast_forest_builder.add_syscall(callee_id)? }, } }; @@ -145,7 +145,7 @@ impl Assembler { &self, mast_forest_builder: &mut MastForestBuilder, ) -> Result, AssemblyError> { - let dyn_node_id = mast_forest_builder.ensure_dyn()?; + let dyn_node_id = mast_forest_builder.add_dyn()?; Ok(Some(dyn_node_id)) } @@ -156,8 +156,8 @@ impl Assembler { mast_forest_builder: &mut MastForestBuilder, ) -> Result, AssemblyError> { let dyn_call_node_id = { - let dyn_node_id = mast_forest_builder.ensure_dyn()?; - mast_forest_builder.ensure_call(dyn_node_id)? + let dyn_node_id = mast_forest_builder.add_dyn()?; + mast_forest_builder.add_call(dyn_node_id)? }; Ok(Some(dyn_call_node_id)) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 8deab0598..0f81cf19b 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -239,7 +239,7 @@ impl MastForestBuilder { while let (Some(left), Some(right)) = (source_mast_node_iter.next(), source_mast_node_iter.next()) { - let join_mast_node_id = self.ensure_join(left, right)?; + let join_mast_node_id = self.add_join(left, right)?; node_ids.push(join_mast_node_id); } @@ -321,7 +321,7 @@ impl MastForestBuilder { let block_ops = core::mem::take(&mut operations); let block_decorators = core::mem::take(&mut decorators); let merged_basic_block_id = - self.ensure_block(block_ops, Some(block_decorators))?; + self.add_block(block_ops, Some(block_decorators))?; merged_basic_blocks.push(merged_basic_block_id); } @@ -333,7 +333,7 @@ impl MastForestBuilder { self.merged_basic_block_ids.extend(contiguous_basic_block_ids.iter()); if !operations.is_empty() || !decorators.is_empty() { - let merged_basic_block = self.ensure_block(operations, Some(decorators))?; + let merged_basic_block = self.add_block(operations, Some(decorators))?; merged_basic_blocks.push(merged_basic_block); } @@ -353,7 +353,7 @@ impl MastForestBuilder { } /// Adds a basic block node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_block( + pub fn add_block( &mut self, operations: Vec, decorators: Option, @@ -363,7 +363,7 @@ impl MastForestBuilder { } /// Adds a join node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_join( + pub fn add_join( &mut self, left_child: MastNodeId, right_child: MastNodeId, @@ -373,7 +373,7 @@ impl MastForestBuilder { } /// Adds a split node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_split( + pub fn add_split( &mut self, if_branch: MastNodeId, else_branch: MastNodeId, @@ -383,30 +383,30 @@ impl MastForestBuilder { } /// Adds a loop node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_loop(&mut self, body: MastNodeId) -> Result { + pub fn add_loop(&mut self, body: MastNodeId) -> Result { let loop_node = MastNode::new_loop(body, &self.mast_forest)?; self.add_node(loop_node) } /// Adds a call node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_call(&mut self, callee: MastNodeId) -> Result { + pub fn add_call(&mut self, callee: MastNodeId) -> Result { let call = MastNode::new_call(callee, &self.mast_forest)?; self.add_node(call) } /// Adds a syscall node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_syscall(&mut self, callee: MastNodeId) -> Result { + pub fn add_syscall(&mut self, callee: MastNodeId) -> Result { let syscall = MastNode::new_syscall(callee, &self.mast_forest)?; self.add_node(syscall) } /// Adds a dyn node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_dyn(&mut self) -> Result { + pub fn add_dyn(&mut self) -> Result { self.add_node(MastNode::new_dyn()) } /// Adds an external node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn ensure_external(&mut self, mast_root: RpoDigest) -> Result { + pub fn add_external(&mut self, mast_root: RpoDigest) -> Result { self.add_node(MastNode::new_external(mast_root)) } } diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index ea153d1a3..44fa3c504 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -528,7 +528,7 @@ impl Assembler { // insert external node into the MAST forest for this procedure; if a procedure // with the same MAST rood had been previously added to the builder, this will // have no effect - let proc_node_id = mast_forest_builder.ensure_external(proc_mast_root)?; + let proc_node_id = mast_forest_builder.add_external(proc_mast_root)?; let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); // Make the MAST root available to all dependents @@ -620,7 +620,7 @@ impl Assembler { let else_blk = self.compile_body(else_blk.iter(), proc_ctx, None, mast_forest_builder)?; - let split_node_id = mast_forest_builder.ensure_split(then_blk, else_blk)?; + let split_node_id = mast_forest_builder.add_split(then_blk, else_blk)?; node_ids.push(split_node_id); }, @@ -649,7 +649,7 @@ impl Assembler { let loop_body_node_id = self.compile_body(body.iter(), proc_ctx, None, mast_forest_builder)?; - let loop_node_id = mast_forest_builder.ensure_loop(loop_body_node_id)?; + let loop_node_id = mast_forest_builder.add_loop(loop_body_node_id)?; node_ids.push(loop_node_id); }, } @@ -662,7 +662,7 @@ impl Assembler { } Ok(if node_ids.is_empty() { - mast_forest_builder.ensure_block(vec![Operation::Noop], None)? + mast_forest_builder.add_block(vec![Operation::Noop], None)? } else { mast_forest_builder.join_nodes(node_ids)? }) diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index e2d4885f2..f32accd4d 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -47,9 +47,9 @@ fn nested_blocks() -> Result<(), Report> { // `Assembler::with_kernel_from_module()`. let syscall_foo_node_id = { let kernel_foo_node_id = - expected_mast_forest_builder.ensure_block(vec![Operation::Add], None).unwrap(); + expected_mast_forest_builder.add_block(vec![Operation::Add], None).unwrap(); - expected_mast_forest_builder.ensure_syscall(kernel_foo_node_id).unwrap() + expected_mast_forest_builder.add_syscall(kernel_foo_node_id).unwrap() }; let program = r#" @@ -92,32 +92,32 @@ fn nested_blocks() -> Result<(), Report> { // basic block representing foo::bar.baz procedure let exec_foo_bar_baz_node_id = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(29_u32.into())], None) + .add_block(vec![Operation::Push(29_u32.into())], None) .unwrap(); let before = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(2u32.into())], None) + .add_block(vec![Operation::Push(2u32.into())], None) .unwrap(); let r#true1 = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(3u32.into())], None) + .add_block(vec![Operation::Push(3u32.into())], None) .unwrap(); let r#false1 = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(5u32.into())], None) + .add_block(vec![Operation::Push(5u32.into())], None) .unwrap(); - let r#if1 = expected_mast_forest_builder.ensure_split(r#true1, r#false1).unwrap(); + let r#if1 = expected_mast_forest_builder.add_split(r#true1, r#false1).unwrap(); let r#true3 = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(7u32.into())], None) + .add_block(vec![Operation::Push(7u32.into())], None) .unwrap(); let r#false3 = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(11u32.into())], None) + .add_block(vec![Operation::Push(11u32.into())], None) .unwrap(); - let r#true2 = expected_mast_forest_builder.ensure_split(r#true3, r#false3).unwrap(); + let r#true2 = expected_mast_forest_builder.add_split(r#true3, r#false3).unwrap(); let r#while = { let body_node_id = expected_mast_forest_builder - .ensure_block( + .add_block( vec![ Operation::Push(17u32.into()), Operation::Push(19u32.into()), @@ -127,16 +127,14 @@ fn nested_blocks() -> Result<(), Report> { ) .unwrap(); - expected_mast_forest_builder.ensure_loop(body_node_id).unwrap() + expected_mast_forest_builder.add_loop(body_node_id).unwrap() }; let push_13_basic_block_id = expected_mast_forest_builder - .ensure_block(vec![Operation::Push(13u32.into())], None) + .add_block(vec![Operation::Push(13u32.into())], None) .unwrap(); - let r#false2 = expected_mast_forest_builder - .ensure_join(push_13_basic_block_id, r#while) - .unwrap(); - let nested = expected_mast_forest_builder.ensure_split(r#true2, r#false2).unwrap(); + let r#false2 = expected_mast_forest_builder.add_join(push_13_basic_block_id, r#while).unwrap(); + let nested = expected_mast_forest_builder.add_split(r#true2, r#false2).unwrap(); let combined_node_id = expected_mast_forest_builder .join_nodes(vec![before, r#if1, nested, exec_foo_bar_baz_node_id, syscall_foo_node_id]) From 961d6f665a60238096ebaa935b558802ed93be4d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Wed, 28 Aug 2024 16:09:39 -0400 Subject: [PATCH 18/57] fix regression --- assembly/src/assembler/instruction/procedures.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 708cc8d78..8d462c34b 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -20,7 +20,21 @@ impl Assembler { let span = callee.span(); let node_id_or_digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; match node_id_or_digest { - Either::Left(node_id) => Ok(node_id), + // TODO(plafer): reconcile with `invoke_mast_root` impl + Either::Left(node_id) => match kind { + InvokeKind::ProcRef => Ok(node_id), + InvokeKind::Exec => { + // We make sure to copy the root node so that the `exec` is associated + // with a different `MastNodeId` than the procedure it is referencing. + // Currently the only purpose of this is so that simple procedures that + // only have an `exec` have a different body node id than the procedure + // they're executing. + let root_node = mast_forest_builder.get_mast_node(node_id).unwrap(); + Ok(mast_forest_builder.add_node(root_node.clone())?) + }, + InvokeKind::Call => Ok(mast_forest_builder.add_call(node_id)?), + InvokeKind::SysCall => Ok(mast_forest_builder.add_syscall(node_id)?), + }, Either::Right(digest) => self.invoke_mast_root(kind, span, digest, mast_forest_builder), } } From a4cb422704e1d6685f003845219503527892c8c1 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 09:17:54 -0400 Subject: [PATCH 19/57] fix failing test --- assembly/src/tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index 11794f8fe..9470c0c41 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1355,8 +1355,8 @@ fn ensure_correct_procedure_selection_on_collision() -> TestResult { (split_node.on_true(), split_node.on_false()) }; - assert_eq!(expected_f_node_id, f_node_id); - assert_eq!(expected_g_node_id, g_node_id); + assert_eq!(program.mast_forest()[expected_f_node_id], program.mast_forest()[f_node_id]); + assert_eq!(program.mast_forest()[expected_g_node_id], program.mast_forest()[g_node_id]); Ok(()) } From 0665cf5a2e71497d3d1de98e98122087f347bd1f Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 09:22:52 -0400 Subject: [PATCH 20/57] changelog --- CHANGELOG.md | 2 +- assembly/src/tests.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 48e51b72b..9bc60c164 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ #### Changes - [BREAKING] Wrapped `MastForest`s in `Program` and `Library` structs in `Arc` (#1465). - +- `MastForestBuilder`: use `MastNodeId` instead of MAST root to uniquely identify procedures (#1473) ## 0.10.5 (2024-08-21) diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index 9470c0c41..81b81a314 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1345,7 +1345,7 @@ fn ensure_correct_procedure_selection_on_collision() -> TestResult { let expected_g_node_id = MastNodeId::from_u32_safe(0_u32, program.mast_forest().as_ref()).unwrap(); - let (f_node_id, g_node_id) = { + let (exec_f_node_id, exec_g_node_id) = { let split_node_id = program.entrypoint(); let split_node = match &program.mast_forest()[split_node_id] { MastNode::Split(split_node) => split_node, @@ -1355,8 +1355,8 @@ fn ensure_correct_procedure_selection_on_collision() -> TestResult { (split_node.on_true(), split_node.on_false()) }; - assert_eq!(program.mast_forest()[expected_f_node_id], program.mast_forest()[f_node_id]); - assert_eq!(program.mast_forest()[expected_g_node_id], program.mast_forest()[g_node_id]); + assert_eq!(program.mast_forest()[expected_f_node_id], program.mast_forest()[exec_f_node_id]); + assert_eq!(program.mast_forest()[expected_g_node_id], program.mast_forest()[exec_g_node_id]); Ok(()) } From ef307c2c8dd9b621fd25f8c60141a45861b72887 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 09:58:10 -0400 Subject: [PATCH 21/57] docs --- assembly/src/assembler/mod.rs | 11 +++++++++++ assembly/src/library/mod.rs | 8 ++++++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 44fa3c504..b2455dd8e 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -202,6 +202,15 @@ impl Assembler { } /// Adds the compiled library to provide modules for the compilation. + /// + /// We only current support adding non-vendored libraries - that is, the source code of exported + /// procedures is not included in the program that compiles against the library. The library's + /// source code is instead expected to be loaded in the processor at execution time. Hence, all + /// calls to library procedures will be compiled down to a [`vm_core::mast::ExternalNode`] (i.e. + /// a reference to the procedure's MAST root). This means that when executing a program compiled + /// against a library, the processor will not be able to differentiate procedures with the same + /// MAST root but different decorators. Hence, it is not recommended to export two procedures + /// that have the same MAST root (i.e. are identical except for their decorators). pub fn add_library(&mut self, library: impl AsRef) -> Result<(), Report> { self.module_graph .add_compiled_modules(library.as_ref().module_infos()) @@ -210,6 +219,8 @@ impl Assembler { } /// Adds the compiled library to provide modules for the compilation. + /// + /// See [`Self::add_library`] for more detailed information. pub fn with_library(mut self, library: impl AsRef) -> Result { self.add_library(library)?; Ok(self) diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 925275e44..94b49b671 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -45,8 +45,12 @@ pub struct Library { /// lexicographical order (by digest, not procedure name) digest: RpoDigest, /// A map between procedure paths and the corresponding procedure roots in the MAST forest. - /// Multiple paths can map to the same root, and also, some roots may not be associated with - /// any paths. + /// Multiple paths can map to the same root, and also, some roots may not be associated with any + /// paths. + /// + /// Note that we use `MastNodeId` as a unique identifier for procedures instead of MAST root, + /// since 2 different procedures with the same MAST root can be different due to the decorators + /// they contain. exports: BTreeMap, /// The MAST forest underlying this library. mast_forest: Arc, From cccf0184496919f86086988a5d061cb3e949464d Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 10:06:41 -0400 Subject: [PATCH 22/57] docs --- assembly/src/assembler/mast_forest_builder.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 0f81cf19b..f91e26327 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -38,10 +38,8 @@ pub struct MastForestBuilder { /// with the same digest are added to the MAST forest builder, only the first procedure is /// added to the map, and all subsequent insertions are ignored. procedures: BTreeMap, - /// A map from procedure MAST root to its global procedure index. Similar to the `procedures` - /// map, this map contains only the first inserted procedure for procedures with the same MAST - /// root. - /// TODO(plafer): fix docs + /// A map from procedure body node id to its global procedure index. We guarantee that unique + /// procedures have a unique body node id. proc_gid_by_node_id: BTreeMap, /// A map from procedure MAST root to its global procedure index. Similar to the `procedures` /// map, this map contains only the first inserted procedure for procedures with the same MAST From 883356a408838c9455e1a94a64da019627c35bf3 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 10:24:50 -0400 Subject: [PATCH 23/57] add test `ensure_unique_node_ids_for_identical_procedures` --- assembly/src/tests.rs | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index 81b81a314..fa8ed601e 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1309,6 +1309,44 @@ end"; // PROGRAMS WITH PROCEDURES // ================================================================================================ +/// This ensures the invariant that no two procedures can have the same root node ID, even if the +/// procedures are identical. +#[test] +fn ensure_unique_node_ids_for_identical_procedures() -> TestResult { + let context = TestContext::default(); + + // if with else + let source = source_file!( + &context, + " + proc.f + add + end + + proc.g + add + end + + begin + call.f + call.g + end" + ); + let program = context.assemble(source)?; + + // f, g and entrypoint + assert_eq!(program.num_procedures(), 3); + + let root_id_1 = program.mast_forest().procedure_roots()[0]; + let root_id_2 = program.mast_forest().procedure_roots()[1]; + let root_id_3 = program.mast_forest().procedure_roots()[2]; + + assert_ne!(root_id_1, root_id_2); + assert_ne!(root_id_2, root_id_3); + + Ok(()) +} + /// If the program has 2 procedures with the same MAST root (but possibly different decorators), the /// correct procedure is chosen on exec #[test] From ecec03b16ea6295f8824e455a514cab6fbedc668 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 10:33:23 -0400 Subject: [PATCH 24/57] cleanup `MastForestBuilder` --- assembly/src/assembler/mast_forest_builder.rs | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index f91e26327..cc62a6d03 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -38,9 +38,6 @@ pub struct MastForestBuilder { /// with the same digest are added to the MAST forest builder, only the first procedure is /// added to the map, and all subsequent insertions are ignored. procedures: BTreeMap, - /// A map from procedure body node id to its global procedure index. We guarantee that unique - /// procedures have a unique body node id. - proc_gid_by_node_id: BTreeMap, /// A map from procedure MAST root to its global procedure index. Similar to the `procedures` /// map, this map contains only the first inserted procedure for procedures with the same MAST /// root. @@ -130,13 +127,6 @@ impl MastForestBuilder { self.procedures.get(&gid) } - /// Returns a reference to the procedure with the specified [`MastNodeId`], or None - /// if such a procedure is not present in this MAST forest builder. - #[inline(always)] - pub fn find_procedure_by_id(&self, node_id: MastNodeId) -> Option<&Procedure> { - self.proc_gid_by_node_id.get(&node_id).and_then(|gid| self.get_procedure(*gid)) - } - /// Returns a reference to the procedure with the specified MAST root, or None /// if such a procedure is not present in this MAST forest builder. #[inline(always)] @@ -187,7 +177,7 @@ impl MastForestBuilder { // We don't have a cache entry yet, but we do want to make sure we don't have a conflicting // cache entry with the same MAST root: - if let Some(cached) = self.find_procedure_by_id(procedure.body_node_id()) { + if let Some(cached) = self.find_procedure_by_digest(&procedure.mast_root()) { // Handle the case where a procedure with no locals is lowered to a MastForest // consisting only of an `External` node to another procedure which has one or more // locals. This will result in the calling procedure having the same digest as the @@ -209,7 +199,6 @@ impl MastForestBuilder { } self.mast_forest.make_root(procedure.body_node_id()); - self.proc_gid_by_node_id.insert(procedure.body_node_id(), gid); self.proc_gid_by_digest.insert(procedure.mast_root(), gid); self.procedures.insert(gid, procedure); From aa04649ea39c236fd811880f5dbc2865c2257a74 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 10:44:06 -0400 Subject: [PATCH 25/57] remove `ensure_unique_node_ids_for_identical_procedures` test --- assembly/src/tests.rs | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/assembly/src/tests.rs b/assembly/src/tests.rs index fa8ed601e..81b81a314 100644 --- a/assembly/src/tests.rs +++ b/assembly/src/tests.rs @@ -1309,44 +1309,6 @@ end"; // PROGRAMS WITH PROCEDURES // ================================================================================================ -/// This ensures the invariant that no two procedures can have the same root node ID, even if the -/// procedures are identical. -#[test] -fn ensure_unique_node_ids_for_identical_procedures() -> TestResult { - let context = TestContext::default(); - - // if with else - let source = source_file!( - &context, - " - proc.f - add - end - - proc.g - add - end - - begin - call.f - call.g - end" - ); - let program = context.assemble(source)?; - - // f, g and entrypoint - assert_eq!(program.num_procedures(), 3); - - let root_id_1 = program.mast_forest().procedure_roots()[0]; - let root_id_2 = program.mast_forest().procedure_roots()[1]; - let root_id_3 = program.mast_forest().procedure_roots()[2]; - - assert_ne!(root_id_1, root_id_2); - assert_ne!(root_id_2, root_id_3); - - Ok(()) -} - /// If the program has 2 procedures with the same MAST root (but possibly different decorators), the /// correct procedure is chosen on exec #[test] From 72a482b0113fa97c8a55e00b80ba4c3321fedb38 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 11:25:36 -0400 Subject: [PATCH 26/57] `MastForestBuilder::eq_hash()` --- assembly/src/assembler/mod.rs | 4 +-- assembly/src/library/mod.rs | 6 ++-- core/src/mast/node/mod.rs | 56 +++++++++++++++++++++++++++++++++-- 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index b2455dd8e..8726e91b2 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -202,7 +202,7 @@ impl Assembler { } /// Adds the compiled library to provide modules for the compilation. - /// + /// /// We only current support adding non-vendored libraries - that is, the source code of exported /// procedures is not included in the program that compiles against the library. The library's /// source code is instead expected to be loaded in the processor at execution time. Hence, all @@ -219,7 +219,7 @@ impl Assembler { } /// Adds the compiled library to provide modules for the compilation. - /// + /// /// See [`Self::add_library`] for more detailed information. pub fn with_library(mut self, library: impl AsRef) -> Result { self.add_library(library)?; diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 94b49b671..2df85adba 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -45,9 +45,9 @@ pub struct Library { /// lexicographical order (by digest, not procedure name) digest: RpoDigest, /// A map between procedure paths and the corresponding procedure roots in the MAST forest. - /// Multiple paths can map to the same root, and also, some roots may not be associated with any - /// paths. - /// + /// Multiple paths can map to the same root, and also, some roots may not be associated with + /// any paths. + /// /// Note that we use `MastNodeId` as a unique identifier for procedures instead of MAST root, /// since 2 different procedures with the same MAST root can be different due to the decorators /// they contain. diff --git a/core/src/mast/node/mod.rs b/core/src/mast/node/mod.rs index 63293a333..bbe1fe75e 100644 --- a/core/src/mast/node/mod.rs +++ b/core/src/mast/node/mod.rs @@ -1,11 +1,13 @@ mod basic_block_node; use alloc::{boxed::Box, vec::Vec}; use core::fmt; +use std::string::ToString; pub use basic_block_node::{ BasicBlockNode, OpBatch, OperationOrDecorator, BATCH_SIZE as OP_BATCH_SIZE, GROUP_SIZE as OP_GROUP_SIZE, }; +use num_traits::ToBytes; mod call_node; pub use call_node::CallNode; @@ -20,7 +22,13 @@ mod join_node; pub use join_node::JoinNode; mod split_node; -use miden_crypto::{hash::rpo::RpoDigest, Felt}; +use miden_crypto::{ + hash::{ + blake::{Blake3Digest, Blake3_256}, + rpo::RpoDigest, + }, + Felt, +}; use miden_formatting::prettier::{Document, PrettyPrint}; pub use split_node::SplitNode; @@ -30,7 +38,7 @@ pub use loop_node::LoopNode; use super::MastForestError; use crate::{ mast::{MastForest, MastNodeId}, - DecoratorList, Operation, + Decorator, DecoratorList, Operation, }; // MAST NODE @@ -187,6 +195,50 @@ impl MastNode { MastNode::External(node) => MastNodeDisplay::new(node.to_display(mast_forest)), } } + + /// Returns the Blake3 hash of this node, to be used for equality testing. + /// + /// Specifically, two nodes with the same MAST root but different decorators will have a + /// different hash. + pub fn eq_hash(&self) -> Blake3Digest<32> { + match self { + MastNode::Block(node) => { + let mut bytes_to_hash = node.digest().as_bytes().to_vec(); + + for (idx, decorator) in node.decorators() { + bytes_to_hash.extend(idx.to_le_bytes()); + + match decorator { + Decorator::Advice(advice) => { + bytes_to_hash.extend(advice.to_string().as_bytes()) + }, + Decorator::AsmOp(asm_op) => { + bytes_to_hash.extend(asm_op.context_name().as_bytes()); + bytes_to_hash.extend(asm_op.op().as_bytes()); + bytes_to_hash.push(asm_op.num_cycles()); + }, + Decorator::Debug(debug) => { + bytes_to_hash.extend(debug.to_string().as_bytes()) + }, + Decorator::Event(event) => { + bytes_to_hash.extend(event.to_le_bytes()); + }, + Decorator::Trace(trace) => { + bytes_to_hash.extend(trace.to_le_bytes()); + }, + } + } + + Blake3_256::hash(&bytes_to_hash) + }, + MastNode::Join(node) => Blake3_256::hash(&node.digest().as_bytes()), + MastNode::Split(node) => Blake3_256::hash(&node.digest().as_bytes()), + MastNode::Loop(node) => Blake3_256::hash(&node.digest().as_bytes()), + MastNode::Call(node) => Blake3_256::hash(&node.digest().as_bytes()), + MastNode::Dyn => Blake3_256::hash(&MastNode::Dyn.digest().as_bytes()), + MastNode::External(node) => Blake3_256::hash(&node.digest().as_bytes()), + } + } } // PRETTY PRINTING From e18c4e7fb9dafd2b6818d65c904c4569fa4fc1b5 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 11:26:53 -0400 Subject: [PATCH 27/57] `MastForestBuilder`: digest -> mast_root --- assembly/src/assembler/instruction/procedures.rs | 2 +- assembly/src/assembler/mast_forest_builder.rs | 16 +++++++++------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 8d462c34b..643ea4b6a 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -50,7 +50,7 @@ impl Assembler { let current_source_file = self.source_manager.get(span.source_id()).ok(); // If the procedure is cached and is a system call, ensure that the call is valid. - match mast_forest_builder.find_procedure_by_digest(&mast_root) { + match mast_forest_builder.find_procedure_by_mast_root(&mast_root) { Some(proc) if matches!(kind, InvokeKind::SysCall) => { // Verify if this is a syscall, that the callee is a kernel procedure // diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index cc62a6d03..fd264823b 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -41,7 +41,7 @@ pub struct MastForestBuilder { /// A map from procedure MAST root to its global procedure index. Similar to the `procedures` /// map, this map contains only the first inserted procedure for procedures with the same MAST /// root. - proc_gid_by_digest: BTreeMap, + proc_gid_by_mast_root: BTreeMap, /// A set of IDs for basic blocks which have been merged into a bigger basic blocks. This is /// used as a candidate set of nodes that may be eliminated if the are not referenced by any /// other node in the forest and are not a root of any procedure. @@ -130,15 +130,17 @@ impl MastForestBuilder { /// Returns a reference to the procedure with the specified MAST root, or None /// if such a procedure is not present in this MAST forest builder. #[inline(always)] - pub fn find_procedure_by_digest(&self, digest: &RpoDigest) -> Option<&Procedure> { - self.proc_gid_by_digest.get(digest).and_then(|gid| self.get_procedure(*gid)) + pub fn find_procedure_by_mast_root(&self, mast_root: &RpoDigest) -> Option<&Procedure> { + self.proc_gid_by_mast_root + .get(mast_root) + .and_then(|gid| self.get_procedure(*gid)) } /// Returns the [`MastNodeId`] of the procedure associated with a given MAST root, or None /// if such a procedure is not present in this MAST forest builder. #[inline(always)] - pub fn find_procedure_node_id(&self, digest: RpoDigest) -> Option { - self.mast_forest.find_procedure_root(digest) + pub fn find_procedure_node_id(&self, mast_root: RpoDigest) -> Option { + self.mast_forest.find_procedure_root(mast_root) } /// Returns the [`MastNode`] for the provided MAST node ID, or None if a node with this ID is @@ -177,7 +179,7 @@ impl MastForestBuilder { // We don't have a cache entry yet, but we do want to make sure we don't have a conflicting // cache entry with the same MAST root: - if let Some(cached) = self.find_procedure_by_digest(&procedure.mast_root()) { + if let Some(cached) = self.find_procedure_by_mast_root(&procedure.mast_root()) { // Handle the case where a procedure with no locals is lowered to a MastForest // consisting only of an `External` node to another procedure which has one or more // locals. This will result in the calling procedure having the same digest as the @@ -199,7 +201,7 @@ impl MastForestBuilder { } self.mast_forest.make_root(procedure.body_node_id()); - self.proc_gid_by_digest.insert(procedure.mast_root(), gid); + self.proc_gid_by_mast_root.insert(procedure.mast_root(), gid); self.procedures.insert(gid, procedure); Ok(()) From 4a46d3245bb3586212b23f0a8b4f72b6f1d46b7a Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 12:31:23 -0400 Subject: [PATCH 28/57] `MastForestBuilder`: don't duplicate equal nodes --- assembly/src/assembler/basic_block_builder.rs | 2 +- .../src/assembler/instruction/procedures.rs | 26 ++++----- assembly/src/assembler/mast_forest_builder.rs | 56 +++++++++++-------- assembly/src/assembler/mod.rs | 8 +-- assembly/src/assembler/tests.rs | 32 ++++++----- 5 files changed, 69 insertions(+), 55 deletions(-) diff --git a/assembly/src/assembler/basic_block_builder.rs b/assembly/src/assembler/basic_block_builder.rs index c535115a9..5a6a05cab 100644 --- a/assembly/src/assembler/basic_block_builder.rs +++ b/assembly/src/assembler/basic_block_builder.rs @@ -139,7 +139,7 @@ impl BasicBlockBuilder { let ops = self.ops.drain(..).collect(); let decorators = self.decorators.drain(..).collect(); - let basic_block_node_id = mast_forest_builder.add_block(ops, Some(decorators))?; + let basic_block_node_id = mast_forest_builder.ensure_block(ops, Some(decorators))?; Ok(Some(basic_block_node_id)) } else if !self.decorators.is_empty() { diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 643ea4b6a..39e3aaba9 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -30,10 +30,10 @@ impl Assembler { // only have an `exec` have a different body node id than the procedure // they're executing. let root_node = mast_forest_builder.get_mast_node(node_id).unwrap(); - Ok(mast_forest_builder.add_node(root_node.clone())?) + Ok(mast_forest_builder.ensure_node(root_node.clone())?) }, - InvokeKind::Call => Ok(mast_forest_builder.add_call(node_id)?), - InvokeKind::SysCall => Ok(mast_forest_builder.add_syscall(node_id)?), + InvokeKind::Call => Ok(mast_forest_builder.ensure_call(node_id)?), + InvokeKind::SysCall => Ok(mast_forest_builder.ensure_syscall(node_id)?), }, Either::Right(digest) => self.invoke_mast_root(kind, span, digest, mast_forest_builder), } @@ -102,7 +102,7 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.add_external(mast_root)? + mast_forest_builder.ensure_external(mast_root)? }, } }, @@ -115,12 +115,12 @@ impl Assembler { // only have an `exec` have a different body node id than the procedure // they're executing. let root_node = mast_forest_builder.get_mast_node(root).unwrap(); - mast_forest_builder.add_node(root_node.clone())? + mast_forest_builder.ensure_node(root_node.clone())? }, None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.add_external(mast_root)? + mast_forest_builder.ensure_external(mast_root)? }, } }, @@ -130,11 +130,11 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.add_external(mast_root)? + mast_forest_builder.ensure_external(mast_root)? }, }; - mast_forest_builder.add_call(callee_id)? + mast_forest_builder.ensure_call(callee_id)? }, InvokeKind::SysCall => { let callee_id = match mast_forest_builder.find_procedure_node_id(mast_root) { @@ -142,11 +142,11 @@ impl Assembler { None => { // If the MAST root called isn't known to us, make it an external // reference. - mast_forest_builder.add_external(mast_root)? + mast_forest_builder.ensure_external(mast_root)? }, }; - mast_forest_builder.add_syscall(callee_id)? + mast_forest_builder.ensure_syscall(callee_id)? }, } }; @@ -159,7 +159,7 @@ impl Assembler { &self, mast_forest_builder: &mut MastForestBuilder, ) -> Result, AssemblyError> { - let dyn_node_id = mast_forest_builder.add_dyn()?; + let dyn_node_id = mast_forest_builder.ensure_dyn()?; Ok(Some(dyn_node_id)) } @@ -170,8 +170,8 @@ impl Assembler { mast_forest_builder: &mut MastForestBuilder, ) -> Result, AssemblyError> { let dyn_call_node_id = { - let dyn_node_id = mast_forest_builder.add_dyn()?; - mast_forest_builder.add_call(dyn_node_id)? + let dyn_node_id = mast_forest_builder.ensure_dyn()?; + mast_forest_builder.ensure_call(dyn_node_id)? }; Ok(Some(dyn_call_node_id)) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index fd264823b..9a8054680 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -4,7 +4,7 @@ use alloc::{ }; use vm_core::{ - crypto::hash::RpoDigest, + crypto::hash::{Blake3Digest, RpoDigest}, mast::{MastForest, MastNode, MastNodeId}, DecoratorList, Operation, }; @@ -42,6 +42,8 @@ pub struct MastForestBuilder { /// map, this map contains only the first inserted procedure for procedures with the same MAST /// root. proc_gid_by_mast_root: BTreeMap, + /// A map of MAST node hashes to their corresponding positions in the MAST forest. + node_id_by_hash: BTreeMap, MastNodeId>, /// A set of IDs for basic blocks which have been merged into a bigger basic blocks. This is /// used as a candidate set of nodes that may be eliminated if the are not referenced by any /// other node in the forest and are not a root of any procedure. @@ -228,7 +230,7 @@ impl MastForestBuilder { while let (Some(left), Some(right)) = (source_mast_node_iter.next(), source_mast_node_iter.next()) { - let join_mast_node_id = self.add_join(left, right)?; + let join_mast_node_id = self.ensure_join(left, right)?; node_ids.push(join_mast_node_id); } @@ -310,7 +312,7 @@ impl MastForestBuilder { let block_ops = core::mem::take(&mut operations); let block_decorators = core::mem::take(&mut decorators); let merged_basic_block_id = - self.add_block(block_ops, Some(block_decorators))?; + self.ensure_block(block_ops, Some(block_decorators))?; merged_basic_blocks.push(merged_basic_block_id); } @@ -322,7 +324,7 @@ impl MastForestBuilder { self.merged_basic_block_ids.extend(contiguous_basic_block_ids.iter()); if !operations.is_empty() || !decorators.is_empty() { - let merged_basic_block = self.add_block(operations, Some(decorators))?; + let merged_basic_block = self.ensure_block(operations, Some(decorators))?; merged_basic_blocks.push(merged_basic_block); } @@ -337,66 +339,76 @@ impl MastForestBuilder { /// /// Note adding the same [`MastNode`] twice will result in two different [`MastNodeId`]s being /// returned. - pub fn add_node(&mut self, node: MastNode) -> Result { - Ok(self.mast_forest.add_node(node)?) + pub fn ensure_node(&mut self, node: MastNode) -> Result { + let node_hash = node.eq_hash(); + + if let Some(node_id) = self.node_id_by_hash.get(&node_hash) { + // node already exists in the forest; return previously assigned id + Ok(*node_id) + } else { + let new_node_id = self.mast_forest.add_node(node)?; + self.node_id_by_hash.insert(node_hash, new_node_id); + + Ok(new_node_id) + } } /// Adds a basic block node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_block( + pub fn ensure_block( &mut self, operations: Vec, decorators: Option, ) -> Result { let block = MastNode::new_basic_block(operations, decorators)?; - self.add_node(block) + self.ensure_node(block) } /// Adds a join node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_join( + pub fn ensure_join( &mut self, left_child: MastNodeId, right_child: MastNodeId, ) -> Result { let join = MastNode::new_join(left_child, right_child, &self.mast_forest)?; - self.add_node(join) + self.ensure_node(join) } /// Adds a split node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_split( + pub fn ensure_split( &mut self, if_branch: MastNodeId, else_branch: MastNodeId, ) -> Result { let split = MastNode::new_split(if_branch, else_branch, &self.mast_forest)?; - self.add_node(split) + self.ensure_node(split) } /// Adds a loop node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_loop(&mut self, body: MastNodeId) -> Result { + pub fn ensure_loop(&mut self, body: MastNodeId) -> Result { let loop_node = MastNode::new_loop(body, &self.mast_forest)?; - self.add_node(loop_node) + self.ensure_node(loop_node) } /// Adds a call node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_call(&mut self, callee: MastNodeId) -> Result { + pub fn ensure_call(&mut self, callee: MastNodeId) -> Result { let call = MastNode::new_call(callee, &self.mast_forest)?; - self.add_node(call) + self.ensure_node(call) } /// Adds a syscall node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_syscall(&mut self, callee: MastNodeId) -> Result { + pub fn ensure_syscall(&mut self, callee: MastNodeId) -> Result { let syscall = MastNode::new_syscall(callee, &self.mast_forest)?; - self.add_node(syscall) + self.ensure_node(syscall) } /// Adds a dyn node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_dyn(&mut self) -> Result { - self.add_node(MastNode::new_dyn()) + pub fn ensure_dyn(&mut self) -> Result { + self.ensure_node(MastNode::new_dyn()) } /// Adds an external node to the forest, and returns the [`MastNodeId`] associated with it. - pub fn add_external(&mut self, mast_root: RpoDigest) -> Result { - self.add_node(MastNode::new_external(mast_root)) + pub fn ensure_external(&mut self, mast_root: RpoDigest) -> Result { + self.ensure_node(MastNode::new_external(mast_root)) } } diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 8726e91b2..480ea9768 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -539,7 +539,7 @@ impl Assembler { // insert external node into the MAST forest for this procedure; if a procedure // with the same MAST rood had been previously added to the builder, this will // have no effect - let proc_node_id = mast_forest_builder.add_external(proc_mast_root)?; + let proc_node_id = mast_forest_builder.ensure_external(proc_mast_root)?; let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); // Make the MAST root available to all dependents @@ -631,7 +631,7 @@ impl Assembler { let else_blk = self.compile_body(else_blk.iter(), proc_ctx, None, mast_forest_builder)?; - let split_node_id = mast_forest_builder.add_split(then_blk, else_blk)?; + let split_node_id = mast_forest_builder.ensure_split(then_blk, else_blk)?; node_ids.push(split_node_id); }, @@ -660,7 +660,7 @@ impl Assembler { let loop_body_node_id = self.compile_body(body.iter(), proc_ctx, None, mast_forest_builder)?; - let loop_node_id = mast_forest_builder.add_loop(loop_body_node_id)?; + let loop_node_id = mast_forest_builder.ensure_loop(loop_body_node_id)?; node_ids.push(loop_node_id); }, } @@ -673,7 +673,7 @@ impl Assembler { } Ok(if node_ids.is_empty() { - mast_forest_builder.add_block(vec![Operation::Noop], None)? + mast_forest_builder.ensure_block(vec![Operation::Noop], None)? } else { mast_forest_builder.join_nodes(node_ids)? }) diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index f32accd4d..e2d4885f2 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -47,9 +47,9 @@ fn nested_blocks() -> Result<(), Report> { // `Assembler::with_kernel_from_module()`. let syscall_foo_node_id = { let kernel_foo_node_id = - expected_mast_forest_builder.add_block(vec![Operation::Add], None).unwrap(); + expected_mast_forest_builder.ensure_block(vec![Operation::Add], None).unwrap(); - expected_mast_forest_builder.add_syscall(kernel_foo_node_id).unwrap() + expected_mast_forest_builder.ensure_syscall(kernel_foo_node_id).unwrap() }; let program = r#" @@ -92,32 +92,32 @@ fn nested_blocks() -> Result<(), Report> { // basic block representing foo::bar.baz procedure let exec_foo_bar_baz_node_id = expected_mast_forest_builder - .add_block(vec![Operation::Push(29_u32.into())], None) + .ensure_block(vec![Operation::Push(29_u32.into())], None) .unwrap(); let before = expected_mast_forest_builder - .add_block(vec![Operation::Push(2u32.into())], None) + .ensure_block(vec![Operation::Push(2u32.into())], None) .unwrap(); let r#true1 = expected_mast_forest_builder - .add_block(vec![Operation::Push(3u32.into())], None) + .ensure_block(vec![Operation::Push(3u32.into())], None) .unwrap(); let r#false1 = expected_mast_forest_builder - .add_block(vec![Operation::Push(5u32.into())], None) + .ensure_block(vec![Operation::Push(5u32.into())], None) .unwrap(); - let r#if1 = expected_mast_forest_builder.add_split(r#true1, r#false1).unwrap(); + let r#if1 = expected_mast_forest_builder.ensure_split(r#true1, r#false1).unwrap(); let r#true3 = expected_mast_forest_builder - .add_block(vec![Operation::Push(7u32.into())], None) + .ensure_block(vec![Operation::Push(7u32.into())], None) .unwrap(); let r#false3 = expected_mast_forest_builder - .add_block(vec![Operation::Push(11u32.into())], None) + .ensure_block(vec![Operation::Push(11u32.into())], None) .unwrap(); - let r#true2 = expected_mast_forest_builder.add_split(r#true3, r#false3).unwrap(); + let r#true2 = expected_mast_forest_builder.ensure_split(r#true3, r#false3).unwrap(); let r#while = { let body_node_id = expected_mast_forest_builder - .add_block( + .ensure_block( vec![ Operation::Push(17u32.into()), Operation::Push(19u32.into()), @@ -127,14 +127,16 @@ fn nested_blocks() -> Result<(), Report> { ) .unwrap(); - expected_mast_forest_builder.add_loop(body_node_id).unwrap() + expected_mast_forest_builder.ensure_loop(body_node_id).unwrap() }; let push_13_basic_block_id = expected_mast_forest_builder - .add_block(vec![Operation::Push(13u32.into())], None) + .ensure_block(vec![Operation::Push(13u32.into())], None) .unwrap(); - let r#false2 = expected_mast_forest_builder.add_join(push_13_basic_block_id, r#while).unwrap(); - let nested = expected_mast_forest_builder.add_split(r#true2, r#false2).unwrap(); + let r#false2 = expected_mast_forest_builder + .ensure_join(push_13_basic_block_id, r#while) + .unwrap(); + let nested = expected_mast_forest_builder.ensure_split(r#true2, r#false2).unwrap(); let combined_node_id = expected_mast_forest_builder .join_nodes(vec![before, r#if1, nested, exec_foo_bar_baz_node_id, syscall_foo_node_id]) From d959dd6e2b03f3c6d9780ca64105356d6b0b3ef0 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 12:44:13 -0400 Subject: [PATCH 29/57] revert changes to tests --- assembly/src/library/tests.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index 27d120957..833bca739 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -98,14 +98,18 @@ fn library_exports() -> Result<(), Report> { let actual_exports: BTreeSet<_> = lib2.exports().collect(); assert_eq!(expected_exports, actual_exports); - // make sure there are 8 roots in the MAST (foo1, foo2, foo3, bar1, bar2, bar3, bar4, and bar5) - assert_eq!(lib2.mast_forest.num_procedures(), 8); + // make sure foo2, bar2, and bar3 map to the same MastNode + assert_eq!(lib2.get_export_node_id(&foo2), lib2.get_export_node_id(&bar2)); + assert_eq!(lib2.get_export_node_id(&foo2), lib2.get_export_node_id(&bar3)); - // bar1 and bar2 should be the only re-exports + // make sure there are 6 roots in the MAST (foo1, foo2, foo3, bar1, bar4, and bar5) + assert_eq!(lib2.mast_forest.num_procedures(), 6); + + // bar1 should be the only re-export (i.e. the only procedure re-exported from a dependency) assert!(!lib2.is_reexport(&foo2)); assert!(!lib2.is_reexport(&foo3)); assert!(lib2.is_reexport(&bar1)); - assert!(lib2.is_reexport(&bar2)); + assert!(!lib2.is_reexport(&bar2)); assert!(!lib2.is_reexport(&bar3)); assert!(!lib2.is_reexport(&bar5)); @@ -156,9 +160,14 @@ fn library_procedure_collision() -> Result<(), Report> { // make sure lib2 has the expected exports (i.e., bar1 and bar2) assert_eq!(lib2.num_exports(), 2); - // make sure that bar1 and bar2 are different nodes in the MAST forest (since they could differ - // in their use of decorators) - assert_ne!(lib2.get_export_node_id(&lib2_bar_bar1), lib2.get_export_node_id(&lib2_bar_bar2)); + // make sure that bar1 and bar2 are equal nodes in the MAST forest + assert_eq!(lib2.get_export_node_id(&lib2_bar_bar1), lib2.get_export_node_id(&lib2_bar_bar2)); + + // make sure only one node was added to the forest + // NOTE: the MAST forest should actually have only 1 node (external node for the re-exported + // procedure), because nodes for the local procedure nodes should be pruned from the forest, + // but this is not implemented yet + assert_eq!(lib2.mast_forest().num_nodes(), 5); Ok(()) } From 187db6b76e207c5473f00ca95f49cd00e34c8ec9 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 12:50:34 -0400 Subject: [PATCH 30/57] invoke: don't create new node needlessly --- assembly/src/assembler/instruction/procedures.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 39e3aaba9..0583ac0bf 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -22,16 +22,7 @@ impl Assembler { match node_id_or_digest { // TODO(plafer): reconcile with `invoke_mast_root` impl Either::Left(node_id) => match kind { - InvokeKind::ProcRef => Ok(node_id), - InvokeKind::Exec => { - // We make sure to copy the root node so that the `exec` is associated - // with a different `MastNodeId` than the procedure it is referencing. - // Currently the only purpose of this is so that simple procedures that - // only have an `exec` have a different body node id than the procedure - // they're executing. - let root_node = mast_forest_builder.get_mast_node(node_id).unwrap(); - Ok(mast_forest_builder.ensure_node(root_node.clone())?) - }, + InvokeKind::ProcRef | InvokeKind::Exec => Ok(node_id), InvokeKind::Call => Ok(mast_forest_builder.ensure_call(node_id)?), InvokeKind::SysCall => Ok(mast_forest_builder.ensure_syscall(node_id)?), }, From 53003b5724ea383684651a84e64ad40566fd0179 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 12:55:22 -0400 Subject: [PATCH 31/57] fix test --- assembly/src/assembler/tests.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index e2d4885f2..90e497abf 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -202,29 +202,23 @@ fn duplicate_nodes() { let mut expected_mast_forest = MastForest::new(); // basic block: mul - let first_mul_basic_block_id = - expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); + let mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); // basic block: add and mul let add_basic_block_id = expected_mast_forest.add_block(vec![Operation::Add], None).unwrap(); - let second_mul_basic_block_id = - expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); // inner split: `if.true add else mul end` - let inner_split_id = expected_mast_forest - .add_split(add_basic_block_id, second_mul_basic_block_id) - .unwrap(); + let inner_split_id = + expected_mast_forest.add_split(add_basic_block_id, mul_basic_block_id).unwrap(); // root: outer split - let root_id = expected_mast_forest - .add_split(first_mul_basic_block_id, inner_split_id) - .unwrap(); + let root_id = expected_mast_forest.add_split(mul_basic_block_id, inner_split_id).unwrap(); expected_mast_forest.make_root(root_id); let expected_program = Program::new(expected_mast_forest.into(), root_id); - assert_eq!(program, expected_program); + assert_eq!(expected_program, program); } #[test] From 126931ef40edaa276217bf53b9b8d632f6f3c81b Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 12:57:26 -0400 Subject: [PATCH 32/57] fix docs --- assembly/src/library/mod.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 2df85adba..fd03d6341 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -45,12 +45,13 @@ pub struct Library { /// lexicographical order (by digest, not procedure name) digest: RpoDigest, /// A map between procedure paths and the corresponding procedure roots in the MAST forest. - /// Multiple paths can map to the same root, and also, some roots may not be associated with - /// any paths. + /// Multiple paths can map to the same root, and also, some roots may not be associated with any + /// paths. /// - /// Note that we use `MastNodeId` as a unique identifier for procedures instead of MAST root, - /// since 2 different procedures with the same MAST root can be different due to the decorators - /// they contain. + /// Note that we use `MastNodeId` as an identifier for procedures instead of MAST root, since 2 + /// different procedures with the same MAST root can be different due to the decorators they + /// contain. However, note that `MastNodeId` is also not a unique identifier for procedures; if + /// the procedures have the same MAST root and decorators, they will have the same `MastNodeId`. exports: BTreeMap, /// The MAST forest underlying this library. mast_forest: Arc, From 45539ed3c200a98b142d40ff6f577e0615c9eaeb Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 12:58:17 -0400 Subject: [PATCH 33/57] fmt --- assembly/src/library/mod.rs | 7 ++++--- assembly/src/library/tests.rs | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index fd03d6341..21caa908b 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -45,13 +45,14 @@ pub struct Library { /// lexicographical order (by digest, not procedure name) digest: RpoDigest, /// A map between procedure paths and the corresponding procedure roots in the MAST forest. - /// Multiple paths can map to the same root, and also, some roots may not be associated with any - /// paths. + /// Multiple paths can map to the same root, and also, some roots may not be associated with + /// any paths. /// /// Note that we use `MastNodeId` as an identifier for procedures instead of MAST root, since 2 /// different procedures with the same MAST root can be different due to the decorators they /// contain. However, note that `MastNodeId` is also not a unique identifier for procedures; if - /// the procedures have the same MAST root and decorators, they will have the same `MastNodeId`. + /// the procedures have the same MAST root and decorators, they will have the same + /// `MastNodeId`. exports: BTreeMap, /// The MAST forest underlying this library. mast_forest: Arc, diff --git a/assembly/src/library/tests.rs b/assembly/src/library/tests.rs index 833bca739..d9e39a819 100644 --- a/assembly/src/library/tests.rs +++ b/assembly/src/library/tests.rs @@ -160,7 +160,7 @@ fn library_procedure_collision() -> Result<(), Report> { // make sure lib2 has the expected exports (i.e., bar1 and bar2) assert_eq!(lib2.num_exports(), 2); - // make sure that bar1 and bar2 are equal nodes in the MAST forest + // make sure that bar1 and bar2 are equal nodes in the MAST forest assert_eq!(lib2.get_export_node_id(&lib2_bar_bar1), lib2.get_export_node_id(&lib2_bar_bar2)); // make sure only one node was added to the forest From ded33b475d2abaff613c075155973915a47be81b Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 13:28:08 -0400 Subject: [PATCH 34/57] cleanup `Assembler::invoke` --- .../src/assembler/instruction/procedures.rs | 93 +++++-------------- 1 file changed, 25 insertions(+), 68 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 0583ac0bf..edc4172db 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -19,18 +19,23 @@ impl Assembler { ) -> Result { let span = callee.span(); let node_id_or_digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; - match node_id_or_digest { - // TODO(plafer): reconcile with `invoke_mast_root` impl - Either::Left(node_id) => match kind { - InvokeKind::ProcRef | InvokeKind::Exec => Ok(node_id), - InvokeKind::Call => Ok(mast_forest_builder.ensure_call(node_id)?), - InvokeKind::SysCall => Ok(mast_forest_builder.ensure_syscall(node_id)?), + let invoked_proc_node_id = match node_id_or_digest { + Either::Left(node_id) => node_id, + Either::Right(mast_root) => { + self.get_proc_root_id_from_mast_root(kind, span, mast_root, mast_forest_builder)? }, - Either::Right(digest) => self.invoke_mast_root(kind, span, digest, mast_forest_builder), + }; + + match kind { + InvokeKind::ProcRef | InvokeKind::Exec => Ok(invoked_proc_node_id), + InvokeKind::Call => mast_forest_builder.ensure_call(invoked_proc_node_id), + InvokeKind::SysCall => mast_forest_builder.ensure_syscall(invoked_proc_node_id), } } - fn invoke_mast_root( + /// Returns the [`MastNodeId`] associated with the provided MAST root if known, or wraps the + /// MAST root in a [`vm_core::mast::ExternalNode`] and returns it. + fn get_proc_root_id_from_mast_root( &self, kind: InvokeKind, span: SourceSpan, @@ -81,68 +86,20 @@ impl Assembler { Some(_) | None => (), } - let mast_root_node_id = { - // Note that here we rely on the fact that we topologically sorted the - // procedures, such that when we assemble a procedure, all - // procedures that it calls will have been assembled, and - // hence be present in the `MastForest`. - match kind { - InvokeKind::ProcRef => { - match mast_forest_builder.find_procedure_node_id(mast_root) { - Some(root) => root, - None => { - // If the MAST root called isn't known to us, make it an external - // reference. - mast_forest_builder.ensure_external(mast_root)? - }, - } - }, - InvokeKind::Exec => { - match mast_forest_builder.find_procedure_node_id(mast_root) { - Some(root) => { - // We make sure to copy the root node so that the `exec` is associated - // with a different `MastNodeId` than the procedure it is referencing. - // Currently the only purpose of this is so that simple procedures that - // only have an `exec` have a different body node id than the procedure - // they're executing. - let root_node = mast_forest_builder.get_mast_node(root).unwrap(); - mast_forest_builder.ensure_node(root_node.clone())? - }, - None => { - // If the MAST root called isn't known to us, make it an external - // reference. - mast_forest_builder.ensure_external(mast_root)? - }, - } - }, - InvokeKind::Call => { - let callee_id = match mast_forest_builder.find_procedure_node_id(mast_root) { - Some(callee_id) => callee_id, - None => { - // If the MAST root called isn't known to us, make it an external - // reference. - mast_forest_builder.ensure_external(mast_root)? - }, - }; - - mast_forest_builder.ensure_call(callee_id)? - }, - InvokeKind::SysCall => { - let callee_id = match mast_forest_builder.find_procedure_node_id(mast_root) { - Some(callee_id) => callee_id, - None => { - // If the MAST root called isn't known to us, make it an external - // reference. - mast_forest_builder.ensure_external(mast_root)? - }, - }; - - mast_forest_builder.ensure_syscall(callee_id)? - }, - } + // Note that here we rely on the fact that we topologically sorted the + // procedures, such that when we assemble a procedure, all + // procedures that it calls will have been assembled, and + // hence be present in the `MastForest`. + let invoked_node_id = match mast_forest_builder.find_procedure_node_id(mast_root) { + Some(root) => root, + None => { + // If the MAST root called isn't known to us, make it an external + // reference. + mast_forest_builder.ensure_external(mast_root)? + }, }; - Ok(mast_root_node_id) + Ok(invoked_node_id) } /// Creates a new DYN block for the dynamic code execution and return. From c5e6b8bb963bf33770188c46501bc19ad38eec1f Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 13:30:11 -0400 Subject: [PATCH 35/57] remove stale TODOs --- assembly/src/assembler/module_graph/mod.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index 420cfc661..38db4a52f 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -529,8 +529,6 @@ impl ModuleGraph { &self, procedure_digest: &RpoDigest, ) -> Option { - // TODO(plafer): emit warning if more than one? - // TODO(plafer): return the whole vec? self.procedure_root_digests.get(procedure_digest).map(|indices| indices[0]) } @@ -555,7 +553,6 @@ impl ModuleGraph { pub(crate) fn register_procedure_root( &mut self, id: GlobalProcedureIndex, - // TODO(plafer): all `procedure_root_id` field, and add to `self.procedure_root_ids` procedure_root_digest: RpoDigest, ) -> Result<(), AssemblyError> { use alloc::collections::btree_map::Entry; From 485cbc8f9a3c0479e2e32fb0901362734e6cc770 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 13:39:55 -0400 Subject: [PATCH 36/57] fix doc tests --- assembly/src/library/mod.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index 21caa908b..d6231defc 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -255,10 +255,15 @@ mod use_std_library { /// For example, let's say I call this function like so: /// /// ```rust + /// use std::sync::Arc; + /// + /// use miden_assembly::{Assembler, Library, LibraryNamespace}; + /// use vm_core::debuginfo::DefaultSourceManager; + /// /// Library::from_dir( /// "~/masm/std", - /// LibraryNamespace::new("std").unwrap() - /// Arc::new(crate::DefaultSourceManager::default()), + /// LibraryNamespace::new("std").unwrap(), + /// Assembler::new(Arc::new(DefaultSourceManager::default())), /// ); /// ``` /// From 191404a1a812fdd167ce9b95088b0d096622822a Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Thu, 29 Aug 2024 13:42:49 -0400 Subject: [PATCH 37/57] fix no_std --- core/src/mast/node/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/src/mast/node/mod.rs b/core/src/mast/node/mod.rs index bbe1fe75e..a9ea9ffda 100644 --- a/core/src/mast/node/mod.rs +++ b/core/src/mast/node/mod.rs @@ -1,7 +1,6 @@ mod basic_block_node; -use alloc::{boxed::Box, vec::Vec}; +use alloc::{boxed::Box, string::ToString, vec::Vec}; use core::fmt; -use std::string::ToString; pub use basic_block_node::{ BasicBlockNode, OpBatch, OperationOrDecorator, BATCH_SIZE as OP_BATCH_SIZE, From 5b3285e4a797b4995e19a549c0cb43c5df43d4f9 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 30 Aug 2024 09:34:50 -0400 Subject: [PATCH 38/57] `TestContext`: don't use debug mode by default --- assembly/src/assembler/tests.rs | 5 ++--- assembly/src/testing.rs | 6 +++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index 90e497abf..c1ef59d28 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -154,8 +154,7 @@ fn nested_blocks() -> Result<(), Report> { Ok(()) } -/// Ensures that adding procedures with the same MAST root results in 2 different procedures in the -/// assembled program. +/// Since `foo` and `bar` have the same body, we only expect them to be added once to the program. #[test] fn duplicate_procedure() { let context = TestContext::new(); @@ -179,7 +178,7 @@ fn duplicate_procedure() { "#; let program = context.assemble(program_source).unwrap(); - assert_eq!(program.num_procedures(), 3); + assert_eq!(program.num_procedures(), 2); } /// Ensures that equal MAST nodes don't get added twice to a MAST forest diff --git a/assembly/src/testing.rs b/assembly/src/testing.rs index e2630fbf0..2ba14bcfe 100644 --- a/assembly/src/testing.rs +++ b/assembly/src/testing.rs @@ -199,9 +199,9 @@ impl TestContext { let _ = set_hook(Box::new(|_| Box::new(ReportHandlerOpts::new().build()))); } let source_manager = Arc::new(crate::DefaultSourceManager::default()); - let assembler = Assembler::new(source_manager.clone()) - .with_debug_mode(true) - .with_warnings_as_errors(true); + // Note: we do not set debug mode by default because we do not want AsmOp decorators to be + // inserted in our programs + let assembler = Assembler::new(source_manager.clone()).with_warnings_as_errors(true); Self { source_manager, assembler } } From f141c764790d5e5a84d439f3ec1993b572827f7e Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Fri, 30 Aug 2024 14:35:10 -0400 Subject: [PATCH 39/57] fix asmop hash --- core/src/mast/node/mod.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/src/mast/node/mod.rs b/core/src/mast/node/mod.rs index a9ea9ffda..d568670ed 100644 --- a/core/src/mast/node/mod.rs +++ b/core/src/mast/node/mod.rs @@ -212,9 +212,15 @@ impl MastNode { bytes_to_hash.extend(advice.to_string().as_bytes()) }, Decorator::AsmOp(asm_op) => { + if let Some(location) = asm_op.location() { + bytes_to_hash.extend(location.path.as_bytes()); + bytes_to_hash.extend(location.start.to_u32().to_le_bytes()); + bytes_to_hash.extend(location.end.to_u32().to_le_bytes()); + } bytes_to_hash.extend(asm_op.context_name().as_bytes()); bytes_to_hash.extend(asm_op.op().as_bytes()); bytes_to_hash.push(asm_op.num_cycles()); + bytes_to_hash.push(asm_op.should_break() as u8); }, Decorator::Debug(debug) => { bytes_to_hash.extend(debug.to_string().as_bytes()) From 8dd16620b45220c41db895bc5dc72adec7cab642 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 10:34:27 -0400 Subject: [PATCH 40/57] Remove redundant comments --- assembly/src/assembler/tests.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/assembly/src/assembler/tests.rs b/assembly/src/assembler/tests.rs index c1ef59d28..911cb04ee 100644 --- a/assembly/src/assembler/tests.rs +++ b/assembly/src/assembler/tests.rs @@ -200,10 +200,8 @@ fn duplicate_nodes() { let mut expected_mast_forest = MastForest::new(); - // basic block: mul let mul_basic_block_id = expected_mast_forest.add_block(vec![Operation::Mul], None).unwrap(); - // basic block: add and mul let add_basic_block_id = expected_mast_forest.add_block(vec![Operation::Add], None).unwrap(); // inner split: `if.true add else mul end` From ca2d6d1f4dae245526929c7eb47d5d3320cbe5aa Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 10:36:01 -0400 Subject: [PATCH 41/57] revert comment change --- assembly/src/assembler/module_graph/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index 38db4a52f..bb82d294f 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -158,7 +158,7 @@ pub struct ModuleGraph { /// The global call graph of calls, not counting those that are performed directly via MAST /// root. callgraph: CallGraph, - /// The set of MAST node ids which have procedure definitions in this graph. There can be + /// The set of MAST roots which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. procedure_root_digests: BTreeMap>, kernel_index: Option, From 1b619e65b8af29b2292086ae8edc1abaf2edeb08 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 10:40:38 -0400 Subject: [PATCH 42/57] fix docstring --- assembly/src/assembler/mast_forest_builder.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 9a8054680..5d71718cc 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -25,6 +25,9 @@ const PROCEDURE_INLINING_THRESHOLD: usize = 32; /// /// The purpose of the builder is to ensure that the underlying MAST forest contains as little /// information as possible needed to adequately describe the logical MAST forest. Specifically: +/// - The builder ensures that only one copy of nodes that have the same MAST root and decorators is +/// added to the MAST forest (i.e., two nodes that have the same MAST root and decorators will +/// have the same [`MastNodeId`]). /// - The builder tries to merge adjacent basic blocks and eliminate the source block whenever this /// does not have an impact on other nodes in the forest. #[derive(Clone, Debug, Default)] From a30a9c73d27ae9147b1271f864496e27dc655040 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 10:47:54 -0400 Subject: [PATCH 43/57] `Library::new()`: check that every procedures are indeed exports --- assembly/src/library/error.rs | 2 ++ assembly/src/library/mod.rs | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/assembly/src/library/error.rs b/assembly/src/library/error.rs index c8e31f79d..3df795ca3 100644 --- a/assembly/src/library/error.rs +++ b/assembly/src/library/error.rs @@ -11,4 +11,6 @@ pub enum LibraryError { InvalidKernelExport { procedure_path: QualifiedProcedureName }, #[error(transparent)] Kernel(#[from] KernelError), + #[error("invalid export: no procedure root for {procedure_path} procedure")] + NoProcedureRootForExport { procedure_path: QualifiedProcedureName }, } diff --git a/assembly/src/library/mod.rs b/assembly/src/library/mod.rs index d6231defc..7ca5f3572 100644 --- a/assembly/src/library/mod.rs +++ b/assembly/src/library/mod.rs @@ -77,6 +77,12 @@ impl Library { mast_forest: Arc, exports: BTreeMap, ) -> Result { + for (fqn, &proc_body_id) in exports.iter() { + if !mast_forest.is_procedure_root(proc_body_id) { + return Err(LibraryError::NoProcedureRootForExport { procedure_path: fqn.clone() }); + } + } + let digest = compute_content_hash(&exports, &mast_forest); Ok(Self { digest, exports, mast_forest }) From 3b280f193413cf7cba0ae1850d905a6831ff21b3 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 10:53:42 -0400 Subject: [PATCH 44/57] rename `procedure_root_digest` --- assembly/src/assembler/module_graph/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/assembly/src/assembler/module_graph/mod.rs b/assembly/src/assembler/module_graph/mod.rs index bb82d294f..c96617a31 100644 --- a/assembly/src/assembler/module_graph/mod.rs +++ b/assembly/src/assembler/module_graph/mod.rs @@ -160,7 +160,7 @@ pub struct ModuleGraph { callgraph: CallGraph, /// The set of MAST roots which have procedure definitions in this graph. There can be /// multiple procedures bound to the same root due to having identical code. - procedure_root_digests: BTreeMap>, + procedures_by_mast_root: BTreeMap>, kernel_index: Option, kernel: Kernel, source_manager: Arc, @@ -175,7 +175,7 @@ impl ModuleGraph { modules: Default::default(), pending: Default::default(), callgraph: Default::default(), - procedure_root_digests: Default::default(), + procedures_by_mast_root: Default::default(), kernel_index: None, kernel: Default::default(), source_manager, @@ -529,7 +529,7 @@ impl ModuleGraph { &self, procedure_digest: &RpoDigest, ) -> Option { - self.procedure_root_digests.get(procedure_digest).map(|indices| indices[0]) + self.procedures_by_mast_root.get(procedure_digest).map(|indices| indices[0]) } /// Resolves `target` from the perspective of `caller`. @@ -553,10 +553,10 @@ impl ModuleGraph { pub(crate) fn register_procedure_root( &mut self, id: GlobalProcedureIndex, - procedure_root_digest: RpoDigest, + procedure_mast_root: RpoDigest, ) -> Result<(), AssemblyError> { use alloc::collections::btree_map::Entry; - match self.procedure_root_digests.entry(procedure_root_digest) { + match self.procedures_by_mast_root.entry(procedure_mast_root) { Entry::Occupied(ref mut entry) => { let prev_id = entry.get()[0]; if prev_id != id { From 41f45d4ea62540d9573a31a6a278991e2966b138 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 10:54:49 -0400 Subject: [PATCH 45/57] fix docs --- assembly/src/assembler/instruction/procedures.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index edc4172db..9f08c7a63 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -70,8 +70,8 @@ impl Assembler { }) .and_then(|module| { // Note: this module is guaranteed to be of AST variant, since we have the - // AST of a procedure contained in it (i.e. `proc`). Hence, it must be - // thatthe entire module is in AST representation as well. + // AST of a procedure contained in it (i.e. `proc`). Hence, it must be that + // the entire module is in AST representation as well. if module.unwrap_ast().is_kernel() { Ok(()) } else { @@ -86,9 +86,8 @@ impl Assembler { Some(_) | None => (), } - // Note that here we rely on the fact that we topologically sorted the - // procedures, such that when we assemble a procedure, all - // procedures that it calls will have been assembled, and + // Note that here we rely on the fact that we topologically sorted the procedures, such that + // when we assemble a procedure, all procedures that it calls will have been assembled, and // hence be present in the `MastForest`. let invoked_node_id = match mast_forest_builder.find_procedure_node_id(mast_root) { Some(root) => root, From ed09c6c73eabfb9f92d0bea38a8fa345bbd4d28e Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 11:29:07 -0400 Subject: [PATCH 46/57] refactor `Assembler::resolve_target()` --- .../src/assembler/instruction/procedures.rs | 99 ++------------- assembly/src/assembler/mod.rs | 119 +++++++++++++++--- 2 files changed, 110 insertions(+), 108 deletions(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 9f08c7a63..c35fbdbbf 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -1,11 +1,11 @@ use smallvec::SmallVec; -use vm_core::{mast::MastNodeId, utils::Either}; +use vm_core::mast::MastNodeId; use super::{Assembler, BasicBlockBuilder, Operation}; use crate::{ assembler::{mast_forest_builder::MastForestBuilder, ProcedureContext}, ast::{InvocationTarget, InvokeKind}, - AssemblyError, RpoDigest, SourceSpan, Spanned, + AssemblyError, RpoDigest, }; /// Procedure Invocation @@ -17,14 +17,8 @@ impl Assembler { proc_ctx: &mut ProcedureContext, mast_forest_builder: &mut MastForestBuilder, ) -> Result { - let span = callee.span(); - let node_id_or_digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; - let invoked_proc_node_id = match node_id_or_digest { - Either::Left(node_id) => node_id, - Either::Right(mast_root) => { - self.get_proc_root_id_from_mast_root(kind, span, mast_root, mast_forest_builder)? - }, - }; + let invoked_proc_node_id = + self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; match kind { InvokeKind::ProcRef | InvokeKind::Exec => Ok(invoked_proc_node_id), @@ -33,74 +27,6 @@ impl Assembler { } } - /// Returns the [`MastNodeId`] associated with the provided MAST root if known, or wraps the - /// MAST root in a [`vm_core::mast::ExternalNode`] and returns it. - fn get_proc_root_id_from_mast_root( - &self, - kind: InvokeKind, - span: SourceSpan, - mast_root: RpoDigest, - mast_forest_builder: &mut MastForestBuilder, - ) -> Result { - // Get the procedure from the assembler - let current_source_file = self.source_manager.get(span.source_id()).ok(); - - // If the procedure is cached and is a system call, ensure that the call is valid. - match mast_forest_builder.find_procedure_by_mast_root(&mast_root) { - Some(proc) if matches!(kind, InvokeKind::SysCall) => { - // Verify if this is a syscall, that the callee is a kernel procedure - // - // NOTE: The assembler is expected to know the full set of all kernel - // procedures at this point, so if we can't identify the callee as a - // kernel procedure, it is a definite error. - if !proc.visibility().is_syscall() { - return Err(AssemblyError::InvalidSysCallTarget { - span, - source_file: current_source_file, - callee: proc.fully_qualified_name().clone(), - }); - } - let maybe_kernel_path = proc.path(); - self.module_graph - .find_module(maybe_kernel_path) - .ok_or_else(|| AssemblyError::InvalidSysCallTarget { - span, - source_file: current_source_file.clone(), - callee: proc.fully_qualified_name().clone(), - }) - .and_then(|module| { - // Note: this module is guaranteed to be of AST variant, since we have the - // AST of a procedure contained in it (i.e. `proc`). Hence, it must be that - // the entire module is in AST representation as well. - if module.unwrap_ast().is_kernel() { - Ok(()) - } else { - Err(AssemblyError::InvalidSysCallTarget { - span, - source_file: current_source_file.clone(), - callee: proc.fully_qualified_name().clone(), - }) - } - })?; - }, - Some(_) | None => (), - } - - // Note that here we rely on the fact that we topologically sorted the procedures, such that - // when we assemble a procedure, all procedures that it calls will have been assembled, and - // hence be present in the `MastForest`. - let invoked_node_id = match mast_forest_builder.find_procedure_node_id(mast_root) { - Some(root) => root, - None => { - // If the MAST root called isn't known to us, make it an external - // reference. - mast_forest_builder.ensure_external(mast_root)? - }, - }; - - Ok(invoked_node_id) - } - /// Creates a new DYN block for the dynamic code execution and return. pub(super) fn dynexec( &self, @@ -129,18 +55,15 @@ impl Assembler { callee: &InvocationTarget, proc_ctx: &mut ProcedureContext, basic_block_builder: &mut BasicBlockBuilder, - mast_forest_builder: &MastForestBuilder, + mast_forest_builder: &mut MastForestBuilder, ) -> Result<(), AssemblyError> { - let digest = match self.resolve_target( - InvokeKind::ProcRef, - callee, - proc_ctx, - mast_forest_builder, - )? { - Either::Left(node_id) => mast_forest_builder.get_mast_node(node_id).unwrap().digest(), - Either::Right(digest) => digest, + let mast_root = { + let proc_body_id = + self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?; + mast_forest_builder.get_mast_node(proc_body_id).unwrap().digest() }; - self.procref_mast_root(digest, basic_block_builder) + + self.procref_mast_root(mast_root, basic_block_builder) } fn procref_mast_root( diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 480ea9768..629b75565 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -2,15 +2,17 @@ use alloc::{collections::BTreeMap, sync::Arc, vec::Vec}; use mast_forest_builder::MastForestBuilder; use module_graph::{ProcedureWrapper, WrappedModule}; -use vm_core::{mast::MastNodeId, utils::Either, DecoratorList, Felt, Kernel, Operation, Program}; +use vm_core::{ + crypto::hash::RpoDigest, debuginfo::SourceSpan, mast::MastNodeId, DecoratorList, Felt, Kernel, + Operation, Program, +}; use crate::{ ast::{self, Export, InvocationTarget, InvokeKind, ModuleKind, QualifiedProcedureName}, diagnostics::Report, library::{KernelLibrary, Library}, sema::SemanticAnalysisError, - AssemblyError, Compile, CompileOptions, LibraryNamespace, LibraryPath, RpoDigest, - SourceManager, Spanned, + AssemblyError, Compile, CompileOptions, LibraryNamespace, LibraryPath, SourceManager, Spanned, }; mod basic_block_builder; @@ -524,23 +526,21 @@ impl Assembler { ) .with_span(proc_alias.span()); - let proc_mast_root = match self.resolve_target( - InvokeKind::ProcRef, - &proc_alias.target().into(), - &pctx, - mast_forest_builder, - )? { - Either::Left(node_id) => { - mast_forest_builder.get_mast_node(node_id).unwrap().digest() - }, - Either::Right(digest) => digest, + let proc_mast_root = { + let node_id = self.resolve_target( + InvokeKind::ProcRef, + &proc_alias.target().into(), + &pctx, + mast_forest_builder, + )?; + mast_forest_builder.get_mast_node(node_id).unwrap().digest() }; // insert external node into the MAST forest for this procedure; if a procedure // with the same MAST rood had been previously added to the builder, this will // have no effect - let proc_node_id = mast_forest_builder.ensure_external(proc_mast_root)?; - let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); + let external_node_id = mast_forest_builder.ensure_external(proc_mast_root)?; + let procedure = pctx.into_procedure(proc_mast_root, external_node_id); // Make the MAST root available to all dependents self.module_graph.register_procedure_root(procedure_gid, proc_mast_root)?; @@ -679,13 +679,14 @@ impl Assembler { }) } + /// Resolves the specified target to the corresponding procedure root [`MastNodeId`]. pub(super) fn resolve_target( &self, kind: InvokeKind, target: &InvocationTarget, proc_ctx: &ProcedureContext, - mast_forest_builder: &MastForestBuilder, - ) -> Result, AssemblyError> { + mast_forest_builder: &mut MastForestBuilder, + ) -> Result { let caller = CallerInfo { span: target.span(), module: proc_ctx.id().module, @@ -693,20 +694,98 @@ impl Assembler { }; let resolved = self.module_graph.resolve_target(&caller, target)?; match resolved { - ResolvedTarget::Phantom(digest) => Ok(Either::Right(digest)), + ResolvedTarget::Phantom(mast_root) => self.get_proc_root_id_from_mast_root( + kind, + target.span(), + mast_root, + mast_forest_builder, + ), ResolvedTarget::Exact { gid } | ResolvedTarget::Resolved { gid, .. } => { match mast_forest_builder.get_procedure(gid) { - Some(proc) => Ok(Either::Left(proc.body_node_id())), + Some(proc) => Ok(proc.body_node_id()), // We didn't find the procedure in our current MAST forest. We still need to // check if it exists in one of a library dependency. None => match self.module_graph.get_procedure_unsafe(gid) { - ProcedureWrapper::Info(p) => Ok(Either::Right(p.digest)), + ProcedureWrapper::Info(p) => self.get_proc_root_id_from_mast_root( + kind, + target.span(), + p.digest, + mast_forest_builder, + ), ProcedureWrapper::Ast(_) => panic!("AST procedure {gid:?} exits in the module graph but not in the MastForestBuilder"), }, } }, } } + + /// Returns the [`MastNodeId`] associated with the provided MAST root if known, or wraps the + /// MAST root in a [`vm_core::mast::ExternalNode`] and returns it. + fn get_proc_root_id_from_mast_root( + &self, + kind: InvokeKind, + span: SourceSpan, + mast_root: RpoDigest, + mast_forest_builder: &mut MastForestBuilder, + ) -> Result { + // Get the procedure from the assembler + let current_source_file = self.source_manager.get(span.source_id()).ok(); + + // If the procedure is cached and is a system call, ensure that the call is valid. + match mast_forest_builder.find_procedure_by_mast_root(&mast_root) { + Some(proc) if matches!(kind, InvokeKind::SysCall) => { + // Verify if this is a syscall, that the callee is a kernel procedure + // + // NOTE: The assembler is expected to know the full set of all kernel + // procedures at this point, so if we can't identify the callee as a + // kernel procedure, it is a definite error. + if !proc.visibility().is_syscall() { + return Err(AssemblyError::InvalidSysCallTarget { + span, + source_file: current_source_file, + callee: proc.fully_qualified_name().clone(), + }); + } + let maybe_kernel_path = proc.path(); + self.module_graph + .find_module(maybe_kernel_path) + .ok_or_else(|| AssemblyError::InvalidSysCallTarget { + span, + source_file: current_source_file.clone(), + callee: proc.fully_qualified_name().clone(), + }) + .and_then(|module| { + // Note: this module is guaranteed to be of AST variant, since we have the + // AST of a procedure contained in it (i.e. `proc`). Hence, it must be that + // the entire module is in AST representation as well. + if module.unwrap_ast().is_kernel() { + Ok(()) + } else { + Err(AssemblyError::InvalidSysCallTarget { + span, + source_file: current_source_file.clone(), + callee: proc.fully_qualified_name().clone(), + }) + } + })?; + }, + Some(_) | None => (), + } + + // Note that here we rely on the fact that we topologically sorted the procedures, such that + // when we assemble a procedure, all procedures that it calls will have been assembled, and + // hence be present in the `MastForest`. + let invoked_node_id = match mast_forest_builder.find_procedure_node_id(mast_root) { + Some(root) => root, + None => { + // If the MAST root called isn't known to us, make it an external + // reference. + mast_forest_builder.ensure_external(mast_root)? + }, + }; + + Ok(invoked_node_id) + } } // HELPERS From 2125923f83ad1657186cc257dfa5feaed6973345 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 11:35:32 -0400 Subject: [PATCH 47/57] split `Assembler::get_proc_root_id_from_mast_root` --- assembly/src/assembler/mast_forest_builder.rs | 7 --- assembly/src/assembler/mod.rs | 45 +++++++++---------- 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/assembly/src/assembler/mast_forest_builder.rs b/assembly/src/assembler/mast_forest_builder.rs index 5d71718cc..25f603e3f 100644 --- a/assembly/src/assembler/mast_forest_builder.rs +++ b/assembly/src/assembler/mast_forest_builder.rs @@ -141,13 +141,6 @@ impl MastForestBuilder { .and_then(|gid| self.get_procedure(*gid)) } - /// Returns the [`MastNodeId`] of the procedure associated with a given MAST root, or None - /// if such a procedure is not present in this MAST forest builder. - #[inline(always)] - pub fn find_procedure_node_id(&self, mast_root: RpoDigest) -> Option { - self.mast_forest.find_procedure_root(mast_root) - } - /// Returns the [`MastNode`] for the provided MAST node ID, or None if a node with this ID is /// not present in this MAST forest builder. pub fn get_mast_node(&self, id: MastNodeId) -> Option<&MastNode> { diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 629b75565..48ad945d7 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -694,24 +694,32 @@ impl Assembler { }; let resolved = self.module_graph.resolve_target(&caller, target)?; match resolved { - ResolvedTarget::Phantom(mast_root) => self.get_proc_root_id_from_mast_root( - kind, - target.span(), - mast_root, - mast_forest_builder, - ), + ResolvedTarget::Phantom(mast_root) => { + self.ensure_valid_procedure_mast_root( + kind, + target.span(), + mast_root, + mast_forest_builder, + )?; + + mast_forest_builder.ensure_external(mast_root) + }, ResolvedTarget::Exact { gid } | ResolvedTarget::Resolved { gid, .. } => { match mast_forest_builder.get_procedure(gid) { Some(proc) => Ok(proc.body_node_id()), // We didn't find the procedure in our current MAST forest. We still need to // check if it exists in one of a library dependency. None => match self.module_graph.get_procedure_unsafe(gid) { - ProcedureWrapper::Info(p) => self.get_proc_root_id_from_mast_root( + ProcedureWrapper::Info(p) => { + self.ensure_valid_procedure_mast_root( kind, target.span(), p.digest, mast_forest_builder, - ), + )?; + + mast_forest_builder.ensure_external(p.digest) + }, ProcedureWrapper::Ast(_) => panic!("AST procedure {gid:?} exits in the module graph but not in the MastForestBuilder"), }, } @@ -719,15 +727,14 @@ impl Assembler { } } - /// Returns the [`MastNodeId`] associated with the provided MAST root if known, or wraps the - /// MAST root in a [`vm_core::mast::ExternalNode`] and returns it. - fn get_proc_root_id_from_mast_root( + /// Verifies the validity of the MAST root as a procedure root. + fn ensure_valid_procedure_mast_root( &self, kind: InvokeKind, span: SourceSpan, mast_root: RpoDigest, mast_forest_builder: &mut MastForestBuilder, - ) -> Result { + ) -> Result<(), AssemblyError> { // Get the procedure from the assembler let current_source_file = self.source_manager.get(span.source_id()).ok(); @@ -772,19 +779,7 @@ impl Assembler { Some(_) | None => (), } - // Note that here we rely on the fact that we topologically sorted the procedures, such that - // when we assemble a procedure, all procedures that it calls will have been assembled, and - // hence be present in the `MastForest`. - let invoked_node_id = match mast_forest_builder.find_procedure_node_id(mast_root) { - Some(root) => root, - None => { - // If the MAST root called isn't known to us, make it an external - // reference. - mast_forest_builder.ensure_external(mast_root)? - }, - }; - - Ok(invoked_node_id) + Ok(()) } } From d294d207c5e5f31811d139d7fa9be18307d6b5b6 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 11:39:24 -0400 Subject: [PATCH 48/57] `Assembler::resolve_target` docstring --- assembly/src/assembler/mod.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 48ad945d7..1c3095c83 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -680,6 +680,9 @@ impl Assembler { } /// Resolves the specified target to the corresponding procedure root [`MastNodeId`]. + /// + /// If no [`MastNodeId`] exists for that procedure root, we wrap the root in an + /// [`crate::mast::ExternalNode`], and return the resulting [`MastNodeId`]. pub(super) fn resolve_target( &self, kind: InvokeKind, From 43185ec67b13827cd31cdddc16ee6882131409fb Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 11:50:09 -0400 Subject: [PATCH 49/57] fmt --- assembly/src/assembler/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 1c3095c83..48eec1603 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -680,7 +680,7 @@ impl Assembler { } /// Resolves the specified target to the corresponding procedure root [`MastNodeId`]. - /// + /// /// If no [`MastNodeId`] exists for that procedure root, we wrap the root in an /// [`crate::mast::ExternalNode`], and return the resulting [`MastNodeId`]. pub(super) fn resolve_target( From 4c139bb9daba7d522b6cd84f06f14cb82f0fcfb5 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 11:59:06 -0400 Subject: [PATCH 50/57] `invoke`: document --- assembly/src/assembler/instruction/procedures.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index c35fbdbbf..7208ce409 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -10,11 +10,14 @@ use crate::{ /// Procedure Invocation impl Assembler { + /// Returns the [`MastNodeId`] of the invoked procedure specified by `callee`. + /// + /// For example, given `exec.f`, this method would return the procedure body id of `f`. pub(super) fn invoke( &self, kind: InvokeKind, callee: &InvocationTarget, - proc_ctx: &mut ProcedureContext, + proc_ctx: &ProcedureContext, mast_forest_builder: &mut MastForestBuilder, ) -> Result { let invoked_proc_node_id = From dd72f689536e88f1f6c1f491c87c2766ee0beea7 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 12:00:49 -0400 Subject: [PATCH 51/57] fmt --- assembly/src/assembler/instruction/procedures.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 7208ce409..42775e535 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -11,7 +11,7 @@ use crate::{ /// Procedure Invocation impl Assembler { /// Returns the [`MastNodeId`] of the invoked procedure specified by `callee`. - /// + /// /// For example, given `exec.f`, this method would return the procedure body id of `f`. pub(super) fn invoke( &self, From 72b9fe55cff5f6544d36496b8ac3ed5d5473f9b3 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 18:11:21 -0400 Subject: [PATCH 52/57] add `unwrap()` comment --- assembly/src/assembler/instruction/procedures.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 42775e535..7c7e642b6 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -63,6 +63,7 @@ impl Assembler { let mast_root = { let proc_body_id = self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?; + // Note: it's ok to `unwrap()` here since `proc_body_id` was returned from `mast_forest_builder` mast_forest_builder.get_mast_node(proc_body_id).unwrap().digest() }; From 8243fa2318baadd44c51fcbd16d07e461e8e24aa Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 18:15:57 -0400 Subject: [PATCH 53/57] fmt --- assembly/src/assembler/instruction/procedures.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 7c7e642b6..d7f80f620 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -63,7 +63,8 @@ impl Assembler { let mast_root = { let proc_body_id = self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?; - // Note: it's ok to `unwrap()` here since `proc_body_id` was returned from `mast_forest_builder` + // Note: it's ok to `unwrap()` here since `proc_body_id` was returned from + // `mast_forest_builder` mast_forest_builder.get_mast_node(proc_body_id).unwrap().digest() }; From 4eeeb03d6ec56beb2e059ebf1b4a5c79ba98cd6e Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 18:20:33 -0400 Subject: [PATCH 54/57] adjust `Assembler::invoke()` comment --- assembly/src/assembler/instruction/procedures.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index d7f80f620..8f7249df0 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -12,7 +12,9 @@ use crate::{ impl Assembler { /// Returns the [`MastNodeId`] of the invoked procedure specified by `callee`. /// - /// For example, given `exec.f`, this method would return the procedure body id of `f`. + /// For example, given `exec.f`, this method would return the procedure body id of `f`. If the + /// only representation of `f` that we have is its MAST root, then this method will also insert + /// a [`core::mast::ExternalNode`] that wraps `f`'s MAST root and return the corresponding id. pub(super) fn invoke( &self, kind: InvokeKind, From 2ffd050b17ad6ca3066085781e3b4ba6bbeed9f2 Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 18:25:34 -0400 Subject: [PATCH 55/57] fix `Assembler::ensure_valid_procedure_mast_root` --- assembly/src/assembler/mod.rs | 34 ++++++++++++++-------------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 48eec1603..55eb01f03 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -537,7 +537,7 @@ impl Assembler { }; // insert external node into the MAST forest for this procedure; if a procedure - // with the same MAST rood had been previously added to the builder, this will + // with the same MAST root had been previously added to the builder, this will // have no effect let external_node_id = mast_forest_builder.ensure_external(proc_mast_root)?; let procedure = pctx.into_procedure(proc_mast_root, external_node_id); @@ -697,32 +697,25 @@ impl Assembler { }; let resolved = self.module_graph.resolve_target(&caller, target)?; match resolved { - ResolvedTarget::Phantom(mast_root) => { - self.ensure_valid_procedure_mast_root( - kind, - target.span(), - mast_root, - mast_forest_builder, - )?; - - mast_forest_builder.ensure_external(mast_root) - }, + ResolvedTarget::Phantom(mast_root) => self.ensure_valid_procedure_mast_root( + kind, + target.span(), + mast_root, + mast_forest_builder, + ), ResolvedTarget::Exact { gid } | ResolvedTarget::Resolved { gid, .. } => { match mast_forest_builder.get_procedure(gid) { Some(proc) => Ok(proc.body_node_id()), // We didn't find the procedure in our current MAST forest. We still need to // check if it exists in one of a library dependency. None => match self.module_graph.get_procedure_unsafe(gid) { - ProcedureWrapper::Info(p) => { - self.ensure_valid_procedure_mast_root( + ProcedureWrapper::Info(p) => self.ensure_valid_procedure_mast_root( kind, target.span(), p.digest, mast_forest_builder, - )?; - - mast_forest_builder.ensure_external(p.digest) - }, + ) + , ProcedureWrapper::Ast(_) => panic!("AST procedure {gid:?} exits in the module graph but not in the MastForestBuilder"), }, } @@ -730,14 +723,15 @@ impl Assembler { } } - /// Verifies the validity of the MAST root as a procedure root. + /// Verifies the validity of the MAST root as a procedure root hash, and returns the ID of the + /// [`core::mast::ExternalNode`] that wraps it. fn ensure_valid_procedure_mast_root( &self, kind: InvokeKind, span: SourceSpan, mast_root: RpoDigest, mast_forest_builder: &mut MastForestBuilder, - ) -> Result<(), AssemblyError> { + ) -> Result { // Get the procedure from the assembler let current_source_file = self.source_manager.get(span.source_id()).ok(); @@ -782,7 +776,7 @@ impl Assembler { Some(_) | None => (), } - Ok(()) + mast_forest_builder.ensure_external(mast_root) } } From 9b74a8b34edef3d16cc63c12135f356985e6ebae Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 18:30:23 -0400 Subject: [PATCH 56/57] cleanup proc alias --- assembly/src/assembler/mod.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 55eb01f03..7b1988b45 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -526,21 +526,18 @@ impl Assembler { ) .with_span(proc_alias.span()); - let proc_mast_root = { - let node_id = self.resolve_target( - InvokeKind::ProcRef, - &proc_alias.target().into(), - &pctx, - mast_forest_builder, - )?; - mast_forest_builder.get_mast_node(node_id).unwrap().digest() - }; + let proc_node_id = self.resolve_target( + InvokeKind::ProcRef, + &proc_alias.target().into(), + &pctx, + mast_forest_builder, + )?; + let proc_mast_root = mast_forest_builder.get_mast_node(proc_node_id).unwrap().digest(); // insert external node into the MAST forest for this procedure; if a procedure // with the same MAST root had been previously added to the builder, this will // have no effect - let external_node_id = mast_forest_builder.ensure_external(proc_mast_root)?; - let procedure = pctx.into_procedure(proc_mast_root, external_node_id); + let procedure = pctx.into_procedure(proc_mast_root, proc_node_id); // Make the MAST root available to all dependents self.module_graph.register_procedure_root(procedure_gid, proc_mast_root)?; From 245022ee201418c19b321bcef1fa18182f568dff Mon Sep 17 00:00:00 2001 From: Philippe Laferriere Date: Tue, 3 Sep 2024 18:30:37 -0400 Subject: [PATCH 57/57] fmt --- assembly/src/assembler/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/assembly/src/assembler/mod.rs b/assembly/src/assembler/mod.rs index 7b1988b45..2caf8a519 100644 --- a/assembly/src/assembler/mod.rs +++ b/assembly/src/assembler/mod.rs @@ -532,7 +532,8 @@ impl Assembler { &pctx, mast_forest_builder, )?; - let proc_mast_root = mast_forest_builder.get_mast_node(proc_node_id).unwrap().digest(); + let proc_mast_root = + mast_forest_builder.get_mast_node(proc_node_id).unwrap().digest(); // insert external node into the MAST forest for this procedure; if a procedure // with the same MAST root had been previously added to the builder, this will