Stream: git-wasmtime

Topic: wasmtime / PR #9089 Cranelift: Add a new backend for emit...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 00:31):

fitzgen requested cfallin for a review on PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 00:31):

fitzgen requested wasmtime-default-reviewers for a review on PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 00:31):

fitzgen opened PR #9089 from fitzgen:pulley-cranelift-backend to bytecodealliance:main:

This commit adds two new backends for Cranelift that emits 32- and 64-bit Pulley
bytecode. The backends are both actually the same, with a common implementation
living in cranelift/codegen/src/isa/pulley_shared. Each backend configures an
ISA flag that determines the pointer size, and lowering inspects this flag's
value when lowering memory accesses.

To avoid multiple ISLE compilation units, and to avoid compiling duplicate
copies of Pulley's generated MInst, I couldn't use MInst as the MachInst
implementation directly. Instead, there is an InstAndKind type that is a
newtype over the generated MInst but which also carries a phantom type
parameter that implements the PulleyTargetKind trait. There are two
implementations of this trait, a 32- and 64-bit version. This is necessary
because there are various static trait methods for the mach backend which we
must implement, and which return the pointer width, but don't have access to any
self. Therefore, we are forced to monomorphize some amount of code. This type
parameter is fairly infectious, and all the "big" backend
types (PulleyBackend<P>, PulleyABICallSite<P>, etc...) are parameterized
over it. Nonetheless, not everything is parameterized over a PulleyTargetKind,
and we manage to avoid duplicate MInst definitions and lowering code.

Note that many methods are still stubbed out with todo!s. It is expected that
we will fill in those implementations as the work on Pulley progresses.

The first commit is https://github.com/bytecodealliance/wasmtime/pull/9085; this depends on that PR.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 00:31):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 00:37):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 00:42):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 02:04):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "isle"

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Aug 08 2024 at 16:31):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2024 at 16:04):

alexcrichton submitted PR review:

Very nice! Great to see this all shape up so well

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2024 at 16:04):

alexcrichton submitted PR review:

Very nice! Great to see this all shape up so well

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2024 at 16:04):

alexcrichton created PR review comment:

This is a pretty huge mouthful and it seems like it might be somewhat common, so perhaps a helper function in this module to perform the conversion? For example to_pulley(r) or something like that

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2024 at 16:04):

alexcrichton created PR review comment:

This looks like a lot of copy/paste with a nontrivial amount of logic, would it be possible to refactor these?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 12 2024 at 16:04):

alexcrichton created PR review comment:

I think I saw this as well in abi.rs as well as lower.isle perhaps, coul the Rust-side parts be deduplicated into a helper?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 18:10):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 18:10):

fitzgen created PR review comment:

The one in abi.rs is constructing MInst::Xconst* variants while this one is calling differentenc::xconst* functions, so there isn't a great way to dedupe this. We could make something that is abstract enough to handle both but that abstraction won't actually be any more concise, simpler, or easy to understand, unfortunately.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 18:17):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 18:19):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 18:19):

fitzgen has enabled auto merge for PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 19:56):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 19:56):

fitzgen has enabled auto merge for PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 21:03):

fitzgen updated PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 21:04):

fitzgen has enabled auto merge for PR #9089.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 14 2024 at 21:32):

fitzgen merged PR #9089.

Last updated: Nov 22 2024 at 16:03 UTC