Fix folded branch hints - #2578
Conversation
The branch hint annotation `(@metadata.code.branch_hint ...)` records the instruction it applies to by index. This index was recorded eagerly as the index of the next instruction to be pushed, which is correct for the flat form (e.g. `... (@...) if`) but wrong for the folded form (e.g. `(@...) (if (cond) ...)`): there the operands of the folded instruction are pushed first and the head instruction (the `if` or `br_if`) is pushed afterwards, so the hint ended up attached to the first operand instead of the branch instruction. Defer the index assignment instead: keep the parsed annotation pending and attach it to the head instruction of the following (flat or folded) form when that instruction is actually pushed. This also accepts the annotation appearing inside the folded form, after the operands (e.g. `(if (cond) (@...) (then ...))` or `(br_if ... (@...))`). Using both a preceding and a trailing annotation on one folded instruction targets it twice and is rejected as a duplicate, matching two adjacent annotations.
The folded printer emitted the `(@metadata.code.branch_hint ...)` annotation inside the folded form: as a pseudo-operand between an `if`'s condition and its `(then`, and after a `br_if`'s operand. The annotation applies to the `if` / `br_if` instruction, so print it on its own line immediately before that instruction instead, matching the flat printer and the natural input form.
Cover the folded "inside" placement of branch hint annotations (between an `if`'s condition and its `(then`, and after a `br_if`'s operand). This is the form older versions of this tool emitted when printing folded, so the parser must keep accepting it and attach the hint to the `if` / `br_if`. Nothing else in the test suite exercises this now that the folded printer emits the annotation before the instruction instead. Also assert that a preceding and a trailing annotation on one folded instruction are rejected as a duplicate.
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Seems reasonable to me but I think @alexcrichton should probably take a look, since I've really only done minor work on the wast crate
|
Thank you for fixing this! Now that branch-hints is merged and there's a normative spec, I would be in favor making the (func
(@metadata.code.branch_hint "\00")
)(func
(@metadata.code.branch_hint "\00")
nop
)(func
(if (i32.const 0) (@metadata.code.branch_hint "\00")
(then))
)Currently (pre and post this PR) wasm-tools accepts these as well-formed and valid. I think it probably should reject them too... and this will make it easier to catch future issues when it does the roundtrip tests. |
|
Thanks! I agree with @keithw that there's no need to keep parsing the old syntax and it's ok to just break things here. I think there's code in I'll note though that @keithw syntax like: (func
(@metadata.code.branch_hint "\00")
nop
)will be relatively difficult to reject I think as that requires running the validator to determine that it's not actually tied to an |
4 participants
This fixes the parsing and printing of branch hints, so that the hint is attached to the
ifand not to thelocal.getin the example below (from the test suite).The form previously emitted (with the branch hint right before the
then) is still accepted on parsing.