Wasmtime GitHub notifications bot (Aug 01 2022 at 19:11): 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.
[ ] 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 01 2022 at 19:11):cfallin requested fitzgen for a review on PR #4571.
Wasmtime GitHub notifications bot (Aug 01 2022 at 19:11):cfallin requested bnjbvr for a review on PR #4571.
Wasmtime GitHub notifications bot (Aug 01 2022 at 19:18):cfallin updated PR #4571 from remove-baldrdash to main.
Wasmtime GitHub notifications bot (Aug 01 2022 at 20:04):bjorn3 submitted PR review.
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.
Wasmtime GitHub notifications bot (Aug 01 2022 at 20:08):bjorn3 submitted PR review.
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?
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.
Wasmtime GitHub notifications bot (Aug 01 2022 at 20:10):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Aug 01 2022 at 20:13):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Aug 01 2022 at 20:13):bjorn3 created PR review comment:
I think this file should be deleted.
Wasmtime GitHub notifications bot (Aug 01 2022 at 20:44):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):cfallin updated PR #4571 from remove-baldrdash to main.
Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):cfallin created PR review comment:
Updated (and added the other missing ones too), thanks!
Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 01 2022 at 21:11):cfallin created PR review comment:
Updated, thanks.
Wasmtime GitHub notifications bot (Aug 01 2022 at 21:12):cfallin updated PR #4571 from remove-baldrdash to main.
Wasmtime GitHub notifications bot (Aug 02 2022 at 04:42):cfallin updated PR #4571 from remove-baldrdash to main.
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
We can likely remove the
use_allocated_encodingnow as it always uses it, instead, and simplify some code in codegen?
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
can this arg be removed now?
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
- Can the unused arguments be removed now?
- (I haven't checked, so maybe not the case) Can the
get_ext_modebe removed entirely and inlined in its call sites (or implemented at a different level), if it returns the same thing on all the archs?
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
can this arg be removed now?
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
looks like this arg could be removed too?
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
oh wow, glad to see this go away
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?
Wasmtime GitHub notifications bot (Aug 02 2022 at 10:09):bnjbvr created PR review comment:
Can this argument be removed now too?
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin updated PR #4571 from remove-baldrdash to main.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin created PR review comment:
Indeed!
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin created PR review comment:
Done, thanks!
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin created PR review comment:
Indeed, removed.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:05):cfallin submitted PR review.
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.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):cfallin created PR review comment:
Indeed, done!
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):cfallin created PR review comment:
I opted to keep
get_ext_modebecause 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!
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:06):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:07):cfallin created PR review comment:
Done!
Wasmtime GitHub notifications bot (Aug 02 2022 at 17:07):cfallin has enabled auto merge for PR #4571.
Wasmtime GitHub notifications bot (Aug 02 2022 at 18:54):cfallin updated PR #4571 from remove-baldrdash to main.
Wasmtime GitHub notifications bot (Aug 02 2022 at 19:37):cfallin merged PR #4571.
Last updated: Nov 22 2024 at 16:03 UTC