Stream: git-wasmtime

Topic: wasmtime / PR #9665 pulley: Implement interpreter-to-host...

Wasmtime GitHub notifications bot (Nov 22 2024 at 22:39):

alexcrichton opened PR #9665 from alexcrichton:pulley-host-calls to bytecodealliance:main:

This commit is an initial stab at implementing interpreter-to-host communication in Pulley. The basic problem is that Pulley needs the ability to call back into Wasmtime to implement tasks such as memory.grow, imported functions, etc. For native platforms this is a simple call_indirect operation in Cranelift but the story for Pulley must be different because it's effectively switching from interpreted code to native code.

The initial idea for this in #9651 is replaced here and looks mostly similar but with a few changes. The overall structure of how this works is:

• A new call_indirect_host opcode is added to Pulley.
  • Function signatures that can be called from Pulley bytecode are statically enumerated at build-time.
  • This enables the implementation of call_indirect_host to take an immediate of which signature is being used and cast the function pointer to the right type.
• A new pulley-specific relocation is added to Cranelift for this opcode.
  • RelocDistance::Far calls to a name trigger the use of call_indirect_host.
  • The relocation is filled in by Wasmtime after compilation where the signature number is inserted.
  • A new NS_* value for user-function namespaces is reserved in wasmtime-cranelift for this new namespace of functions.
• Code generation for Pulley in wasmtime-cranelift now has Pulley-specific handling of the wasm-to-host transition where all previous call_indirect instructions are replaced with a call to a "backend intrinsic" which gets lowered to a call_indirect_host.

Note that most of this still isn't hooked up everywhere in Wasmtime. That means that the testing here is pretty light at this time. It'll require a fair bit more work to get everything fully integrated from Wasmtime in Pulley. This is expected to be one of the significant remaining chunks of work and should help unblock future testing (or make those diffs smaller ideally).

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Nov 22 2024 at 22:39):

alexcrichton requested abrown for a review on PR #9665.

Wasmtime GitHub notifications bot (Nov 22 2024 at 22:39):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #9665.

Wasmtime GitHub notifications bot (Nov 22 2024 at 22:39):

alexcrichton requested pchickey for a review on PR #9665.

Wasmtime GitHub notifications bot (Nov 22 2024 at 22:39):

alexcrichton requested wasmtime-core-reviewers for a review on PR #9665.

Wasmtime GitHub notifications bot (Nov 22 2024 at 22:39):

alexcrichton requested wasmtime-default-reviewers for a review on PR #9665.

Wasmtime GitHub notifications bot (Nov 22 2024 at 23:03):

alexcrichton updated PR #9665.

Wasmtime GitHub notifications bot (Nov 23 2024 at 01:05):

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

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "pulley", "wasmtime:api"

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 (Nov 25 2024 at 21:57):

abrown submitted PR review:

Makes sense to get something working.

Wasmtime GitHub notifications bot (Nov 25 2024 at 21:57):

abrown created PR review comment:

Missing docs...

Wasmtime GitHub notifications bot (Nov 25 2024 at 21:57):

abrown created PR review comment:

            // If pulley is enabled, even if we're not targeting it, determine

Wasmtime GitHub notifications bot (Nov 25 2024 at 21:57):

abrown created PR review comment:

                // fourth byte. The value `n` here should always fit within a

Or describe which 0-based index.

Wasmtime GitHub notifications bot (Nov 25 2024 at 22:57):

alexcrichton updated PR #9665.

Wasmtime GitHub notifications bot (Nov 25 2024 at 22:57):

alexcrichton has enabled auto merge for PR #9665.

Wasmtime GitHub notifications bot (Nov 25 2024 at 23:32):

alexcrichton merged PR #9665.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:43):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:43):

fitzgen created PR review comment:

Does the signature actually need to be resolved at reloc time? It can't be done at compile time and embedded in the instruction itself?

The address of any host function obviously needs to be reloc time (this is a bit of an aside because my understanding is that we aren't actually embedding any host function addresses in the pulley bytecode) however the signature doesn't seem like it should need to be resolved at reloc time.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:43):

fitzgen created PR review comment:

Should we also check the calling conventions at all here? That was what I was (hackily) using to distinguish between pulley-to-pulley and pulley-to-host before. I like reloc-distance better but maybe we should be asserting that pulley-to-pulley always uses tail and pulley-to-host always uses systemv (which is a bit of a lie) or something like that?

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:48):

alexcrichton created PR review comment:

I think what you're thinking is already done actually, but the phrasing here is ambiguous. The "reloc time" technically happens twice -- once when linking things into artifacts and again when loading the artifacts. Putting the signature into the instruction happens in the first of these, during linking time. The relocation here is needed because the UserExternalName isn't available during compilation, only after the compile has finished, so that level of relocation processing is required to stuff it in.

Otherwise though there's no runtime relocation when we load the bytecode itself, it's all frozen and loaded as-is from disk or the compile artifact.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:49):

alexcrichton created PR review comment:

I think that's reasonable yeah, I'll try to go back and add some assertions.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:56):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:56):

fitzgen created PR review comment:

Ahhhh it is the function's id/code that is being reloc'd at link time here? That makes sense to me. When I read "signature" I was thinking "parameter and result types" and perhaps "calling convention", which happens to align with cranelift_codegen::ir::Signature.

Can we replace "signature" with "code" or "id" in these bits?

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:57):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 19:57):

alexcrichton created PR review comment:

Good point yeah, this is also something that changed halfway through the design and I didn't get around to updating all the docs

Last updated: Dec 23 2024 at 12:05 UTC