Stream: git-wasmtime

Topic: wasmtime / PR #6358 winch: Implement new trampolines

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 13:17):

saulecabrera opened PR #6358 from saulecabrera:winch-array-wasm-trampoline to bytecodealliance:main:

This change is a follow-up to
https://github.com/bytecodealliance/wasmtime/pull/6262, in which the new trampolines, described here, were introduced to Wasmtime.

This change, focuses on the array-to-wasm,
native-to-wasm and wasm-to-native trampolines to restore Winch's working state prior to the introduction of the new trampolines. It's worth noting that the new approach for trampolines make it easier to support the TypedFunc API in Winch. Prior to the introduction of the new trampolines, it was not obvious how to approach it.

This change also introduces a pinned register that will hold the VMContext pointer, which is loaded in the *-to-wasm trampolines; the VMContext register is a pre-requisite to this change to support the wasm-to-native trampolines.

Lastly, with the introduction of the VMContext register and the wasm-to-native trampolines, this change also introduces support for calling function imports, which is a variation of the already existing calls to locally defined functions.

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

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
-->

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton submitted PR review:

Nice!

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton submitted PR review:

Nice!

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton created PR review comment:

Not 100% related to this PR per se, but I've found that using all the FooIndex types has been quite helpful throughout Cranelift and the existing wasm lowering to ensure that everything is kept straight. For example I saw this and immediately thought "wait that's bad because this is a defined index" and then looked around a bit before I noticed above that the variable was shadowed with the FuncIndex. This index.as_u32() is fine as-is, but it might be useful to use the wasmtime-types::* index types throughout winch in the future (only if you agree of course!)

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton created PR review comment:

Out of curiosity, is there any particular reason to have accessor methods like this as opposed to exposing the VMOffsets directly?

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton created PR review comment:

Instead of manufacturing a dummy 1u32 here could the index of what to call perhaps be a payload of TrampolineKind? That way WasmToNative would have no payload but the other two would have payloads.

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton created PR review comment:

minor nit, but here instead of ty vs ty.clone() vs &ty could these all take ty: &FuncType?

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton created PR review comment:

Like above, technically this I64 should conditionally be I32 for 32-bit hosts, right? Not that we're going to support those any time soon, but might be good to perhaps abstract ahead of time in case anyone in the future comes to add support if it's not too hard. (if it is a bit onerous I think a // FIXME: ... is fine too since we don't have 32-bit support anywhere on the horizon)

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 14:40):

alexcrichton created PR review comment:

Technically this S64 is actually "whatever the A's pointer size is", right?

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 19:03):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 09 2023 at 19:41):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2023 at 16:47):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2023 at 16:52):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2023 at 18:32):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2023 at 20:02):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 10 2023 at 21:34):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 14:39):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 16:46):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 18:28):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 18:29):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 18:35):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 18:37):

saulecabrera edited PR #6358:

This change is a follow-up to
https://github.com/bytecodealliance/wasmtime/pull/6262, in which the new trampolines, described here, were introduced to Wasmtime.

This change, focuses on the array-to-wasm,
native-to-wasm and wasm-to-native trampolines to restore Winch's working state prior to the introduction of the new trampolines. It's worth noting that the new approach for trampolines make it easier to support the TypedFunc API in Winch. Prior to the introduction of the new trampolines, it was not obvious how to approach it.

This change also introduces a pinned register that will hold the VMContext pointer, which is loaded in the *-to-wasm trampolines; the VMContext register is a pre-requisite to this change to support the wasm-to-native trampolines.

Lastly, with the introduction of the VMContext register and the wasm-to-native trampolines, this change also introduces support for calling function imports, which is a variation of the already existing calls to locally defined functions.

The other notable piece of this change aside from the trampolines is winch-codegen's dependency on wasmtime-environ. Winch is so closely tied to the concepts exposed by the wasmtime crates that it makes sense to tie them together, even though the separation provides some advantages like easier testing in some cases, in the long run, there's probably going to be less need to test Winch in isolation and rather we'd rely more on integration style tests which require all of Wasmtime
pieces anyway (fuzzing, spec tests, etc).

This change doesn't update the existing implmenetation of winch_codegen::FuncEnv, but the intention is to update that part after this change.

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

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
-->

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 19:35):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 19:37):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:06):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:24):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:31):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:34):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:37):

saulecabrera has marked PR #6358 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:37):

saulecabrera requested cfallin for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:37):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:37):

saulecabrera requested itsrainy for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:37):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:37):

saulecabrera requested wasmtime-default-reviewers for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 20:50):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 21:04):

saulecabrera requested alexcrichton for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 21:40):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 15 2023 at 21:45):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 13:07):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 13:11):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 13:58):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 14:32):

alexcrichton submitted PR review:

Looks good to me! @cfallin may want to also take a look at some of the assembly bits too though?

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 16:08):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 16:11):

saulecabrera requested cfallin for a review on PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 16:16):

saulecabrera updated PR #6358.

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 17:31):

cfallin submitted PR review:

Overall this looks very reasonable -- no objections (and thanks Alex for doing the detailed review here)!

view this post on Zulip Wasmtime GitHub notifications bot (May 16 2023 at 18:06):

saulecabrera merged PR #6358.

Last updated: Nov 22 2024 at 16:03 UTC