Plafer 1122 fix decorators #1466

plafer · 2024-08-26T14:17:27Z

Based on #1473

Note that I didn't left DecoratorList in a similar format as it was (instead of the proposed DecoratorSpan) initially to change fewer things in this PR. But then it's not clear to me that we would gain much by using DecoratorSpan, since IIUC, it is useful when there are a lot of decorators sit next to each other. Especially after #1457, I don't think this would be very frequent.

…astNodeId>, Vec<DecoratorId>>` Still WIP on fixing `compile_body()` - specifically handling what to do when `make_basic_block()` returns a list of decorators

…lder-remove-uniqueness

…er-1122-fix-decorators

bobbinth · 2024-09-08T05:26:43Z

One thing we may need to add to this PR (or to a follow-up PR) is related to #1457 (comment). Basically, when we compute eq_hash of a basic block, we should include error codes attached to assert operations into it. This is actually causing some real issues in miden-base as sometimes error codes don't get reported properly.

* Move strings data to its own buffer * move StringTableBuilder out of BasicBlockDataBuilder * Create `StringTable` type * Encode decorators separately * fix strings table * Introduce Operation iterator in BasicBlockNode * Document use of `MAX_DECORATORS` value * fix typo * merge `basic_block_data_decoder` and `basic_block_data_builder` modules * Use `DecoratorId::from_u32_safe` in deserialization * serialization failing test * serialize before_enter/after_exit maps * Document serialization format * rename `BasicBlockNode::operation_iter` * `MastNodeInfo::new()`: soft enforce 0 values for non-basic block nodes * update docstring for `BasicBlockDataDecoder` * decorator id deserializing meaningful errors * move `basic_block_data_decoder` var * Don't use maps for before enter/after exit decorators

plafer · 2024-09-10T18:13:38Z

One thing we may need to add to this PR (or to a follow-up PR) is related to #1457 (comment). Basically, when we compute eq_hash of a basic block, we should include error codes attached to assert operations into it. This is actually causing some real issues in miden-base as sometimes error codes don't get reported properly.

The u32 argument to Assert and U32assert2 are now incorporated into the eq_hash computation (see 20446b7).

bobbinth · 2024-09-10T18:16:21Z

The u32 argument to Assert and U32assert2 are now incorporated into the eq_hash computation (see 20446b7).

Nice! There is one other operation which has carries an error code: MpVerify. Could we include it as well?

plafer · 2024-09-10T19:18:41Z

Nice! There is one other operation which has carries an error code: MpVerify. Could we include it as well?

Ah yes - done (adbfe9a)

bobbinth

All looks good! Thank you! I left one comment for the future inline.

bobbinth · 2024-09-10T20:51:03Z

assembly/src/assembler/mod.rs

+                    let then_blk = self.compile_body(
+                        then_blk.iter(),
+                        proc_ctx,
+                        None,
+                        block_builder.mast_forest_builder_mut(),
+                    )?;


Not for this PR, but in the future we could move wrapper into ProcedureContext and then we won't need to pass None here and in other similar places.

bobbinth · 2024-09-10T23:12:58Z

btw, as a result of this PR we should also be able to close #832. I think the resolution is that we don't allow procedures with just decorators.

bitwalker · 2024-09-11T16:32:26Z

assembly/src/assembler/mod.rs

+            };
+
+        let procedure_body_id = if body_node_ids.is_empty() {
+            // We cannot allow only decorators in a procedure body, since decorators don't change


This should probably be documented in the Miden documentation, specifically which instructions are decorators, and the fact that they cannot appear in a procedure by themselves.

I'm not sure how likely it is in practice, but I imagine the error would be pretty confusing.

This can probably go into the Procedures section. And we can refer to the a advice injectors as well as list emit, debug, and trace instructions (so that we don't have to list all advice injectors).

Updated the docs 2e8481e. With #1457 we can fix it more holistically - the "decorators" name could be removed in favor of "debug instructions" or whatnot, and here in the "debug instructions" section we can state that these do not modify the MAST root, can be stripped away without affecting the behavior of the program, and cannot be the only instructions in a procedure

bitwalker · 2024-09-11T16:41:11Z

assembly/src/errors.rs

+    #[error("invalid procedure: body must contain at least one instruction if it has decorators")]
+    #[diagnostic()]
+    EmptyProcedureBodyWithDecorators {
+        span: SourceSpan,


This is an example of an error where using Report and ad-hoc diagnostics works better than a concrete error type like AssemblyError. This error sets the stage for what is wrong, but is sort of incomplete from the user perspective. Ideally, you'd have the primary label span the procedure body, with a secondary label spanning the first decorator, with a message like '<opcode>' is a decorator, which cannot appear in a procedure body without at least one non-decorator instruction, with help text attached to the diagnostic like try adding a 'nop' instruction at the end of the procedure body to anchor the decorators.

By using AssemblyError for things like this, we make it difficult (if not impossible) to construct these kind of helpful diagnostics, which is why most APIs are designed to return Report rather than AssemblyError.

Your call on whether you want to make the change, but we should try to get in the habit of thinking about how these errors will be seen and acted on by users, particularly those that don't have as strong a familiarity with Miden Assembly, and may not understand the difference between things like decorators and instructions, since they appear the same way in the source code.

I echoed this comment in #1431 so that we can address it when removing AssemblyError.

bitwalker · 2024-09-11T16:51:57Z

core/src/mast/node/basic_block_node/mod.rs

@@ -231,7 +296,7 @@ impl PrettyPrint for BasicBlockNode {
    }
 }

-impl fmt::Display for BasicBlockNode {
+impl<'a> fmt::Display for BasicBlockNodePrettyPrint<'a> {


FYI Display is implemented for dyn PrettyPrint, so a reference to any PrettyPrint impl automatically implements Display. Can still be convenient to impl Display explicitly, like done here, but figured it's useful info to have on hand anyway.

bitwalker · 2024-09-11T16:59:31Z

core/src/mast/node/join_node.rs

@@ -18,6 +19,8 @@ use crate::{
 pub struct JoinNode {
    children: [MastNodeId; 2],
    digest: RpoDigest,
+    before_enter: Vec<DecoratorId>,


Looks like most places where we are using Vec<DecoratorId> here could use Box<[DecoratorId]> instead, and shave off 8 bytes per field (on 64-bit systems). Doesn't look like we ever mutate them in-place, just create/replace. AFAIK Box::from([]) as Box<[DecoratorId]> won't actually allocate anything.

Not something we need to change in this PR necessarily, but that'd be the first thing I'd do to reduce the size of MastNode when we start trying to trim down the size.

Yeah good point. I won't address it now, since these might or might not go away when we investigate reducing the size of MastNode - but good to keep in mind.

bitwalker

Looks good! Left a few notes/suggestions, but nothing that needs to block merging this. Nice work!

plafer added 27 commits August 23, 2024 08:45

feat: move decorators to MastForest

a25dc74

DecoratorList uses DecoratorId

5a15e68

feat: add before_enter and after_exit decorators to MAST nodes

68a0743

WIP: MastForestBuilder::make_basic_block() returns `Either<Option<M…

