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

Try and assign directly to return registers; backtrack to use struct-return param #1213

Conversation

@fitzgen
Copy link
Member

fitzgen commented Nov 7, 2019

This has been discussed in issue #..., or if not, please tell us why here.

bytecodealliance/wasmtime#467

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.

Rather than trying to count number of return registers that would be used by a given set of return values, optimistically assign the return values to registers. If we later find that we can't fit them all in registers, then backtrack and introduce the use of a struct-return pointer parameter.

This PR contains test cases, if meaningful.

Tests in wasmtime. If we compiled the wasm spec testsuite in cranelift, that would have caught the issue as well.

…return param

Rather than trying to count number of return registers that would be used by a
given set of return values, optimistically assign the return values to
registers. If we later find that we can't fit them all in registers, then
backtrack and introduce the use of a struct-return pointer parameter.
@fitzgen fitzgen requested a review from bnjbvr Nov 7, 2019
@fitzgen fitzgen mentioned this pull request Nov 7, 2019
3 of 3 tasks complete
@bnjbvr
bnjbvr approved these changes Nov 8, 2019
Copy link
Member

bnjbvr left a comment

Cool, that's simpler to understand too, thanks.

ArgAction::Assign(ArgumentLoc::Reg(reg)) => {
ret_ptr_param.location = ArgumentLoc::Reg(reg);
sig.to_mut().params.push(ret_ptr_param);
if let Some(new_returns) = legalize_args(&sig.returns, &mut rets) {

This comment has been minimized.

Copy link
@bnjbvr

bnjbvr Nov 8, 2019

Member

(light suggestion) instead of having rets2 (which is not very descriptive a name), could we pass &mut rets.clone() here, and use rets where we used rets2 below?

Otherwise, could we rename rets2 to rets_backup and explain in a small comment why it's needed in the first place

This comment has been minimized.

Copy link
@bnjbvr

bnjbvr Nov 8, 2019

Member

(or maybe original_rets, so it doesn't sound too weird with respect to the other comment.)

@@ -331,71 +224,72 @@ pub fn legalize_signature(
shared_flags,
isa_flags,
);
let mut rets2 = rets.clone();

This comment has been minimized.

Copy link
@bnjbvr

bnjbvr Nov 8, 2019

Member

It's unfortunate the clone happens even when one doesn't use multi-value returns. To make this slightly more zero-cost, could we clone only when sig.is_multi_return instead?

@fitzgen

This comment has been minimized.

Copy link
Member Author

fitzgen commented Nov 8, 2019

Thanks for review! Will fix up with your suggestions and then merge once CI is green again 👍

@fitzgen fitzgen merged commit 377aa73 into bytecodealliance:master Nov 8, 2019
12 checks passed
12 checks passed
Rustfmt
Details
Build API Docs
Details
Test (windows-earliest)
Details
Test (linux-earliest)
Details
Test (mac-earliest)
Details
Test (stable)
Details
Test (beta)
Details
Test (nightly) Test (nightly)
Details
Test (windows-release)
Details
Test (linux-release)
Details
Test (mac-release)
Details
Fuzz Regression
Details
@fitzgen fitzgen deleted the fitzgen:backtrack-to-struct-return-param-when-we-cant-fit-returns-in-registers branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.