Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-value return support on aarch64/Wasmtime. #1774

Merged
merged 2 commits into from Jun 3, 2020

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented May 27, 2020

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.

@cfallin cfallin added the cranelift:area:aarch64 Issues related to AArch64 backend. label May 27, 2020
@cfallin cfallin requested a review from bnjbvr May 27, 2020 21:47
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen labels May 27, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:x64"

Thus the following users have been cc'd because of the following labels:

  • bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@bjorn3
Copy link
Contributor

bjorn3 commented May 27, 2020

The SystemV abi on AArch64 uses x0 and x1 to return pairs of integers. Could you please do that in Cranelif too? Even if only in the non baldrdash variant of the abi?

@cfallin
Copy link
Member Author

cfallin commented May 27, 2020

The SystemV abi on AArch64 uses x0 and x1 to return pairs of integers. Could you please do that in Cranelif too? Even if only in the non baldrdash variant of the abi?

Happy to take a followup patch for this, but perhaps not in the initial patch, if only because I don't have a way to test it easily. As far as I can tell, the principle in the past (e.g. in #1178) was to keep the ABI as simple as possible initially. I'm not opposed to declaring that we achieve full SysV ABI compatibility eventually, but it's a higher bar for sure!

@bjorn3
Copy link
Contributor

bjorn3 commented May 28, 2020

Keeping the abi simple is fine, but I want to have all the primitives necessary to implement full SystemV abi support from the clif ir producer side.

@cfallin
Copy link
Member Author

cfallin commented May 28, 2020

@bjorn3: after more thought, I think it actually wouldn't be too bad to at least give a "all return values are in registers; error if > K returns" (K = 8 on aarch64?) mode. Perhaps we can define this to be the default if we're not in Baldrdash (SpiderMonkey) mode. This would give you the building blocks needed to legalize whatever other ABI (SysV or otherwise) down to CLIF. Does that sound reasonable?

@bjorn3
Copy link
Contributor

bjorn3 commented May 28, 2020

👍

@cfallin
Copy link
Member Author

cfallin commented Jun 2, 2020

Updated to return values in x0-x7 by default.

@bnjbvr, PTAL -- I think we'll need to land this first before we can version-bump vendored Cranelift in SpiderMonkey and fix the issues there. Thanks!

Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Nice that we can support both Baldrdash and be compatible with the native Rust ABI on the first try :)

cranelift/codegen/src/machinst/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Show resolved Hide resolved
cranelift/codegen/src/isa/aarch64/abi.rs Outdated Show resolved Hide resolved
cranelift/codegen/src/machinst/abi.rs Outdated Show resolved Hide resolved
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition, thanks.

cranelift/codegen/src/isa/aarch64/inst/args.rs Outdated Show resolved Hide resolved
@cfallin cfallin merged commit 5078e4e into bytecodealliance:master Jun 3, 2020
@cfallin cfallin deleted the aarch64-multivalue branch June 8, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants