alexcrichton opened PR #3196 from move-module-trans
to main
:
This commit deletes the
ModuleEnvironment
trait andtranslate_module
functions in thecranelift-wasm
crate, inlining the implementation
into thewasmtime-environ
crate. This also removes thewasm
mode of
theclif-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 oncranelift-wasm
in that
situation for module translation which means that something needs to
happen. One option is to refactor what's incranelift-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 fromcranelift-wasm
. Afterwards the
only functionality remaining incranelift-wasm
is function translation
(which is quite important, and won't be moving!). This means that users
ofcranelift-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 ofcranelift-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.
-->
alexcrichton requested pchickey for a review on PR #3196.
bjorn3 created PR review comment:
This will make it much harder to use souper_harvest for wasm modules.
bjorn3 submitted PR review.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Does wasmtime have similar functionality builtin?
fitzgen submitted PR review.
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.
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 incranelift-wasm
which are barely maintained at best and shouldn't be a great source of "we're getting real data from these"
alexcrichton submitted PR review.
alexcrichton updated PR #3196 from move-module-trans
to main
.
alexcrichton updated PR #3196 from move-module-trans
to main
.
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 oncranelift-wasm
in that
situation for module translation which means that something needs to
happen. One option is to refactor what's incranelift-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 bywasmtime-environ
directly intowasmtime-environ
. This means
that the only real meat that Wasmtime uses fromcranelift-wasm
is the
function-translation bits in thewasmtime-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.
fitzgen submitted PR review.
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.
pchickey submitted PR review.
alexcrichton merged PR #3196.
Last updated: Dec 23 2024 at 12:05 UTC