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 uniqueness invariant from MastForestBuilder #1473

Merged
merged 57 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
6523811
`Assembler::invoke`: no longer returns an `Option`
plafer Aug 27, 2024
4c77063
`MastForestBuilder`: `ensure_node` -> `add_node`
plafer Aug 27, 2024
bc5cb74
`Library`: use `MastNodeId` as unique identifier for procedures
plafer Aug 27, 2024
7272a55
fix tests
plafer Aug 28, 2024
725eff5
add failing test
plafer Aug 28, 2024
62b4c52
use `MastNodeId` to identify procedures in ModuleGraph
plafer Aug 28, 2024
72a7093
ProcedureInfo: store `body_node_id`
plafer Aug 28, 2024
30d2933
`ResolvedProcedure`: store body_id when present
plafer Aug 28, 2024
c678944
create a new root node on exec
plafer Aug 28, 2024
a352fa2
Revert "ProcedureInfo: store `body_node_id`"
plafer Aug 28, 2024
5ae4fd5
fix build after `ProcedureInfo` revert
plafer Aug 28, 2024
de51e2c
Remove `ResolvedProcedure::BodyNodeId`
plafer Aug 28, 2024
03471eb
`Assembler::resolve_target` returns `Either<MastNodeId, RpoDigest>`
plafer Aug 28, 2024
bc045b6
fix failing test
plafer Aug 28, 2024
67a061a
fmt
plafer Aug 28, 2024
a3c5d8c
add back map digest -> GID to mast forest builder
plafer Aug 28, 2024
b45d459
`MastForestBuilder`: change all `ensure_*` to `add_*`
plafer Aug 28, 2024
961d6f6
fix regression
plafer Aug 28, 2024
a4cb422
fix failing test
plafer Aug 29, 2024
0665cf5
changelog
plafer Aug 29, 2024
ef307c2
docs
plafer Aug 29, 2024
cccf018
docs
plafer Aug 29, 2024
883356a
add test `ensure_unique_node_ids_for_identical_procedures`
plafer Aug 29, 2024
ecec03b
cleanup `MastForestBuilder`
plafer Aug 29, 2024
aa04649
remove `ensure_unique_node_ids_for_identical_procedures` test
plafer Aug 29, 2024
72a482b
`MastForestBuilder::eq_hash()`
plafer Aug 29, 2024
e18c4e7
`MastForestBuilder`: digest -> mast_root
plafer Aug 29, 2024
4a46d32
`MastForestBuilder`: don't duplicate equal nodes
plafer Aug 29, 2024
d959dd6
revert changes to tests
plafer Aug 29, 2024
187db6b
invoke: don't create new node needlessly
plafer Aug 29, 2024
53003b5
fix test
plafer Aug 29, 2024
126931e
fix docs
plafer Aug 29, 2024
45539ed
fmt
plafer Aug 29, 2024
ded33b4
cleanup `Assembler::invoke`
plafer Aug 29, 2024
c5e6b8b
remove stale TODOs
plafer Aug 29, 2024
485cbc8
fix doc tests
plafer Aug 29, 2024
191404a
fix no_std
plafer Aug 29, 2024
5b3285e
`TestContext`: don't use debug mode by default
plafer Aug 30, 2024
f141c76
fix asmop hash
plafer Aug 30, 2024
8dd1662
Remove redundant comments
plafer Sep 3, 2024
ca2d6d1
revert comment change
plafer Sep 3, 2024
1b619e6
fix docstring
plafer Sep 3, 2024
a30a9c7
`Library::new()`: check that every procedures are indeed exports
plafer Sep 3, 2024
3b280f1
rename `procedure_root_digest`
plafer Sep 3, 2024
41f45d4
fix docs
plafer Sep 3, 2024
ed09c6c
refactor `Assembler::resolve_target()`
plafer Sep 3, 2024
2125923
split `Assembler::get_proc_root_id_from_mast_root`
plafer Sep 3, 2024
d294d20
`Assembler::resolve_target` docstring
plafer Sep 3, 2024
43185ec
fmt
plafer Sep 3, 2024
4c139bb
`invoke`: document
plafer Sep 3, 2024
dd72f68
fmt
plafer Sep 3, 2024
72b9fe5
add `unwrap()` comment
plafer Sep 3, 2024
8243fa2
fmt
plafer Sep 3, 2024
4eeeb03
adjust `Assembler::invoke()` comment
plafer Sep 3, 2024
2ffd050
fix `Assembler::ensure_valid_procedure_mast_root`
plafer Sep 3, 2024
9b74a8b
cleanup proc alias
plafer Sep 3, 2024
245022e
fmt
plafer Sep 3, 2024
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
2 changes: 1 addition & 1 deletion assembly/src/assembler/basic_block_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
12 changes: 9 additions & 3 deletions assembly/src/assembler/instruction/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
90 changes: 66 additions & 24 deletions assembly/src/assembler/instruction/procedures.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand All @@ -16,10 +16,27 @@ impl Assembler {
callee: &InvocationTarget,
proc_ctx: &mut ProcedureContext,
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, AssemblyError> {
) -> Result<MastNodeId, AssemblyError> {
bobbinth marked this conversation as resolved.
Show resolved Hide resolved
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 {
// 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),
}
}

fn invoke_mast_root(
Expand All @@ -28,12 +45,12 @@ impl Assembler {
span: SourceSpan,
mast_root: RpoDigest,
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, AssemblyError> {
) -> Result<MastNodeId, AssemblyError> {
// 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(&mast_root) {
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
//
Expand All @@ -57,8 +74,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 that
// the entire module is in AST representation as well.
// AST of a procedure contained in it (i.e. `proc`). Hence, it must be
// thatthe entire module is in AST representation as well.
plafer marked this conversation as resolved.
Show resolved Hide resolved
if module.unwrap_ast().is_kernel() {
Ok(())
} else {
Expand All @@ -74,18 +91,36 @@ 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 => {
// 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)?
},
}
},
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.add_external(mast_root)?
},
}
},
Expand All @@ -95,36 +130,36 @@ 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) {
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.add_external(mast_root)?
},
};

mast_forest_builder.ensure_syscall(callee_id)?
mast_forest_builder.add_syscall(callee_id)?
},
}
};

Ok(Some(mast_root_node_id))
Ok(mast_root_node_id)
}

/// Creates a new DYN block for the dynamic code execution and return.
pub(super) fn dynexec(
&self,
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, AssemblyError> {
let dyn_node_id = mast_forest_builder.ensure_dyn()?;
let dyn_node_id = mast_forest_builder.add_dyn()?;

Ok(Some(dyn_node_id))
}
Expand All @@ -135,8 +170,8 @@ impl Assembler {
mast_forest_builder: &mut MastForestBuilder,
) -> Result<Option<MastNodeId>, 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))
Expand All @@ -149,8 +184,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)
}

Expand Down
Loading
Loading