Pulley: tail calls #9251

Kmeakin · 2024-09-15T22:00:31Z

Sorry for the delay, I was on holiday.
The proof of concept works, but codegen for become is not actually implemented yet and so triggers an ICE:

error: internal compiler error: /rustc/9b72238eb813e9d06e9e9d270168512fbffd7ee7/compiler/rustc_codegen_ssa/src/mir/block.rs:1372:17: `TailCall` terminator is not yet supported by `rustc_codegen_ssa`
   --> pulley/src/interp/interp_loop.rs:85:25
    |
85  |                           become (next_handler.fun)($state, $pc);
    |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So for now return is used instead of become and we must hope that LLVM will be smart enough to recongize the tail call

github-actions · 2024-09-16T01:04:41Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "pulley"

Thus the following users have been cc'd because of the following labels:

fitzgen: pulley

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton · 2024-09-16T15:40:18Z

Thanks for this! I might jump in with a small recommendation on overall structure / CI here -- with a Cargo feature controlling this it unfortunately doesn't play well with our "test with all features enabled" in CI well because it enables the feature when a stable compiler is in use. To fix this I might recommend #[cfg(pulley_tail_call)] perhaps? That would require setting RUSTFLAGS in CI but that could be added to the "Monolith Checks" perhaps as a single cargo check of pulley on nightly? That would also enable this to land despite codegen support not being in upstream rust-lang/rust yet.

fitzgen · 2024-09-16T18:46:18Z

Thanks! I'll take a look either later today or tomorrow.

Mildly surprised about become not working anymore, as it was working when I did a few experiments around the time of the RFC. I guess rustc is doing a new ssa thing in the midend and hasn't extended support to become yet?

fitzgen · 2024-09-16T19:00:38Z

FWIW, it looks like rust-lang/rust#127520 was filed about this regression, but closed as WONTFIX, so I also cross-posted the bug to the explicit_tail_calls tracking issue: rust-lang/rust#112788 (comment)

WaffleLapkin · 2024-09-16T19:57:39Z

@fitzgen I'm not sure how it could have worked for you -- become was never fully implemented. (maybe you tried to use it when it was just an alternative syntax for return?)

either way the reason why rust-lang/rust#127520 was WONTFIXED is that tails calls are simply not implemented yet, they are an incomplete feature. The error even says as much:

TailCall terminator is not yet supported by rustc_codegen_ssa

there is no reason to bring any more awareness to this issue -- it will be fixed naturally once the implementation is complete.

fitzgen

Looks great! Really happy with how this shaped up.

Feel free to split out subcommits if you want, but it isn't necessary. Not sure they will actually land first at this point, since the final commit doesn't have as much feedback. Probably good to do for future PRs tho, just to keep things nice and easy for reviewers 👍

Big thing needed to land this is integrating with our CI infrastructure and replacing the cargo feature, as Alex mentioned.

Thanks!

pulley/src/decode.rs

pulley/src/interp.rs

fitzgen · 2024-09-17T16:54:57Z

pulley/src/interp.rs