53c4a84

…astNodeId>, Vec<DecoratorId>>` Still WIP on fixing `compile_body()` - specifically handling what to do when `make_basic_block()` returns a list of decorators

Merge branch 'next' into plafer-1122-fix-decorators

904d47f

fix build

a69a19c

Merge branch 'plafer-1122-fix-decorators' into plafer-mast-forest-bui…

2462e1d

…lder-remove-uniqueness

fix build after merge

f488c80

fix repeat statement

2c77cbe

implement before_enter and after_exit

6788e97

changelog

5593c15

fix no_std

fcbb30e

print decorators in BasicBlockNode

0833c42

Merge branch 'plafer-mast-forest-builder-remove-uniqueness' into plaf…

8514ca9

…er-1122-fix-decorators

join node pretty print

32ff3b1

basic block and repeat tests

0323246

join decorators test

58626da

implement pretty print for all nodes

9422897

call node pretty print

6512af3

dyn node pretty print

8f95a7b

decorators external node

7ed5bee

join and split node pretty print

3cb7379

fix eq_hash, and test decorators_repeat_split

4d283e1

MastForestBuilder: add_decorator -> ensure_decorator

c863a19

fix asmop hash

642e5bf

fix docs

7266eba

fmt

ad0341f

plafer marked this pull request as ready for review August 30, 2024 18:47

plafer requested review from bobbinth and bitwalker August 30, 2024 18:47

plafer mentioned this pull request Sep 6, 2024

Fix MastForest serialization #1482

Merged

plafer added 4 commits September 10, 2024 09:01

Merge branch 'next' into plafer-1122-fix-decorators

f817cfc

block_builder var name

dbb3a7d

Introduce new EqHash

2d97beb

plafer mentioned this pull request Sep 10, 2024

Investigate reducing the size of MastNode to 64 bytes #1490

Open

Cleanup BasicBlockBuilder::make_block()

4007733

plafer mentioned this pull request Sep 10, 2024

Modify DecoratorIterator to return &Decorator instead of DecoratorId #1491

Closed

plafer force-pushed the plafer-1122-fix-decorators branch from 831c1db to 20446b7 Compare September 10, 2024 18:12

plafer requested a review from bobbinth September 10, 2024 18:13

Incorporate assert and mpverify arguments into eq_hash

adbfe9a

plafer force-pushed the plafer-1122-fix-decorators branch from 20446b7 to adbfe9a Compare September 10, 2024 19:18

plafer mentioned this pull request Sep 10, 2024

Refactor debug iterator #757

Open

bobbinth approved these changes Sep 10, 2024

View reviewed changes

bitwalker reviewed Sep 11, 2024

View reviewed changes

bitwalker approved these changes Sep 11, 2024

View reviewed changes

bobbinth mentioned this pull request Sep 11, 2024

Investigate removing AssemblyError enum #1431

Open

update code organization docs

2e8481e

plafer merged commit 4ba0c53 into next Sep 11, 2024
9 checks passed

plafer deleted the plafer-1122-fix-decorators branch September 11, 2024 20:35

This was referenced Sep 11, 2024

Decorator bug report #1122

Closed

Simple program causing panic #832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plafer 1122 fix decorators #1466

Plafer 1122 fix decorators #1466

plafer commented Aug 26, 2024 •

edited

Loading

bobbinth commented Sep 8, 2024

plafer commented Sep 10, 2024

bobbinth commented Sep 10, 2024

plafer commented Sep 10, 2024 •

edited

Loading

bobbinth left a comment

bobbinth Sep 10, 2024

bobbinth commented Sep 10, 2024

bitwalker Sep 11, 2024

bobbinth Sep 11, 2024

plafer Sep 11, 2024

bitwalker Sep 11, 2024

bobbinth Sep 11, 2024

bitwalker Sep 11, 2024

bitwalker Sep 11, 2024

plafer Sep 11, 2024

bitwalker left a comment

Plafer 1122 fix decorators #1466

Plafer 1122 fix decorators #1466

Conversation

plafer commented Aug 26, 2024 • edited Loading

bobbinth commented Sep 8, 2024

plafer commented Sep 10, 2024

bobbinth commented Sep 10, 2024

plafer commented Sep 10, 2024 • edited Loading

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwalker left a comment

Choose a reason for hiding this comment

plafer commented Aug 26, 2024 •

edited

Loading

plafer commented Sep 10, 2024 •

edited

Loading