Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove procedure cache from the assembler #1411

Merged
merged 3 commits into from
Jul 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
- Introduced `MastForestError` to enforce `MastForest` node count invariant (#1394)
- Added functions to `MastForestBuilder` to allow ensuring of nodes with fewer LOC (#1404)
- Make `Assembler` single-use (#1409)
- Remove `ProcedureCache` from the assembler (#1411).

#### Changed

Expand Down
15 changes: 7 additions & 8 deletions assembly/src/assembler/basic_block_builder.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
use crate::AssemblyError;

use super::{
context::ProcedureContext, mast_forest_builder::MastForestBuilder, BodyWrapper, Decorator,
DecoratorList, Instruction,
mast_forest_builder::MastForestBuilder, BodyWrapper, Decorator, DecoratorList, Instruction,
ProcedureContext,
};
use alloc::{borrow::Borrow, string::ToString, vec::Vec};
use vm_core::{
mast::{MastForestError, MastNodeId},
AdviceInjector, AssemblyOp, Operation,
};
use vm_core::{mast::MastNodeId, AdviceInjector, AssemblyOp, Operation};

// BASIC BLOCK BUILDER
// ================================================================================================
Expand Down Expand Up @@ -129,7 +128,7 @@ impl BasicBlockBuilder {
pub fn make_basic_block(
&mut self,
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, MastForestError> {
) -> Result<Option<MastNodeId>, AssemblyError> {
if !self.ops.is_empty() {
let ops = self.ops.drain(..).collect();
let decorators = self.decorators.drain(..).collect();
Expand Down Expand Up @@ -157,7 +156,7 @@ impl BasicBlockBuilder {
pub fn try_into_basic_block(
mut self,
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, MastForestError> {
) -> Result<Option<MastNodeId>, AssemblyError> {
self.ops.append(&mut self.epilogue);
self.make_basic_block(mast_forest_builder)
}
Expand Down
139 changes: 0 additions & 139 deletions assembly/src/assembler/context.rs

This file was deleted.

8 changes: 4 additions & 4 deletions assembly/src/assembler/id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ use crate::ast::ProcedureIndex;
/// </div>
///
/// In addition to the [super::ModuleGraph], these indices are also used with an instance of a
/// [super::ProcedureCache]. This is because the [super::ModuleGraph] and [super::ProcedureCache]
/// instances are paired, i.e. the [super::ModuleGraph] stores the syntax trees and call graph
/// analysis for a program, while the [super::ProcedureCache] caches the compiled
/// [super::Procedure]s for the same program, as derived from the corresponding graph.
/// [super::MastForestBuilder]. This is because the [super::ModuleGraph] and
/// [super::MastForestBuilder] instances are paired, i.e. the [super::ModuleGraph] stores the syntax
/// trees and call graph analysis for a program, while the [super::MastForestBuilder] caches the
/// compiled [super::Procedure]s for the same program, as derived from the corresponding graph.
///
/// This is intended for use when we are doing global inter-procedural analysis on a (possibly
/// growable) set of modules. It is expected that the index of a module in the set, as well as the
Expand Down
2 changes: 1 addition & 1 deletion assembly/src/assembler/instruction/env_ops.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{mem_ops::local_to_absolute_addr, push_felt, BasicBlockBuilder};
use crate::{assembler::context::ProcedureContext, AssemblyError, Felt, Spanned};
use crate::{assembler::ProcedureContext, AssemblyError, Felt, Spanned};
use vm_core::Operation::*;

// CONSTANT INPUTS
Expand Down
2 changes: 1 addition & 1 deletion assembly/src/assembler/instruction/field_ops.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::{validate_param, BasicBlockBuilder};
use crate::{
assembler::context::ProcedureContext,
assembler::ProcedureContext,
diagnostics::{RelatedError, Report},
AssemblyError, Felt, Span, MAX_EXP_BITS, ONE, ZERO,
};
Expand Down
2 changes: 1 addition & 1 deletion assembly/src/assembler/instruction/mem_ops.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{push_felt, push_u32_value, validate_param, BasicBlockBuilder};
use crate::{assembler::context::ProcedureContext, diagnostics::Report, AssemblyError};
use crate::{assembler::ProcedureContext, diagnostics::Report, AssemblyError};
use alloc::string::ToString;
use vm_core::{Felt, Operation::*};

Expand Down
6 changes: 3 additions & 3 deletions assembly/src/assembler/instruction/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::{
ast::InvokeKind, context::ProcedureContext, mast_forest_builder::MastForestBuilder, Assembler,
BasicBlockBuilder, Felt, Instruction, Operation, ONE, ZERO,
ast::InvokeKind, mast_forest_builder::MastForestBuilder, Assembler, BasicBlockBuilder, Felt,
Instruction, Operation, ProcedureContext, ONE, ZERO,
};
use crate::{diagnostics::Report, utils::bound_into_included_u64, AssemblyError};
use core::ops::RangeBounds;
Expand Down Expand Up @@ -391,7 +391,7 @@ impl Assembler {
Instruction::DynExec => return self.dynexec(mast_forest_builder),
Instruction::DynCall => return self.dyncall(mast_forest_builder),
Instruction::ProcRef(ref callee) => {
self.procref(callee, proc_ctx, span_builder, mast_forest_builder.forest())?
self.procref(callee, proc_ctx, span_builder, mast_forest_builder)?
}

// ----- debug decorators -------------------------------------------------------------
Expand Down
35 changes: 17 additions & 18 deletions assembly/src/assembler/instruction/procedures.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use super::{Assembler, BasicBlockBuilder, Operation};
use crate::{
assembler::{context::ProcedureContext, mast_forest_builder::MastForestBuilder},
assembler::{mast_forest_builder::MastForestBuilder, ProcedureContext},
ast::{InvocationTarget, InvokeKind},
AssemblyError, RpoDigest, SourceSpan, Spanned,
};

use smallvec::SmallVec;
use vm_core::mast::{MastForest, MastNodeId};
use vm_core::mast::MastNodeId;

/// Procedure Invocation
impl Assembler {
Expand All @@ -18,7 +18,7 @@ impl Assembler {
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, AssemblyError> {
let span = callee.span();
let digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder.forest())?;
let digest = self.resolve_target(kind, callee, proc_ctx, mast_forest_builder)?;
self.invoke_mast_root(kind, span, digest, proc_ctx, mast_forest_builder)
}

Expand All @@ -31,12 +31,11 @@ impl Assembler {
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, AssemblyError> {
// Get the procedure from the assembler
let cache = &self.procedure_cache;
let current_source_file = proc_ctx.source_file();

// If the procedure is cached, register the call to ensure the callset
// is updated correctly.
match cache.get_by_mast_root(&mast_root) {
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
//
Expand Down Expand Up @@ -69,11 +68,9 @@ impl Assembler {
})
}
})?;
proc_ctx.register_external_call(&proc, false, mast_forest_builder.forest())?;
}
Some(proc) => {
proc_ctx.register_external_call(&proc, false, mast_forest_builder.forest())?
proc_ctx.register_external_call(&proc, false)?;
}
Some(proc) => proc_ctx.register_external_call(&proc, false)?,
None if matches!(kind, InvokeKind::SysCall) => {
return Err(AssemblyError::UnknownSysCallTarget {
span,
Expand All @@ -91,7 +88,7 @@ impl Assembler {
// 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 mast_forest_builder.find_procedure_root(mast_root) {
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
Expand All @@ -101,7 +98,7 @@ impl Assembler {
}
}
InvokeKind::Call => {
let callee_id = match mast_forest_builder.find_procedure_root(mast_root) {
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
Expand All @@ -113,7 +110,7 @@ impl Assembler {
mast_forest_builder.ensure_call(callee_id)?
}
InvokeKind::SysCall => {
let callee_id = match mast_forest_builder.find_procedure_root(mast_root) {
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
Expand Down Expand Up @@ -158,23 +155,25 @@ impl Assembler {
callee: &InvocationTarget,
proc_ctx: &mut ProcedureContext,
span_builder: &mut BasicBlockBuilder,
mast_forest: &MastForest,
mast_forest_builder: &MastForestBuilder,
) -> Result<(), AssemblyError> {
let digest = self.resolve_target(InvokeKind::Exec, callee, proc_ctx, mast_forest)?;
self.procref_mast_root(digest, proc_ctx, span_builder, mast_forest)
let digest =
self.resolve_target(InvokeKind::Exec, callee, proc_ctx, mast_forest_builder)?;
self.procref_mast_root(digest, proc_ctx, span_builder, mast_forest_builder)
}

fn procref_mast_root(
&self,
mast_root: RpoDigest,
proc_ctx: &mut ProcedureContext,
span_builder: &mut BasicBlockBuilder,
mast_forest: &MastForest,
mast_forest_builder: &MastForestBuilder,
) -> Result<(), AssemblyError> {
// Add the root to the callset to be able to use dynamic instructions
// with the referenced procedure later
if let Some(proc) = self.procedure_cache.get_by_mast_root(&mast_root) {
proc_ctx.register_external_call(&proc, false, mast_forest)?;

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
Expand Down
2 changes: 1 addition & 1 deletion assembly/src/assembler/instruction/u32_ops.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::{field_ops::append_pow2_op, push_u32_value, validate_param, BasicBlockBuilder};
use crate::{
assembler::context::ProcedureContext,
assembler::ProcedureContext,
diagnostics::{RelatedError, Report},
AssemblyError, Span, MAX_U32_ROTATE_VALUE, MAX_U32_SHIFT_VALUE,
};
Expand Down
Loading
Loading