-    fn pc_rel_jump(&mut self, offset: PcRelOffset, inst_size: isize) -> Continuation {
+    fn pc_rel_jump(&mut self, offset: PcRelOffset, inst_size: isize) {
        let offset = isize::try_from(i32::from(offset)).unwrap();
        self.pc = unsafe { self.pc.offset(offset - inst_size) };
-        Continuation::Continue


For caller ergonomics, this could still return a ControlFlow::Continue(()). This way, we wouldn't need to update every call site to also return that value.

fitzgen · 2024-09-17T16:59:33Z

pulley/src/interp.rs

-enum Continuation {
-    Continue,
+/// The reason the interpreter loop terminated.
+#[derive(Copy, Clone, Debug, PartialEq, Eq)]
+enum Done {
+    /// A `ret` instruction was executed and the call stack was empty. This is
+    /// how the loop normally ends.
    ReturnToHost,
+
+    /// a `trap` instruction was executed.
    Trap,


FWIW, the size of ControlFlow<Done> is going to be bigger than the size of Continuation was, because there will be two enum discriminant tags instead of one. But that is fine, and we can measure performance down the line and see if this actually matters. I suspect it might matter if it crosses an ABI threshold where returning these values goes from being in a register to going through a return pointer to a caller-supplied, stack-allocated space. But, as I said, we can always measure later.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4536593481e0f790a4f2d31c969b7e19

ControlFlow<Done, ()> and Continuation are both 1 byte (it seems rustc is smart enough to use the "unused discriminants" from Done as niches for ControlFlow).

ControlFlow<Done, OpcodeHandler> is 2 words. The alternative would be to return ControlFlow<Done, ()> and set the next handler via an &mut OpcodeHandler parameter, but I expect that might be harder for LLVM to optimize. In the worst case, returning a large aggregate will be lowered to returning via an out-parameter anyway, so it shouldn't be any worse performance wise

We could also squeeze ControlFlow<Done, OpcodeHandler> into a single word by using the lower 2 bits as the discriminant tag, since function pointers must be word aligned. I think there was a proposal for rustc to do this automatically but it has not yet been implemented.

pulley/src/decode.rs

pulley/src/interp/interp_loop.rs

pulley/src/lib.rs

pulley/src/interp/interp_loop.rs

fitzgen · 2024-09-19T18:09:47Z

Once #9274 lands, is this ready to merge as well?

fitzgen · 2024-10-02T17:22:45Z

Once #9274 lands, is this ready to merge as well?

@Kmeakin, now that #9274 landed, is this ready to rebase and land as well, or are there still more things left to do first?

Kmeakin · 2024-10-04T22:32:04Z

Once #9274 lands, is this ready to merge as well?

@Kmeakin, now that #9274 landed, is this ready to rebase and land as well, or are there still more things left to do first?

I think it is ready to land, I don't have anything else I want to add to it.

fitzgen

Thanks!

After bytecodealliance#9251, we will check that Pulley builds with rustc's experimental tail calls feature enabled in the nightly CI job. So if we touch any pulley source files in a PR, we should also run that job in the PR's CI.

fitzgen · 2024-10-07T19:14:54Z

I pushed a commit moving the new CI check to the nightly tests CI job, since it requires a nightly rustc.

But it looks like there are some failures running pulley tests under miri now as well, taking a quick look.

Not the initial PC passed into `run`

fitzgen · 2024-10-07T19:28:32Z

Pushed a fix for the miri tests: Done::Trap needed to have the trapping PC as a payload so that we could pass that trapping PC to self.trap(pc) rather than the first PC passed to run.

fitzgen · 2024-10-07T20:07:21Z

Thanks again @Kmeakin!

* Run the nightly CI job when `pulley` files are touched in a PR After #9251, we will check that Pulley builds with rustc's experimental tail calls feature enabled in the nightly CI job. So if we touch any pulley source files in a PR, we should also run that job in the PR's CI. * Also run MIRI tests if `pulley` files are changed * review feedback * grep for miri in changed files, not commit messages

Rudxain · 2024-10-11T07:25:25Z

pulley/src/interp/interp_loop.rs

+/// when compiling without `#![feature(explicit_tail_calls)]` enabled (via
+/// `--cfg pulley_tail_calls`).
+///
+/// It seems rustc first parses the the function, encounters `become` and emits


"the the" mistake (typo?)

@Rudxain good eye! Care to make a PR fixing it?

Nevermind, I see that you already did -- thanks!

#9251 (comment)

Kmeakin requested a review from a team as a code owner September 15, 2024 22:00

Kmeakin requested review from fitzgen and removed request for a team September 15, 2024 22:00

Kmeakin force-pushed the km/pulley/tail-calls branch from 86c8be0 to 12d17b7 Compare September 15, 2024 22:15

github-actions bot added the pulley Issues related to the Pulley interpreter label Sep 16, 2024

fitzgen reviewed Sep 17, 2024

View reviewed changes

Kmeakin force-pushed the km/pulley/tail-calls branch 6 times, most recently from 2228667 to 5c8e2f8 Compare September 18, 2024 00:16

This was referenced Sep 18, 2024

Use split_first_chunk instead of splitting manually #9273

Merged

Use NonNull<u8> for pc instead of *mut u8 #9274

Merged

Kmeakin force-pushed the km/pulley/tail-calls branch 3 times, most recently from 51f2406 to 3d19204 Compare September 18, 2024 01:41

fitzgen mentioned this pull request Oct 2, 2024

Explicit Tail Calls rust-lang/rfcs#3407

Open

Kmeakin added 4 commits October 5, 2024 01:47

Document host return address and extract to const

09579d3

Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <[email protected]>

Replace Continutation with ControlFlow

d63c1dd

Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <[email protected]>

Tail recursive interpreter loop

7609227

Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <[email protected]>

Delete visitor based interpreter implementation

e64c1a4

Copyright (c) 2024, Arm Limited. Signed-off-by: Karl Meakin <[email protected]>

Kmeakin force-pushed the km/pulley/tail-calls branch from 3d19204 to e64c1a4 Compare October 5, 2024 00:47

fitzgen approved these changes Oct 7, 2024

View reviewed changes

Move pulley tail call CI step to a job that uses nightly rust

6677101

fitzgen added this pull request to the merge queue Oct 7, 2024

fitzgen mentioned this pull request Oct 7, 2024

Run the nightly CI job when pulley files are touched in a PR #9391

Merged

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 7, 2024

Record the PC at which a trap occurred

f9907d1

Not the initial PC passed into `run`

fitzgen enabled auto-merge October 7, 2024 19:28

fitzgen added this pull request to the merge queue Oct 7, 2024

Merged via the queue into bytecodealliance:main with commit 623a821 Oct 7, 2024
35 checks passed

Rudxain reviewed Oct 11, 2024

View reviewed changes

Rudxain mentioned this pull request Oct 11, 2024

fix doc mistake in interp_loop.rs #9456

Merged

github-merge-queue bot pushed a commit that referenced this pull request Oct 11, 2024

Update interp_loop.rs (#9456)

ef271bf

#9251 (comment)

Kmeakin deleted the km/pulley/tail-calls branch October 29, 2024 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pulley: tail calls #9251

Pulley: tail calls #9251

Kmeakin commented Sep 15, 2024

github-actions bot commented Sep 16, 2024

alexcrichton commented Sep 16, 2024

fitzgen commented Sep 16, 2024

fitzgen commented Sep 16, 2024

WaffleLapkin commented Sep 16, 2024

fitzgen left a comment

fitzgen Sep 17, 2024

fitzgen Sep 17, 2024

Kmeakin Sep 17, 2024

Kmeakin Sep 17, 2024

fitzgen commented Sep 19, 2024

fitzgen commented Oct 2, 2024

Kmeakin commented Oct 4, 2024

fitzgen left a comment

fitzgen commented Oct 7, 2024

fitzgen commented Oct 7, 2024

fitzgen commented Oct 7, 2024

Rudxain Oct 11, 2024

fitzgen Oct 11, 2024

fitzgen Oct 11, 2024

Pulley: tail calls #9251

Pulley: tail calls #9251

Conversation

Kmeakin commented Sep 15, 2024

github-actions bot commented Sep 16, 2024

Subscribe to Label Action

alexcrichton commented Sep 16, 2024

fitzgen commented Sep 16, 2024

fitzgen commented Sep 16, 2024

WaffleLapkin commented Sep 16, 2024

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen Sep 17, 2024

Choose a reason for hiding this comment

fitzgen Sep 17, 2024

Choose a reason for hiding this comment

Kmeakin Sep 17, 2024

Choose a reason for hiding this comment

Kmeakin Sep 17, 2024

Choose a reason for hiding this comment

fitzgen commented Sep 19, 2024

fitzgen commented Oct 2, 2024

Kmeakin commented Oct 4, 2024

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen commented Oct 7, 2024

fitzgen commented Oct 7, 2024

fitzgen commented Oct 7, 2024

Rudxain Oct 11, 2024

Choose a reason for hiding this comment

fitzgen Oct 11, 2024

Choose a reason for hiding this comment

fitzgen Oct 11, 2024

Choose a reason for hiding this comment