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

Returns values up to 2*usize by value #77434

Merged
merged 1 commit into from
Oct 4, 2020
Merged

Returns values up to 2*usize by value #77434

merged 1 commit into from
Oct 4, 2020

Conversation

jonas-schievink
Copy link
Contributor

Addresses #76986 (comment) and #76986 (comment) by doing the optimization on all targets.

This matches what we do for functions returning &[T] and other fat pointers, so it should be Harmless™

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2020
@jonas-schievink
Copy link
Contributor Author

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned estebank Oct 1, 2020
@nagisa
Copy link
Member

nagisa commented Oct 1, 2020

@bors try @rust-timer queue

cc @eddyb

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Oct 1, 2020

⌛ Trying commit b01694e with merge af826c3de514b426f1172de2883db4a251e45c65...

@mati865
Copy link
Contributor

mati865 commented Oct 1, 2020

Perf runs only on x86_64 so there should be no noticeable change.

@bors
Copy link
Contributor

bors commented Oct 1, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: af826c3de514b426f1172de2883db4a251e45c65 (af826c3de514b426f1172de2883db4a251e45c65)

@rust-timer
Copy link
Collaborator

Queued af826c3de514b426f1172de2883db4a251e45c65 with parent 8fe73e8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (af826c3de514b426f1172de2883db4a251e45c65): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

Comment on lines -618 to -620
match spec.target_spec().arch.as_str() {
// System-V will pass return values up to 128 bits in RAX/RDX.
"x86_64" => Size::from_bits(128),
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping removing this string comparison would help perf but I guess not?

if is_ret { call::max_ret_by_val(cx) } else { Pointer.size(cx) };
// Return structures up to 2 pointers in size by value, matching `ScalarPair`. LLVM
// will usually return these in 2 registers, which is more efficient than by-ref.
let max_by_val_size = if is_ret { Pointer.size(cx) * 2 } else { Pointer.size(cx) };
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it makes sense to limit this to returns, but also whether 3 might work better than 2 as the multiplier.
But I guess there's a risk it might get downgraded from registers... I really wish LLVM modeled ABIs more precisely so we can control that.

@nagisa
Copy link
Member

nagisa commented Oct 3, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2020

📌 Commit b01694e has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2020
@bors
Copy link
Contributor

bors commented Oct 3, 2020

⌛ Testing commit b01694e with merge bad9ad0...

@bors
Copy link
Contributor

bors commented Oct 4, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nagisa
Pushing bad9ad0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 4, 2020
@bors bors merged commit bad9ad0 into rust-lang:master Oct 4, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 4, 2020
@jonas-schievink jonas-schievink deleted the ret-in-reg-2-electric-boogalo branch November 30, 2020 01:18
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2020
Pass arguments up to 2*usize by value

In rust-lang#77434 (comment), `@eddyb` said:

> I wonder if it makes sense to limit this to returns [...]

Let's do a perf run and find out.

It seems the `extern "C"` ABI will pass arguments up to 2*usize in registers: https://godbolt.org/z/n8E6zc. (modified from rust-lang#26494 (comment))

r? `@nagisa`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants