From 07134e3858aeb489ee3ee7a85501a4bad84b1e7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Laferri=C3=A8re?= Date: Tue, 27 Aug 2024 13:11:40 -0400 Subject: [PATCH 001/104] chore: remove callset from `Procedure` (#1470) * chore: remove callset from `Procedure` * chore: fix clippy --- air/src/constraints/stack/field_ops/mod.rs | 8 ++- air/src/constraints/stack/mod.rs | 18 +++--- .../stack/stack_manipulation/mod.rs | 8 ++- air/src/constraints/stack/system_ops/mod.rs | 7 ++- air/src/constraints/stack/u32_ops/mod.rs | 13 +++-- air/src/trace/chiplets/kernel_rom.rs | 9 +-- air/src/trace/chiplets/memory.rs | 7 ++- .../src/assembler/instruction/procedures.rs | 27 +++------ assembly/src/assembler/procedure.rs | 55 +------------------ assembly/src/diagnostics.rs | 10 ++-- assembly/src/testing.rs | 11 ++-- processor/src/debug.rs | 7 +-- test-utils/src/lib.rs | 11 ++-- 13 files changed, 70 insertions(+), 121 deletions(-) diff --git a/air/src/constraints/stack/field_ops/mod.rs b/air/src/constraints/stack/field_ops/mod.rs index 648d8afdfc..747a19f528 100644 --- a/air/src/constraints/stack/field_ops/mod.rs +++ b/air/src/constraints/stack/field_ops/mod.rs @@ -358,9 +358,11 @@ pub fn enforce_expacc_constraints( 4 } -/// Enforces constraints of the EXT2MUL operation. The EXT2MUL operation computes the product of -/// two elements in the extension field of degree 2. Therefore, the following constraints are -/// enforced, assuming the first 4 elements of the stack in the current frame are a1, a0, b1, b0: +/// Enforces constraints of the EXT2MUL operation. +/// +/// The EXT2MUL operation computes the product of two elements in the extension field of degree 2. +/// Therefore, the following constraints are enforced, assuming the first 4 elements of the stack in +/// the current frame are a1, a0, b1, b0: /// - The first element in the next frame should be a1. /// - The second element in the next frame should be a0. /// - The third element in the next frame should be equal to (b0 + b1) * (a0 + a1) - b0 * a0. diff --git a/air/src/constraints/stack/mod.rs b/air/src/constraints/stack/mod.rs index b5046a4684..7cc021824c 100644 --- a/air/src/constraints/stack/mod.rs +++ b/air/src/constraints/stack/mod.rs @@ -35,14 +35,16 @@ pub const NUM_ASSERTIONS: usize = 2 * STACK_TOP_SIZE + 2; /// The number of general constraints in the stack operations. pub const NUM_GENERAL_CONSTRAINTS: usize = 17; -/// The degrees of constraints in the general stack operations. Each operation being executed -/// either shifts the stack to the left, right or doesn't effect it at all. Therefore, majority -/// of the general transitions of a stack item would be common across the operations and composite -/// flags were introduced to compute the individual stack item transition. A particular item lets -/// say at depth ith in the next stack frame can be transitioned into from ith depth (no shift op) -/// or (i+1)th depth(left shift) or (i-1)th depth(right shift) in the current frame. Therefore, the -/// VM would require only 16 general constraints to encompass all the 16 stack positions. -/// The last constraint checks if the top element in the stack is a binary or not. +/// The degrees of constraints in the general stack operations. +/// +/// Each operation being executed either shifts the stack to the left, right or doesn't effect it at +/// all. Therefore, majority of the general transitions of a stack item would be common across the +/// operations and composite flags were introduced to compute the individual stack item transition. +/// A particular item lets say at depth ith in the next stack frame can be transitioned into from +/// ith depth (no shift op) or (i+1)th depth(left shift) or (i-1)th depth(right shift) in the +/// current frame. Therefore, the VM would require only 16 general constraints to encompass all the +/// 16 stack positions. The last constraint checks if the top element in the stack is a binary or +/// not. pub const CONSTRAINT_DEGREES: [usize; NUM_GENERAL_CONSTRAINTS] = [ // Each degree are being multiplied with the respective composite flags which are of degree 7. // Therefore, all the degree would incorporate 7 in their degree calculation. diff --git a/air/src/constraints/stack/stack_manipulation/mod.rs b/air/src/constraints/stack/stack_manipulation/mod.rs index 70a4911a3b..34824668de 100644 --- a/air/src/constraints/stack/stack_manipulation/mod.rs +++ b/air/src/constraints/stack/stack_manipulation/mod.rs @@ -91,9 +91,11 @@ pub fn enforce_pad_constraints( 1 } -/// Enforces constraints of the DUPn and MOVUPn operations. The DUPn operation copies the element -/// at depth n in the stack and pushes the copy onto the stack, whereas MOVUPn opearation moves the -/// element at depth n to the top of the stack. Therefore, the following constraints are enforced: +/// Enforces constraints of the DUPn and MOVUPn operations. +/// +/// The DUPn operation copies the element at depth n in the stack and pushes the copy onto the +/// stack, whereas MOVUPn opearation moves the element at depth n to the top of the stack. +/// Therefore, the following constraints are enforced: /// - The top element in the next frame should be equal to the element at depth n in the current /// frame. s0` - sn = 0. pub fn enforce_dup_movup_n_constraints( diff --git a/air/src/constraints/stack/system_ops/mod.rs b/air/src/constraints/stack/system_ops/mod.rs index e21887c977..089eed8d7d 100644 --- a/air/src/constraints/stack/system_ops/mod.rs +++ b/air/src/constraints/stack/system_ops/mod.rs @@ -89,9 +89,10 @@ pub fn enforce_fmpadd_constraints( 1 } -/// Enforces constraints of the FMPUPDATE operation. The FMPUPDATE operation increments the fmp -/// register value by the first element value in the current trace. Therefore, the following -/// constraints are enforced: +/// Enforces constraints of the FMPUPDATE operation. +/// +/// The FMPUPDATE operation increments the fmp register value by the first element value in the +/// current trace. Therefore, the following constraints are enforced: /// - The fmp register value in the next frame should be equal to the sum of the fmp register value /// and the top stack element in the current frame. fmp` - (s0 + fmp) = 0. pub fn enforce_fmpupdate_constraints( diff --git a/air/src/constraints/stack/u32_ops/mod.rs b/air/src/constraints/stack/u32_ops/mod.rs index dd45e4d8a2..e4d0c3fe15 100644 --- a/air/src/constraints/stack/u32_ops/mod.rs +++ b/air/src/constraints/stack/u32_ops/mod.rs @@ -209,9 +209,10 @@ pub fn enforce_u32mul_constraints>( 1 } -/// Enforces constraints of the U32MADD operation. The U32MADD operation adds the third -/// element to the product of the first two elements in the current trace. Therefore, the -/// following constraints are enforced: +/// Enforces constraints of the U32MADD operation. +/// +/// The U32MADD operation adds the third element to the product of the first two elements in the +/// current trace. Therefore, the following constraints are enforced: /// - The aggregation of all the limbs in the helper registers is equal to the sum of the third /// element with the product of the first two elements in the current trace. pub fn enforce_u32madd_constraints>( @@ -231,8 +232,10 @@ pub fn enforce_u32madd_constraints>( 1 } -/// Enforces constraints of the U32DIV operation. The U32DIV operation divides the second element -/// with the first element in the current trace. Therefore, the following constraints are enforced: +/// Enforces constraints of the U32DIV operation. +/// +/// The U32DIV operation divides the second element with the first element in the current trace. +/// Therefore, the following constraints are enforced: /// - The second element in the current trace should be equal to the sum of the first element in the /// next trace with the product of the first element in the current trace and second element in /// the next trace. diff --git a/air/src/trace/chiplets/kernel_rom.rs b/air/src/trace/chiplets/kernel_rom.rs index 28ac08f379..8d87f286ee 100644 --- a/air/src/trace/chiplets/kernel_rom.rs +++ b/air/src/trace/chiplets/kernel_rom.rs @@ -8,8 +8,9 @@ pub const TRACE_WIDTH: usize = 6; // --- OPERATION SELECTORS ------------------------------------------------------------------------ -/// Specifies a kernel procedure call operation to access a procedure in the kernel ROM. The unique -/// operation label is computed as 1 plus the combined chiplet and internal selector with the bits -/// reversed. -/// kernel ROM selector=[1, 1, 1, 0] +1=[0, 0, 0, 1] +/// Specifies a kernel procedure call operation to access a procedure in the kernel ROM. +/// +/// The unique operation label is computed as 1 plus the combined chiplet and internal selector with +/// the bits reversed. +/// - kernel ROM selector=[1, 1, 1, 0] +1=[0, 0, 0, 1] pub const KERNEL_PROC_LABEL: Felt = Felt::new(0b1000); diff --git a/air/src/trace/chiplets/memory.rs b/air/src/trace/chiplets/memory.rs index 9990e1219b..6e531d30d1 100644 --- a/air/src/trace/chiplets/memory.rs +++ b/air/src/trace/chiplets/memory.rs @@ -9,9 +9,10 @@ pub const TRACE_WIDTH: usize = 12; /// Number of selector columns in the trace. pub const NUM_SELECTORS: usize = 2; -/// Type for Memory trace selectors. These selectors are used to define which operation and memory -/// state update (init & read / copy & read / write) is to be applied at a specific row of the -/// memory execution trace. +/// Type for Memory trace selectors. +/// +/// These selectors are used to define which operation and memory state update (init & read / copy & +/// read / write) is to be applied at a specific row of the memory execution trace. pub type Selectors = [Felt; NUM_SELECTORS]; // --- OPERATION SELECTORS ------------------------------------------------------------------------ diff --git a/assembly/src/assembler/instruction/procedures.rs b/assembly/src/assembler/instruction/procedures.rs index 1dfbfabc2c..c751109a80 100644 --- a/assembly/src/assembler/instruction/procedures.rs +++ b/assembly/src/assembler/instruction/procedures.rs @@ -19,7 +19,7 @@ impl Assembler { ) -> Result, AssemblyError> { let span = callee.span(); let digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?; - self.invoke_mast_root(kind, span, digest, proc_ctx, mast_forest_builder) + self.invoke_mast_root(kind, span, digest, mast_forest_builder) } fn invoke_mast_root( @@ -27,14 +27,12 @@ impl Assembler { kind: InvokeKind, span: SourceSpan, mast_root: RpoDigest, - proc_ctx: &mut ProcedureContext, mast_forest_builder: &mut MastForestBuilder, ) -> Result, AssemblyError> { // Get the procedure from the assembler let current_source_file = self.source_manager.get(span.source_id()).ok(); - // If the procedure is cached, register the call to ensure the callset - // is updated correctly. + // 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 @@ -71,10 +69,8 @@ impl Assembler { }) } })?; - proc_ctx.register_external_call(&proc, false)?; }, - Some(proc) => proc_ctx.register_external_call(&proc, false)?, - None => (), + Some(_) | None => (), } let mast_root_node_id = { @@ -150,34 +146,25 @@ impl Assembler { &self, callee: &InvocationTarget, proc_ctx: &mut ProcedureContext, - span_builder: &mut BasicBlockBuilder, + basic_block_builder: &mut BasicBlockBuilder, mast_forest_builder: &MastForestBuilder, ) -> Result<(), AssemblyError> { let digest = self.resolve_target(InvokeKind::ProcRef, callee, proc_ctx, mast_forest_builder)?; - self.procref_mast_root(digest, proc_ctx, span_builder, mast_forest_builder) + self.procref_mast_root(digest, basic_block_builder) } fn procref_mast_root( &self, mast_root: RpoDigest, - proc_ctx: &mut ProcedureContext, - span_builder: &mut BasicBlockBuilder, - mast_forest_builder: &MastForestBuilder, + basic_block_builder: &mut BasicBlockBuilder, ) -> Result<(), AssemblyError> { - // Add the root to the callset to be able to use dynamic instructions - // with the referenced procedure later - - if let Some(proc) = mast_forest_builder.find_procedure(&mast_root) { - proc_ctx.register_external_call(&proc, false)?; - } - // Create an array with `Push` operations containing root elements let ops = mast_root .iter() .map(|elem| Operation::Push(*elem)) .collect::>(); - span_builder.push_ops(ops); + basic_block_builder.push_ops(ops); Ok(()) } } diff --git a/assembly/src/assembler/procedure.rs b/assembly/src/assembler/procedure.rs index c1a59063b7..325e1e44cf 100644 --- a/assembly/src/assembler/procedure.rs +++ b/assembly/src/assembler/procedure.rs @@ -1,4 +1,4 @@ -use alloc::{collections::BTreeSet, sync::Arc}; +use alloc::sync::Arc; use vm_core::mast::MastNodeId; @@ -6,11 +6,9 @@ use super::GlobalProcedureIndex; use crate::{ ast::{ProcedureName, QualifiedProcedureName, Visibility}, diagnostics::{SourceManager, SourceSpan, Spanned}, - AssemblyError, LibraryPath, RpoDigest, + LibraryPath, RpoDigest, }; -pub type CallSet = BTreeSet; - // PROCEDURE CONTEXT // ================================================================================================ @@ -23,7 +21,6 @@ pub struct ProcedureContext { visibility: Visibility, is_kernel: bool, num_locals: u16, - callset: CallSet, } // ------------------------------------------------------------------------------------------------ @@ -44,7 +41,6 @@ impl ProcedureContext { visibility, is_kernel, num_locals: 0, - callset: Default::default(), } } @@ -93,38 +89,6 @@ impl ProcedureContext { // ------------------------------------------------------------------------------------------------ /// State mutators impl ProcedureContext { - pub fn insert_callee(&mut self, callee: RpoDigest) { - self.callset.insert(callee); - } - - pub fn extend_callset(&mut self, callees: I) - where - I: IntoIterator, - { - self.callset.extend(callees); - } - - /// Registers a call to an externally-defined procedure which we have previously compiled. - /// - /// The call set of the callee is added to the call set of the procedure we are currently - /// compiling, to reflect that all of the code reachable from the callee is by extension - /// reachable by the caller. - pub fn register_external_call( - &mut self, - callee: &Procedure, - inlined: bool, - ) -> Result<(), AssemblyError> { - // If we call the callee, it's callset is by extension part of our callset - self.extend_callset(callee.callset().iter().cloned()); - - // If the callee is not being inlined, add it to our callset - if !inlined { - self.insert_callee(callee.mast_root()); - } - - Ok(()) - } - /// Transforms this procedure context into a [Procedure]. /// /// The passed-in `mast_root` defines the MAST root of the procedure's body while @@ -138,7 +102,6 @@ impl ProcedureContext { pub fn into_procedure(self, mast_root: RpoDigest, mast_node_id: MastNodeId) -> Procedure { Procedure::new(self.name, self.visibility, self.num_locals as u32, mast_root, mast_node_id) .with_span(self.span) - .with_callset(self.callset) } } @@ -170,8 +133,6 @@ pub struct Procedure { mast_root: RpoDigest, /// The MAST node id which resolves to the above MAST root. body_node_id: MastNodeId, - /// The set of MAST roots called by this procedure - callset: CallSet, } // ------------------------------------------------------------------------------------------------ @@ -191,7 +152,6 @@ impl Procedure { num_locals, mast_root, body_node_id, - callset: Default::default(), } } @@ -199,11 +159,6 @@ impl Procedure { self.span = span; self } - - pub(crate) fn with_callset(mut self, callset: CallSet) -> Self { - self.callset = callset; - self - } } // ------------------------------------------------------------------------------------------------ @@ -244,12 +199,6 @@ impl Procedure { pub fn body_node_id(&self) -> MastNodeId { self.body_node_id } - - /// Returns a reference to a set of all procedures (identified by their MAST roots) which may - /// be called during the execution of this procedure. - pub fn callset(&self) -> &CallSet { - &self.callset - } } impl Spanned for Procedure { diff --git a/assembly/src/diagnostics.rs b/assembly/src/diagnostics.rs index 196c7f733a..45a5222730 100644 --- a/assembly/src/diagnostics.rs +++ b/assembly/src/diagnostics.rs @@ -83,12 +83,12 @@ impl From