Wasmtime GitHub notifications bot (Jun 06 2025 at 19:56): Wasmtime GitHub notifications bot (Jun 06 2025 at 19:56):mmcloughlin opened PR #10962 from mmcloughlin:cranelift-isle-print to bytecodealliance:main:
This PR adds a printer to the ISLE library. This is intended to support tooling which generates ISLE source code. In such cases, this printer library allows applications to work with ISLE's AST types rather than raw string manipulation, and produce more readable output due to the pretty-printing features.
The printer is tested with a new integration test that verifies the print-parse roundtrip property for provided ISLE files.
Wasmtime GitHub notifications bot (Jun 06 2025 at 19:56):mmcloughlin requested alexcrichton for a review on PR #10962.
Wasmtime GitHub notifications bot (Jun 06 2025 at 19:56):mmcloughlin requested wasmtime-compiler-reviewers for a review on PR #10962.
Wasmtime GitHub notifications bot (Jun 06 2025 at 19:59):mmcloughlin submitted PR review.
Wasmtime GitHub notifications bot (Jun 06 2025 at 19:59):mmcloughlin created PR review comment:
Previous discussion still applies: https://github.com/bytecodealliance/wasmtime/pull/8263#discussion_r1543966705. I still tend to think this is the simplest of the options.
@cfallin regarding the suggestion "a helper that maps through all AST nodes and sets positions to e.g. zero?". I feel like this could be quite a bit of boilerplate to implement. Happy to do that if you prefer.
Wasmtime GitHub notifications bot (Jun 06 2025 at 20:46):alexcrichton commented on PR #10962:
redirecting review to @cfallin who I believe has more context on this
Wasmtime GitHub notifications bot (Jun 06 2025 at 20:46):alexcrichton requested cfallin for a review on PR #10962.
Wasmtime GitHub notifications bot (Jun 06 2025 at 21:01):cfallin submitted PR review:
Looks generally good -- thanks for writing this out! A few things below but nothing too bad:
Wasmtime GitHub notifications bot (Jun 06 2025 at 21:01):cfallin created PR review comment:
Unfortunately we can't refer to one crate from another with paths like this: the tests have to work even when the crates are vendored or in some other directory layout. Can you run the print-tests over the existing ISLE test files we have in this crate?
Wasmtime GitHub notifications bot (Jun 06 2025 at 21:01):cfallin created PR review comment:
Rather than a setter, can we have an alternative constructor, e.g.
Parser::new_without_pos_tracking()?
Wasmtime GitHub notifications bot (Jun 06 2025 at 21:01):cfallin created PR review comment:
Here and elsewhere -- anywhere a node is a struct with fields -- a pattern I like to use is an infallible destructuring without an ellipsis, to ensure I've got all the fields and do something with them; e.g.:
let Type { is_extern, is_nodebug, ty } = self; // ... print `is_extern`, `is_nodebug`, `ty`This way, if/when we add new fields, we don't forget to add them to the printer, because we'll get a compile error otherwise.
Wasmtime GitHub notifications bot (Jun 06 2025 at 23:45):github-actions[bot] commented on PR #10962:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:42):mmcloughlin updated PR #10962.
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:42):mmcloughlin submitted PR review.
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:42):mmcloughlin created PR review comment:
Yes, done.
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:43):mmcloughlin submitted PR review.
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:43):mmcloughlin created PR review comment:
This is a really nice technique, thanks for sharing. I've used it throughout the file.
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:52):mmcloughlin submitted PR review.
Wasmtime GitHub notifications bot (Jun 08 2025 at 22:52):mmcloughlin created PR review comment:
Makes sense, removed. I also consolidated the print tests back into the existing
run_tests.rs. The separateprinter_tests.rswas a holdover from #8263, which had the printer guarded by a feature flag because of the pretty-printer dependency.I did find the tests over the complete ISLE files to be useful/reassuring during development, and a useful backup to ensure the printer stays in sync with the ISLE syntax. So I wonder if there's a good way to do this that maintains the independence of the ISLE crate. Ideas:
- Dump a snapshot of ISLE files into
isle_examples/print. For example, from the cranelift directory running:find . -name '*.isle' | xargs cat >isle/isle/isle_examples/print/all.isle.- Some special environment variable signals that additional test cases should be generated (e.g.
WASMTIME_CRANELIFT_DIR). This would be set in Wasmtime CI but not considered an error if it's missing.
Wasmtime GitHub notifications bot (Jun 08 2025 at 23:02):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 08 2025 at 23:02):cfallin created PR review comment:
We could potentially add a test to the
cranelift-codegenside to roundtrip-test the printer -- that's the natural point in the dep graph at which we have both those files and the ISLE compiler. I'm happy to review that as a followup if you'd like.
Wasmtime GitHub notifications bot (Jun 08 2025 at 23:03):cfallin submitted PR review:
Thanks!
Wasmtime GitHub notifications bot (Jun 08 2025 at 23:25):cfallin merged PR #10962.
Last updated: Dec 06 2025 at 07:03 UTC