Skip to content

Conversation

@PivasDesant
Copy link

Implements 1-1 mapping for operations and AsmOp decorators (#2446)

Before this change, only the first operation was linked to an AsmOp when an instruction compiled to multiple operations. Now all operations are linked.

The implementation adds decorator links for all operations in set_instruction_cycle_count(). This also simplifies get_assembly_op() since we can check index equality directly instead of doing range checks.

No data duplication since we reuse the same DecoratorId for all operations covered by an AsmOp.

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! This change needs a test that goes through the assembly path to verify the 1-1 mapping.

Existing tests in core/src/mast/tests.rs manually construct decorators like vec![(2, decorator_id)], which doesn't test that set_instruction_cycle_count() creates the mapping.

I would suggest a test in this PR in crates/assembly/src/tests.rs that:

  1. Assembles MASM code with multi-cycle instructions
  2. Verifies all operations in a multi-cycle range share the same DecoratorId
  3. Uses MastForest::all_decorators() (which should be gated under the testing feature, and miden-core should be imported as a dev-dependency of miden-assembly activating that feature to get access to it)

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the test!

// ================================================================================================

#[cfg(test)]
#[cfg(feature = "testing")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using #[cfg(any(test, feature = "testing"))] instead of just #[cfg(feature = "testing")].

The current approach means that unit tests in the core crate itself cannot use all_decorators() when running cargo test without explicitly enabling the testing feature.

The suggested alternative is the standard pattern for test helpers that need to be shared across crates while still being available for local unit tests.

@PivasDesant
Copy link
Author

Thanks for the review! So i fixed test naming and also fixed fmt

@bobbinth
Copy link
Contributor

bobbinth commented Jan 4, 2026

@PivasDesant - seems like build/tests are currently failing on this PR. Would you be able to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants