Stream: git-wasmtime

Topic: wasmtime / PR #4571 Cranellift: remove Baldrdash support ...


view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 19:11):

cfallin opened PR #4571 from remove-baldrdash to main:

As noted in Mozilla's bugzilla bug 1781425 [1], the SpiderMonkey team
has recently determined that their current form of integration with
Cranelift is too hard to maintain, and they have chosen to remove it
from their codebase. If and when they decide to build updated support
for Cranelift, they will adopt different approaches to several details
of the integration.

In the meantime, after discussion with the SpiderMonkey folks, they
agree that it makes sense to remove the bits of Cranelift that exist
to support the integration ("Baldrdash"), as they will not need
them. Many of these bits are difficult-to-maintain special cases that
are not actually tested in Cranelift proper: for example, the
Baldrdash integration required Cranelift to emit function bodies
without prologues/epilogues, and instead communicate very precise
information about the expected frame size and layout, then stitched
together something post-facto. This was brittle and caused a lot of
incidental complexity ("fallthrough returns", the resulting special
logic in block-ordering); this is just one example. As another
example, one particular Baldrdash ABI variant processed stack args in
reverse order, so our ABI code had to support both traversal
orders. We had a number of other Baldrdash-specific settings as well
that did various special things.

This PR removes Baldrdash ABI support, the fallthrough_return
instruction, and pulls some threads to remove now-unused bits as a
result of those two, with the understanding that the SpiderMonkey folks
will build new functionality as needed in the future and we can perhaps
find cleaner abstractions to make it all work.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1781425

<!--

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

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

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 19:11):

cfallin requested fitzgen for a review on PR #4571.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 19:11):

cfallin requested bnjbvr for a review on PR #4571.

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

cfallin updated PR #4571 from remove-baldrdash to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:04):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:04):

bjorn3 created PR review comment:

I think this test should be kept, but with a different call conv.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:08):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:08):

bjorn3 created PR review comment:

Maybe keep this comment, but replace the description of spidermonkey with one of wasmtime's call conv?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:10):

bjorn3 created PR review comment:

this misses the wasmtime variants of system_v and fastcall.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:10):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:13):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:13):

bjorn3 created PR review comment:

I think this file should be deleted.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 20:44):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):

cfallin updated PR #4571 from remove-baldrdash to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):

cfallin created PR review comment:

Updated (and added the other missing ones too), thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):

cfallin created PR review comment:

Updated, thanks.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 01 2022 at 21:12):

cfallin updated PR #4571 from remove-baldrdash to main.

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

cfallin updated PR #4571 from remove-baldrdash to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

We can likely remove the use_allocated_encoding now as it always uses it, instead, and simplify some code in codegen?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

can this arg be removed now?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

can this arg be removed now?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

looks like this arg could be removed too?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

oh wow, glad to see this go away

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

ubernitty, but: these are still valid _initial_ planned uses, even if they don't materialize now; maybe we can keep this for archeologists, and add a mention that these are not planned uses at this point anymore?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):

bnjbvr created PR review comment:

Can this argument be removed now too?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin updated PR #4571 from remove-baldrdash to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin created PR review comment:

Indeed!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin created PR review comment:

Done, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin created PR review comment:

Indeed, removed.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):

cfallin created PR review comment:

Fixed the wording a bit with use of past tense: "initial planned uses were..." plus some current-status verbiage.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):

cfallin created PR review comment:

Indeed, done!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):

cfallin created PR review comment:

I opted to keep get_ext_mode because it's used in s390x currently and I didn't want to dig too deeply to see how it might or might not be inlinable. Good question though!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:07):

cfallin created PR review comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 17:07):

cfallin has enabled auto merge for PR #4571.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 18:54):

cfallin updated PR #4571 from remove-baldrdash to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 02 2022 at 19:37):

cfallin merged PR #4571.


Last updated: Dec 23 2024 at 12:05 UTC