feat(assembly): add trace for generating read-only events - #3478
Conversation
dcd2c84 to
220f6e2
Compare
|
Automated check (CONTRIBUTING.md) Findings:
Recommendations:
Next steps:
|
|
/cc @bobbinth @bitwalker |
| Self::Trace => const_text("trace"), | ||
| // Printing `TraceImm` such that it is consistent with `EmitImm`, even though this | ||
| // does not round-trip. `trace.<FELT_IMM>` is invalid. | ||
| Self::TraceImm(value) => inst_with_felt_imm("trace", value), |
There was a problem hiding this comment.
Formatting a parsed trace.event(...) now writes trace.<felt>, but this PR also rejects numeric trace immediates.
I reproduced this with trace.event("test::roundtrip"): the formatted module fails to parse with invalid instruction trace or malformed operands.
Could this print a valid equivalent such as push.<felt> trace drop, and cover it with a parse-format-parse test?
There was a problem hiding this comment.
The idea of printing as trace.<felt> was to be in line with emit.<felt>, see the comment displayed in the code snippet above. In 23fb57b I've changed it to print as push.<felt> trace drop and added a test to verify that this can be parsed back.
Should we open an issue to change the printing of EmitImm to push.<felt> emit drop so that it can be parsed back too? Though I assume that would be a breaking change.
| }, | ||
|
|
||
| ExtendedInstructionKind::Emit => lower_emit_instruction(context, span, &tokens), | ||
| ExtendedInstructionKind::Trace => lower_trace_instruction(context, span, &tokens), |
There was a problem hiding this comment.
nit: trace duplicates the full emit extended parser below, including constant lookup and event-name hashing.
THe instruction spec could carry the immediate constructor and use one lowering helper keyed by spec.keyword instead.
There was a problem hiding this comment.
THe instruction spec could carry the immediate constructor and use one lowering helper keyed by spec.keyword instead.
Not sure if I understand correctly, but Wouldn't that require changing ExtendedInstructionSpec?
For now I removed the duplication by lowering both through a new shared function: lower_event_imm_instruction.
|
|
||
| #### Features | ||
|
|
||
| - Added `trace`, `trace.CONST`, and `trace.event("...")` assembly as syntactic sugar for emitting optional read-only trace events ([#3478](https://github.com/0xMiden/miden-vm/pull/3478)). |
There was a problem hiding this comment.
This also adds Trace and TraceImm to the public exhaustive miden_assembly_syntax::ast::Instruction enum, so downstream exhaustive matches stop compiling. cargo semver-checks check-release -p miden-assembly-syntax --baseline-version 0.29.0 --release-type minor reports enum_variant_added for both variants.
Could we mark this entry [BREAKING]?
| Self::Trace => const_text("trace"), | ||
| // `trace.<FELT_IMM>` is invalid syntax, so to support a `print -> parse` round trip | ||
| // we print the equivalent `push.<value> trace drop` sequence instead. | ||
| Self::TraceImm(value) => { |
There was a problem hiding this comment.
nit: this repeats the Felt immediate rendering in inst_with_felt_imm just below. Could this be flatten(inst_with_felt_imm("push", value) + const_text(" trace drop"))?
2 participants
Closes #3373
For now I kept the variant with immediate consistent with
emit. Reasoning:trace.event("miden::mytrace")looks natural as we referred to it as "trace event"miden::mytraceis an event, so it fitsevent(...)