Skip to content

refactor: create 1-1 mapping between operations and AsmOp decorators #2455

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.

Sign up for GitHub

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

Open
PivasDesant wants to merge 15 commits into
base: next
Choose a base branch
from
+331 −38
Open

refactor: create 1-1 mapping between operations and AsmOp decorators #2455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
50b4228
Update basic_block_builder.rs
PivasDesant Dec 11, 2025
55f0ab3
Update mod.rs
PivasDesant Dec 11, 2025
99922a4
Update mod.rs
PivasDesant Dec 11, 2025
e9bbb32
Update tests.rs
PivasDesant Dec 19, 2025
f32cb03
Update mod.rs
PivasDesant Dec 19, 2025
f2c8f1b
Update Cargo.toml
PivasDesant Dec 19, 2025
075947f
test fix and fmt
PivasDesant Dec 23, 2025
6715b6e
Update mod.rs
PivasDesant Dec 29, 2025
1271400
Update mod.rs
PivasDesant Dec 29, 2025
8bf769f
Update tests.rs
PivasDesant Dec 29, 2025
501f9b0
Update mod.rs
PivasDesant Jan 5, 2026
6136116
Update mod.rs
PivasDesant Jan 5, 2026
3fd08b0
test: update snapshots for 1-1 operation-AsmOp mapping
huitseeker Jan 20, 2026
dc7edbe
chore: changelog for PR 2455
huitseeker Jan 20, 2026
8961202
fix: use BTreeMap instead of HashMap for no-std compatibility
huitseeker Jan 20, 2026
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
4 changes: 2 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Added `procedure_names` to `DebugInfo` for storing procedure name mappings by MAST root digest, enabling debuggers to resolve human-readable procedure names during execution (#[2474](https://github.com/0xMiden/miden-vm/pull/2474)).
- Added constants support as an immediate value of the repeat statement ([#2548](https://github.com/0xMiden/miden-vm/pull/2548)).
- Established 1-1 mapping between operations and AsmOp decorators, linking every operation to its corresponding assembly instruction in multi-operation sequences ([#2455](https://github.com/0xMiden/miden-vm/pull/2455)).

#### Fixes

Expand Down Expand Up @@ -38,11 +39,10 @@
- Updated documentation URLs from mdBook to docs.miden.xyz ([#2560](https://github.com/0xMiden/miden-vm/pull/2560)).
- Add API to serialize the `MastForest` without `DebugInfo` ([#2549](https://github.com/0xMiden/miden-vm/pull/2549)).
- Removed `ErrorContext` trait and `err_ctx!` macro; error context is now computed lazily by passing raw parameters to error extension traits ([#2544](https://github.com/0xMiden/miden-vm/pull/2544)).

- Use `IndexVec::try_from` instead of pushing elements one by one in `DebugInfo::empty_for_nodes` ([#2559](https://github.com/0xMiden/miden-vm/pull/2559)).
- [BREAKING] Remove `NodeExecutionState` in favor of `Continuation` ([#2587](https://github.com/0xMiden/miden-vm/pull/2587)).

- Made `StackInputs` and `StackOutputs` implement `Copy` trait ([#2581](https://github.com/0xMiden/miden-vm/pull/2581)).

## 0.20.2 (TBD)
- Fix issue where decorator access was not bypassed properly in release mode ([#2529](https://github.com/0xMiden/miden-vm/pull/2529)).

Expand Down
64 changes: 52 additions & 12 deletions core/src/mast/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,18 +602,58 @@ impl MastForest {
// If a target operation index is provided, return the assembly op associated with that
// operation.
Some(target_op_idx) => {
for (op_idx, decorator_id) in decorator_links {
if let Some(Decorator::AsmOp(assembly_op)) = self.decorator_by_id(decorator_id)
{
// when an instruction compiles down to multiple operations, only the first
// operation is associated with the assembly op. We need to check if the
// target operation index falls within the range of operations associated
// with the assembly op.
if target_op_idx >= op_idx
&& target_op_idx < op_idx + assembly_op.num_cycles() as usize
{
return Some(assembly_op);
// With the 1-1 mapping, every operation covered by an AsmOp is explicitly linked
// to that AsmOp. We need to find the minimum op_idx for each unique AsmOp decorator
// to determine the start of its range, then check if target_op_idx falls within
// that range.
let mut asmop_ranges: BTreeMap<DecoratorId, (usize, usize)> = BTreeMap::new();
Copy link
Contributor

@huitseeker huitseeker Jan 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the 1-1 mapping now in place, could we simplify this function? The current implementation builds a BTreeMap and does two passes, but since every operation is now directly linked to its AsmOp, we can query by index directly.

Something like:

pub fn get_assembly_op(
    &self,
    node_id: MastNodeId,
    target_op_idx: Option<usize>,
) -> Option<&AssemblyOp> {
    let node = &self[node_id];

    let find_asmop = |ids: &[DecoratorId]| {
        ids.iter().find_map(|&id| match &self[id] {
            Decorator::AsmOp(asm_op) => Some(asm_op),
            _ => None,
        })
    };

    match target_op_idx {
        Some(idx) => match node {
            MastNode::Block(_) => find_asmop(self.decorators_for_operation(node_id, idx)),
            _ => match idx {
                0 => find_asmop(self.before_enter_decorators(node_id)),
                1 => find_asmop(self.after_exit_decorators(node_id)),
                _ => None,
            },
        },

        None => {
            // Search order: before_enter, then per-op (for blocks), then after_exit
            find_asmop(self.before_enter_decorators(node_id))
                .or_else(|| match node {
                    MastNode::Block(block) => (0..block.num_operations() as usize)
                        .find_map(|i| find_asmop(self.decorators_for_operation(node_id, i))),
                    _ => None,
                })
                .or_else(|| find_asmop(self.after_exit_decorators(node_id)))
        }
    }
}

This removes the Box<dyn Iterator> and BTreeMap, and the common case (Block + target index) becomes a direct lookup.

Copy link
Contributor

@plafer plafer Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still at a loss as to why we're doing this in the first place. Bobbin's answer to this question was that it would simplify the decorator finding logic - but comparing this with what we currently have, I find them pretty equally complex. IMO, most of the complexity comes from the fact that we have "before enter", "after exit", and sometimes "operations" decorators. The num_cycles check is not really the issue - it only applies to "operations" decorators, and is honestly pretty simple.

In contrast, the clear downside of this new approach is increased memory usage.

Am I missing something?

Copy link
Contributor

@huitseeker huitseeker Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it was worth it as an experiment which has shown it may be best to not push this one further? WDYT @bobbinth ?
Though I would add that the above is a bit complex to avoid allocations, and non-allocative code is always a bit involved.


// For Block nodes with target_op_idx, we should only consider operation-indexed
// decorators (not before_enter/after_exit) for operation-specific lookups
match node {
MastNode::Block(_) => {
// For Block nodes, use only operation-indexed decorators
let op_iter = self
.decorator_links_for_node(node_id)
.expect("Block node must have some valid set of decorator links");

for (op_idx, decorator_id) in op_iter {
if let Some(Decorator::AsmOp(assembly_op)) =
self.decorator_by_id(decorator_id)
{
let num_cycles = assembly_op.num_cycles() as usize;
asmop_ranges
.entry(decorator_id)
.and_modify(|(min_idx, cycles)| {
*min_idx = (*min_idx).min(op_idx);
*cycles = num_cycles; // num_cycles should be the same for same decorator_id
})
.or_insert((op_idx, num_cycles));
}
}
},
_ => {
// For non-Block nodes, before_enter is at index 0, after_exit is at index 1
// These are not operation ranges, so we check exact matches
for (op_idx, decorator_id) in decorator_links {
if op_idx == target_op_idx
&& let Some(Decorator::AsmOp(assembly_op)) =
self.decorator_by_id(decorator_id)
{
return Some(assembly_op);
}
}
},
}

// Second pass: check if target_op_idx falls within any AsmOp's range
for (decorator_id, (min_op_idx, num_cycles)) in asmop_ranges {
if target_op_idx >= min_op_idx
&& target_op_idx < min_op_idx + num_cycles
&& let Some(Decorator::AsmOp(assembly_op)) =
self.decorator_by_id(decorator_id)
{
return Some(assembly_op);
}
}
},
Expand Down Expand Up @@ -713,7 +753,7 @@ impl MastForest {
// TEST HELPERS
// ================================================================================================

#[cfg(test)]
#[cfg(any(test, feature = "testing"))]
impl MastForest {
/// Returns all decorators for a given node as a vector of (position, DecoratorId) tuples.
///
Expand Down
3 changes: 2 additions & 1 deletion core/src/mast/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,13 +869,14 @@ fn test_mast_forest_get_assembly_op_with_target_index() {
let decorator_id = forest.add_decorator(Decorator::AsmOp(assembly_op.clone())).unwrap();

// Add a basic block node with the decorator at index 2
// With 1-to-1 mapping, all operations covered by the AsmOp (indices 2, 3, 4) should be linked
let operations = vec![
Operation::Push(Felt::new(1)),
Operation::Push(Felt::new(2)),
Operation::Mul,
Operation::Add,
];
let decorators = vec![(2, decorator_id)]; // Decorator at operation index 2
let decorators = vec![(2, decorator_id), (3, decorator_id), (4, decorator_id)]; // Decorator linked to all operations it covers
let node_id = BasicBlockNodeBuilder::new(operations, decorators)
.add_to_forest(&mut forest)
.unwrap();
Expand Down
3 changes: 2 additions & 1 deletion core/src/operations/decorators/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,5 +87,6 @@ impl fmt::Display for Decorator {
/// index.
///
/// Note: for `AssemblyOp` decorators, when an instruction compiles down to multiple operations,
/// only the first operation is associated with the assembly op.
/// all operations are associated with the same assembly op, creating a 1-1 mapping between
/// operations and AsmOp decorators.
pub type DecoratorList = Vec<DecoratedOpLink>;
1 change: 1 addition & 0 deletions crates/assembly/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ thiserror.workspace = true
miden-assembly = { path = ".", default-features = false, features = ["testing"] }
miden-mast-package = { workspace = true, features = ["arbitrary"] }
# core lib and processor are imported using paths to avoid specifying dependency versions
miden-core = { path = "../../core", default-features = false, features = ["testing"] }
miden-core-lib = { path = "../lib/core", default-features = false }
miden-processor = { path = "../../processor", features = ["testing"] }
insta = { workspace = true }
Expand Down
19 changes: 15 additions & 4 deletions crates/assembly/src/basic_block_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,21 +138,24 @@ impl BasicBlockBuilder<'_> {
/// Computes the number of cycles elapsed since the last invocation of track_instruction() and
/// updates the related AsmOp decorator to include this cycle count.
///
/// Creates a 1-1 mapping between operations and AsmOp: every operation covered by the AsmOp
/// is linked to that AsmOp. This means if an instruction compiles to multiple operations, all
/// of those operations will be associated with the same AsmOp decorator.
///
/// If the cycle count is 0, the original decorator is removed from the list and returned. This
/// can happen for instructions which do not contribute any operations to the span block - e.g.,
/// exec, call, and syscall.
pub fn set_instruction_cycle_count(&mut self) -> Option<DecoratorId> {
// get the last asmop decorator and the cycle at which it was added
let (op_start, assembly_op_id) =
self.decorators.get_mut(self.last_asmop_pos).expect("no asmop decorator");
let (op_start, assembly_op_id) = self.decorators[self.last_asmop_pos];

let assembly_op = match &mut self.mast_forest_builder[*assembly_op_id] {
let assembly_op = match &mut self.mast_forest_builder[assembly_op_id] {
Decorator::AsmOp(assembly_op) => assembly_op,
_ => panic!("internal error: last asmop decorator is not an AsmOp"),
};

// compute the cycle count for the instruction
let cycle_count = self.ops.len() - *op_start;
let cycle_count = self.ops.len() - op_start;

// if the cycle count is 0, remove the decorator; otherwise update its cycle count
if cycle_count == 0 {
Expand All @@ -161,6 +164,14 @@ impl BasicBlockBuilder<'_> {
} else {
assembly_op.set_num_cycles(cycle_count as u8);

// Create a 1-1 mapping: link the AsmOp to all operations it covers.
// The first operation is already linked at op_start, so we add links for
// operations from op_start + 1 to op_start + cycle_count - 1.
// We insert them in reverse order to maintain the sorted order of the decorators list.
for op_idx in ((op_start + 1)..(op_start + cycle_count)).rev() {
self.decorators.insert(self.last_asmop_pos + 1, (op_idx, assembly_op_id));
}

None
}
}
Expand Down
4 changes: 3 additions & 1 deletion crates/assembly/src/snapshots/miden_assembly__tests__adv_has_map_key.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/assembly/src/tests.rs
assertion_line: 2803
assertion_line: 3026
expression: program
---
begin
Expand All @@ -11,7 +11,9 @@ begin
drop
asmOp(adv.has_mapkey, 3)
push(5642583036089175977)
asmOp(adv.has_mapkey, 3)
emit
asmOp(adv.has_mapkey, 3)
drop
asmOp(assert, 1)
assert(0)
Expand Down
7 changes: 6 additions & 1 deletion crates/assembly/src/snapshots/miden_assembly__tests__advmap_push.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/assembly/src/tests.rs
assertion_line: 2773
assertion_line: 2996
expression: program
---
begin
Expand All @@ -11,14 +11,19 @@ begin
drop
asmOp(push.[2,2,2,2], 4)
push(2)
asmOp(push.[2,2,2,2], 4)
push(2)
asmOp(push.[2,2,2,2], 4)
push(2)
asmOp(push.[2,2,2,2], 4)
push(2)
noop
noop
asmOp(adv.push_mapval, 3)
push(17843484659000820118)
asmOp(adv.push_mapval, 3)
emit
asmOp(adv.push_mapval, 3)
drop
asmOp(assert, 1)
assert(0)
Expand Down
6 changes: 6 additions & 0 deletions crates/assembly/src/snapshots/miden_assembly__tests__advmap_push_nokey.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/assembly/src/tests.rs
assertion_line: 3011
expression: program
---
begin
Expand All @@ -10,14 +11,19 @@ begin
drop
asmOp(push.[4841096222507812910,2461319744924557480,11072036045095845230,4645558446535935538], 4)
push(4645558446535935538)
asmOp(push.[4841096222507812910,2461319744924557480,11072036045095845230,4645558446535935538], 4)
push(11072036045095845230)
asmOp(push.[4841096222507812910,2461319744924557480,11072036045095845230,4645558446535935538], 4)
push(2461319744924557480)
asmOp(push.[4841096222507812910,2461319744924557480,11072036045095845230,4645558446535935538], 4)
push(4841096222507812910)
noop
noop
asmOp(adv.push_mapval, 3)
push(17843484659000820118)
asmOp(adv.push_mapval, 3)
emit
asmOp(adv.push_mapval, 3)
drop
asmOp(assert, 1)
assert(0)
Expand Down
5 changes: 4 additions & 1 deletion crates/assembly/src/snapshots/miden_assembly__tests__assert_eq_with_code.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/assembly/src/tests.rs
assertion_line: 1713
assertion_line: 1905
expression: program
---
begin
Expand All @@ -11,12 +11,15 @@ begin
drop
asmOp(assert_eq, 2)
eq
asmOp(assert_eq, 2)
assert(0)
asmOp(assert_eq.err="Oh no", 2)
eq
asmOp(assert_eq.err="Oh no", 2)
assert(15491226248792286710)
asmOp(assert_eq.err="Oh no", 2)
eq
asmOp(assert_eq.err="Oh no", 2)
assert(15491226248792286710)
end
end
32 changes: 31 additions & 1 deletion crates/assembly/src/snapshots/miden_assembly__tests__assert_eqw_with_code.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/assembly/src/tests.rs
assertion_line: 1737
assertion_line: 1929
expression: program
---
begin
Expand All @@ -11,39 +11,69 @@ begin
drop
asmOp(assert_eqw, 11)
movup4
asmOp(assert_eqw, 11)
eq
asmOp(assert_eqw, 11)
assert(0)
asmOp(assert_eqw, 11)
movup3
asmOp(assert_eqw, 11)
eq
asmOp(assert_eqw, 11)
assert(0)
asmOp(assert_eqw, 11)
movup2
asmOp(assert_eqw, 11)
eq
asmOp(assert_eqw, 11)
assert(0)
asmOp(assert_eqw, 11)
eq
asmOp(assert_eqw, 11)
assert(0)
asmOp(assert_eqw.err="Oh no", 11)
movup4
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
movup3
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
movup2
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
movup4
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
movup3
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
movup2
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
asmOp(assert_eqw.err="Oh no", 11)
eq
asmOp(assert_eqw.err="Oh no", 11)
assert(15491226248792286710)
noop
end
Expand Down
Loading