wasmtime / PR #9251 Pulley: tail calls · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #9251 Pulley: tail calls

Wasmtime GitHub notifications bot (Sep 15 2024 at 22:00):

Kmeakin opened PR #9251 from Kmeakin:km/pulley/tail-calls to bytecodealliance:main:

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

Wasmtime GitHub notifications bot (Sep 15 2024 at 22:00):

Kmeakin requested fitzgen for a review on PR #9251.

Wasmtime GitHub notifications bot (Sep 15 2024 at 22:00):

Kmeakin requested wasmtime-default-reviewers for a review on PR #9251.

Wasmtime GitHub notifications bot (Sep 15 2024 at 22:15):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 16 2024 at 01:04):

github-actions[bot] commented on PR #9251:

Subscribe to Label Action

cc @fitzgen

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Sep 16 2024 at 15:40):

alexcrichton commented on PR #9251:

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.

Wasmtime GitHub notifications bot (Sep 16 2024 at 18:46):

fitzgen commented on PR #9251:

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?

Wasmtime GitHub notifications bot (Sep 16 2024 at 19:00):

fitzgen commented on PR #9251:

FWIW, it looks like https://github.com/rust-lang/rust/issues/127520 was filed about this regression, but closed as WONTFIX, so I also cross-posted the bug to the explicit_tail_calls tracking issue: https://github.com/rust-lang/rust/issues/112788#issuecomment-2353680606

Wasmtime GitHub notifications bot (Sep 16 2024 at 19:57):

WaffleLapkin commented on PR #9251:

@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 https://github.com/rust-lang/rust/issues/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.

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen submitted PR review:

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 :+1:

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

Thanks!

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

Ditto for this second commit: it could also be split out and land sooner.

Also, can this debug_assert!(!return_addr.is_null())?

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

Nice! This first commit could be split out into its own PR and probably land sooner than the following bits, if you want.

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

Maybe some docs for the module, even if we are skipping them for the individual functions?

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

Can this take a generic T: BytecodeStream and return a Result<(...), T::Error>? No need to foist the unsafety upon everyone. We can make unwrap_uninhabited a method on an exported extension trait, if we want to make it generally available to users of the decoder.
pub trait UnwrapUninhabited<T> {
    fn unwrap_uninhabited(self) -> T;
}

impl<T> UnwrapUninhabited<T> for Result<T, Uninhabited> {
    ...
}

...

pub fn $snake_name<T>(pc: &mut T) -> Result<($($($field_ty,)*)?), T::Error> {
    ...
}

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

As Alex mentioned, to better integrate with our CI infrastructure (which passes --all-features to tests), let's replace all feature = "tail_calls" in cfg(...)s with cfg(pulley_tail_calls) and cfg(not(pulley_tail_calls)).

Then we can turn it on and off via the RUSTFLAGS env var, for example
RUSTFLAGS="--cfg pulley_tail_calls" cargo test --all-features
And then by default CI will test the regular-calls version, but we can also add a step to the "Monolith Checks" CI job that sets the above RUSTFLAGS to check that the tail-calls version compiles.

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

Nice! I think this worked out really well, especially how each opcode handler doesn't need to explicitly decode immediates/operands, and everything Just Works

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

And then we can actually use become here. I've tested locally and cargo check works with become, and avoids the ICE.

Wasmtime GitHub notifications bot (Sep 17 2024 at 18:50):

fitzgen created PR review comment:

Although maybe we want another cfg(pulley_become) or something so that if people want to rely on LLVM to optimize calls in tail position into tail calls, then they can?

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:20):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:20):

Kmeakin created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:26):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:26):

Kmeakin created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:37):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:37):

Kmeakin created PR review comment:

cargo check works because it doesn't actually generate LLVM/assembly, but if you try cargo build or cargo test it will

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:41):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:47):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:47):

fitzgen created PR review comment:

Yep, that's the point: to be using become and still have cargo check type check the Rust code that we cfg on.

Not sure if it is actually implemented yet, but that should check that we aren't relying on any Drop implementations running after the tail call (which would preclude it from being an actual tail call instead of a call in tail position).

Wasmtime GitHub notifications bot (Sep 17 2024 at 22:48):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Sep 17 2024 at 23:53):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 17 2024 at 23:55):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 17 2024 at 23:59):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Sep 17 2024 at 23:59):

Kmeakin created PR review comment:

So run cargo check/cargo clippy with RUSTFLAGS="--cfg pulley_tail_calls", but not cargo test?

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:02):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:09):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:11):

Kmeakin submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:11):

Kmeakin created PR review comment:

Also, can this debug_assert!(!return_addr.is_null())?

NonNull::new_unchecked already performs the check in debug builds: https://doc.rust-lang.org/1.81.0/src/core/ptr/non_null.rs.html#217

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:16):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:42):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Sep 18 2024 at 00:42):

fitzgen created PR review comment:

Yeah just add a line something like
    # Make sure that the tail-calls version of Pulley checks okay.
    - run: cargo check -p pulley-interpreter
      env:
        RUSTFLAGS: "--cfg pulley_tail_calls"
around here: https://github.com/bytecodealliance/wasmtime/blob/e38ffa197cf951ed513896fc63bb8e5c7b23135a/.github/workflows/main.yml#L499

Wasmtime GitHub notifications bot (Sep 18 2024 at 01:19):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 18 2024 at 01:37):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 18 2024 at 01:41):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Sep 19 2024 at 18:09):

fitzgen commented on PR #9251:

Once https://github.com/bytecodealliance/wasmtime/pull/9274 lands, is this ready to merge as well?

Wasmtime GitHub notifications bot (Oct 02 2024 at 17:22):

fitzgen commented on PR #9251:

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?

Wasmtime GitHub notifications bot (Oct 04 2024 at 22:32):

Kmeakin commented on PR #9251:

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.

Wasmtime GitHub notifications bot (Oct 05 2024 at 00:47):

Kmeakin updated PR #9251.

Wasmtime GitHub notifications bot (Oct 07 2024 at 18:37):

fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Oct 07 2024 at 18:53):

fitzgen updated PR #9251.

Wasmtime GitHub notifications bot (Oct 07 2024 at 19:14):

fitzgen commented on PR #9251:

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.

Wasmtime GitHub notifications bot (Oct 07 2024 at 19:27):

fitzgen updated PR #9251.

Wasmtime GitHub notifications bot (Oct 07 2024 at 19:28):

fitzgen commented on PR #9251:

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.