Stream: git-wasmtime

Topic: wasmtime / PR #3196 Move module translation from cranelif...

Wasmtime GitHub notifications bot (Aug 17 2021 at 17:55):

alexcrichton opened PR #3196 from move-module-trans to main:

This commit deletes the ModuleEnvironment trait and translate_module
functions in the cranelift-wasm crate, inlining the implementation
into the wasmtime-environ crate. This also removes the wasm mode of
the clif-util tool.

The main purpose for doing this is that this is a large piece of
functionality used by Wasmtime which is entirely independent of
Cranelift. Eventually Wasmtime wants to be able to compile without
Cranelift, but it can't also depend on cranelift-wasm in that
situation for module translation which means that something needs to
happen. One option is to refactor what's in cranelift-wasm into a
separate crate (since all these pieces don't actually depend on
cranelift-codegen), but I personally chose to not do this because:

• The ModuleEnvironment trait, AFAIK, only has a primary user of
  Wasmtime. The Spidermonkey integration, for example, does not use this.

• This is an extra layer of abstraction between Wasmtime and the
  compilation phase which was a bit of a pain to maintain. It couldn't
  be Wasmtime-specific as it was part of Cranelift but at the same time
  it had lots of Wasmtime-centric functionality (such as module
  linking).

• Updating the "dummy" implementation has become pretty onerous over
  time as frequent additions are made and the "dummy" implementation was
  never actually used anywhere. This ended up feeling like effectively
  busy-work to update this.

For these reasons I've opted to delete the "dummy" implementation and
entirely remove module translation from cranelift-wasm. Afterwards the
only functionality remaining in cranelift-wasm is function translation
(which is quite important, and won't be moving!). This means that users
of cranelift-wasm will need to reimplement module translation
themselves, if there are any.

The changes in wasmtime-environ are largely to inline module parsing
together so it's a bit easier to follow instead of trying to connect
the dots between lots of various function calls.

Note, though, I did put in the work to keep the cranelift-wasm tests
passing. They effectively implement a "bare bones" module translator
which can serve to test the functionality of cranelift-wasm that
Wasmtime itself does not exercise.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Aug 17 2021 at 17:56):

alexcrichton requested pchickey for a review on PR #3196.

Wasmtime GitHub notifications bot (Aug 17 2021 at 18:00):

bjorn3 created PR review comment:

This will make it much harder to use souper_harvest for wasm modules.

Wasmtime GitHub notifications bot (Aug 17 2021 at 18:00):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 18:00):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 18:00):

bjorn3 created PR review comment:

Does wasmtime have similar functionality builtin?

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:06):

fitzgen created PR review comment:

Yeah, ideally we would still be able to harvest LHSes from Wasm modules here. Or we would move the Wasm harvester into the wasmtime CLI and leave this as a CLIF-only harvestor.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:24):

alexcrichton created PR review comment:

FWIW @fitzgen I'll leave this in since I'll back out the changes to wasmtime-cranelift, but I think if we want to harvest things from wasm that Wasmtime uses then this is the wrong level of abstraction becaue it's missing all of Wasmtime's configuration of Cranelift as well as Wasmtime-specific implementation details of runtime things like tables/references/etc.

I'll admit I don't know why this command exists, I've never actually seen anyone use it. It also seems to rely somewhat heavily on the Dummy* traits in cranelift-wasm which are barely maintained at best and shouldn't be a great source of "we're getting real data from these"

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:24):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:28):

alexcrichton updated PR #3196 from move-module-trans to main.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:28):

alexcrichton updated PR #3196 from move-module-trans to main.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:29):

alexcrichton edited PR #3196 from move-module-trans to main:

The main purpose for doing this is that this is a large piece of
functionality used by Wasmtime which is entirely independent of
Cranelift. Eventually Wasmtime wants to be able to compile without
Cranelift, but it can't also depend on cranelift-wasm in that
situation for module translation which means that something needs to
happen. One option is to refactor what's in cranelift-wasm into a
separate crate (since all these pieces don't actually depend on
cranelift-codegen), but I personally chose to not do this because:

• The ModuleEnvironment trait, AFAIK, only has a primary user of
  Wasmtime. The Spidermonkey integration, for example, does not use this.

• This is an extra layer of abstraction between Wasmtime and the
  compilation phase which was a bit of a pain to maintain. It couldn't
  be Wasmtime-specific as it was part of Cranelift but at the same time
  it had lots of Wasmtime-centric functionality (such as module
  linking).

• Updating the "dummy" implementation has become pretty onerous over
  time as frequent additions are made and the "dummy" implementation was
  never actually used anywhere. This ended up feeling like effectively
  busy-work to update this.

For these reasons I've opted to to move the meat of cranelift-wasm
used by wasmtime-environ directly into wasmtime-environ. This means
that the only real meat that Wasmtime uses from cranelift-wasm is the
function-translation bits in the wasmtime-cranelift crate.

The changes in wasmtime-environ are largely to inline module parsing
together so it's a bit easier to follow instead of trying to connect
the dots between lots of various function calls.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:40):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2021 at 21:40):

fitzgen created PR review comment:

Yeah, is has known issues around using a different environment than Wasmtime. That said, most of the interesting stuff from a superoptimizer's point of view are the expressions, which don't typically change much depending on the environment, not the I/O and state integration code that do change a bunch depending on the environment.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:13):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2021 at 17:15):

alexcrichton merged PR #3196.

Last updated: Oct 23 2024 at 20:03 UTC