wasmtime / PR #1774 Multi-value return support on aarch64... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1774 Multi-value return support on aarch64...

Wasmtime GitHub notifications bot (May 27 2020 at 21:47):

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.

Wasmtime GitHub notifications bot (May 27 2020 at 21:47):

cfallin requested bnjbvr for a review on PR #1774.

Wasmtime GitHub notifications bot (May 28 2020 at 00:55):

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.

Wasmtime GitHub notifications bot (Jun 02 2020 at 00:22):

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.

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

bnjbvr created PR Review Comment:

nit: requested

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

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?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

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)

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

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?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

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 a SmallVec<[Inst, N]>?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

bnjbvr created PR Review Comment:

An idea (that would avoid a few bounds checks): instead of doing this, could we take params.iter(), and call rev() on it in the Rets & Baldrdash case, before iterating?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

bnjbvr created PR Review Comment:

Do we want to check for overflow here?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

bnjbvr created PR Review Comment:

and here?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

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?

Wasmtime GitHub notifications bot (Jun 02 2020 at 14:26):

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 single tmp argument. Alternatively, could we pass a closure function to init_with_tmps that returns a tmp, so that it can claim as many as it requires? (and then needed_tmps wouldn't be needed at all)

(Implementing any of these ideas would avoid the short-lived SmallVec allocation below.)

Wasmtime GitHub notifications bot (Jun 02 2020 at 17:34):

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...

Wasmtime GitHub notifications bot (Jun 02 2020 at 17:34):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jun 02 2020 at 17:36):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jun 02 2020 at 17:36):

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 or std::iter::Rev), hence this explicit indexing.

Wasmtime GitHub notifications bot (Jun 02 2020 at 23:51):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jun 02 2020 at 23:51):

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?

Wasmtime GitHub notifications bot (Jun 02 2020 at 23:52):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jun 02 2020 at 23:52):

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 :-)

Wasmtime GitHub notifications bot (Jun 02 2020 at 23:58):

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.

Wasmtime GitHub notifications bot (Jun 03 2020 at 00:01):

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.

Wasmtime GitHub notifications bot (Jun 03 2020 at 09:21):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 09:21):

bjorn3 created PR Review Comment:

WASM may prevent it, but other users may not impose such constraints themselves.

Wasmtime GitHub notifications bot (Jun 03 2020 at 10:12):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 10:12):

bnjbvr created PR Review Comment:

Yes, what @bjorn3 said (thanks!).

Wasmtime GitHub notifications bot (Jun 03 2020 at 14:54):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 14:54):

bnjbvr created PR Review Comment:

Could this return MemArg::RegOffset (not in an option anymore) unconditionally now?

Wasmtime GitHub notifications bot (Jun 03 2020 at 14:54):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 16:24):

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.

Wasmtime GitHub notifications bot (Jun 03 2020 at 16:25):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 16:25):

cfallin created PR Review Comment:

Done -- added a reasonable limit (128MB) to the ABISig construction so this is guaranteed not to overflow. Thanks!

Wasmtime GitHub notifications bot (Jun 03 2020 at 16:26):

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).

Wasmtime GitHub notifications bot (Jun 03 2020 at 16:26):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jun 03 2020 at 20:36):

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.

Wasmtime GitHub notifications bot (Jun 03 2020 at 21:14):

cfallin merged PR #1774.

Last updated: Nov 22 2024 at 17:03 UTC