cfallin opened PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
cfallin requested bnjbvr for a review on PR #1774.
cfallin updated PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
cfallin updated PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
nit: requested
bnjbvr created PR Review Comment:
uber-nit: I might be on the side of being overly explicit, but what do you think about precising (maybe just once) that
a return
is a return value, not a return instruction?
bnjbvr created PR Review Comment:
(a missed opportunity to use the XOR binary operator! more seriously, i like that what we have here is very explicit, so either way works fine)
bnjbvr created PR Review Comment:
This is breaking the encapsulation with
compute_arg_locs
, since the index of the stack return area pointer is chosen there; could this function return it, instead?
bnjbvr created PR Review Comment:
To avoid the Vec of at most one instruction (and the associated memory allocation), could this return either an
Option<Inst>
or aSmallVec<[Inst, N]>
?
bnjbvr created PR Review Comment:
An idea (that would avoid a few bounds checks): instead of doing this, could we take
params.iter()
, and callrev()
on it in theRets & Baldrdash
case, before iterating?
bnjbvr created PR Review Comment:
Do we want to check for overflow here?
bnjbvr created PR Review Comment:
and here?
bnjbvr created PR Review Comment:
Since this is basically user-controlled, I think this could happen in practice, right? If so, could we bubble up the error with an ImplLimit error, or open an issue to not forget about it?
bnjbvr created PR Review Comment:
This might have been made with future-proofing in mind, but in this case, since we only need 0 or 1 temporary all the time, it seems that this could just return a boolean right now, and
init_with_tmps
could take a singletmp
argument. Alternatively, could we pass a closure function toinit_with_tmps
that returns a tmp, so that it can claim as many as it requires? (and thenneeded_tmps
wouldn't be needed at all)(Implementing any of these ideas would avoid the short-lived SmallVec allocation below.)
cfallin created PR Review Comment:
I thought about that too :-) But it's a little obscure that it works for bools (at least my C-trained brain was unsure) so I think it might be better to be explicit...
cfallin submitted PR Review.
cfallin submitted PR Review.
cfallin created PR Review Comment:
I wanted to do that! Unfortunately if we want the iterator type to be statically known, this doesn't work, I think (the iterator would be either
std::slice::Iter
orstd::iter::Rev
), hence this explicit indexing.
cfallin submitted PR Review.
cfallin created PR Review Comment:
The u32 + u32 you mean? I think that wasm implementation limits will prevent the arg space + ret space from reaching 2GB (or getting close), but maybe I'm misunderstanding which overflow case you mean?
cfallin submitted PR Review.
cfallin created PR Review Comment:
So, this sent me on a bit of an adventure to add a new
RegOffset
pseudo-amode that does the Right Thing during emission (e.g. synthesize a large offset with a constant and an add). This had some carryover effects elsewhere so I'll PTAL the patch back to you before merging :-)
cfallin updated PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
cfallin updated PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
WASM may prevent it, but other users may not impose such constraints themselves.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Yes, what @bjorn3 said (thanks!).
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Could this return
MemArg::RegOffset
(not in an option anymore) unconditionally now?
bnjbvr submitted PR Review.
cfallin updated PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
cfallin submitted PR Review.
cfallin created PR Review Comment:
Done -- added a reasonable limit (128MB) to the
ABISig
construction so this is guaranteed not to overflow. Thanks!
cfallin created PR Review Comment:
Ah, I had somehow stopped halfway through updating that, but it turns out it's dead code, so I just removed it (all the uses build a
MemArg
directly).
cfallin submitted PR Review.
cfallin updated PR #1774 from aarch64-multivalue
to master
:
This PR adds support for multi-value returns to the new aarch64 backend's ABI implementation, using an ABI that should match that of SpiderMonkey's Wasm baseline compiler on aarch64.
The existing multivalue Wasm spec-tests pass; curiously, these did not seem to actually be disabled before, so the rudimentary "return values in x0..x7" ABI that was previously implemented actually was sufficient. Nevertheless, it seems most reasonable to standardize on the SpiderMonkey-style struct-return-area ABI so that we don't need to maintain two different schemes.
I have not yet been able to test this in SpiderMonkey; we first need to resolve the newly-failing tests on an Cranelift version bump on the SM side, and then I need to work out how to cross-compile SM, since I'm back to working on an x86 host for now. Putting this up for review now in any case, and can tweak if need be as we work out the above.
<!--
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.
-->
cfallin merged PR #1774.
Last updated: Dec 23 2024 at 13:07 UTC