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

Perform MIR NRVO even if types don't match #72451

Merged
merged 1 commit into from
May 26, 2020

Conversation

ecstatic-morse
Copy link
Contributor

This is the most straightforward way to resolve #72428, but it could cause problems in codegen since the type of _0 may no longer match the return type of the body.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 May 22, 2020
@ecstatic-morse
Copy link
Contributor Author

Single pointers get returned via register, so I think there's almost no chance this was actually responsible for the perf changes described in #72428. Just in case:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 22, 2020

⌛ Trying commit d24ba6d with merge bc74f58ab5960fcfa233c8d5c77cd3407597b747...

@bors
Copy link
Contributor

bors commented May 22, 2020

☀️ Try build successful - checks-azure
Build commit: bc74f58ab5960fcfa233c8d5c77cd3407597b747 (bc74f58ab5960fcfa233c8d5c77cd3407597b747)

@rust-timer
Copy link
Collaborator

Queued bc74f58ab5960fcfa233c8d5c77cd3407597b747 with parent 458a3e7, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit bc74f58ab5960fcfa233c8d5c77cd3407597b747, comparison URL.

@ecstatic-morse
Copy link
Contributor Author

This has no effect. Perhaps we should just close this along with #72428?

@bjorn3
Copy link
Member

bjorn3 commented May 23, 2020

While it may look like the only difference is noise, this saves ~0.1% on average for all changes >0.0%. A few regress ~0.1%, but this is only for a few of the benchmarks, while the ~0.1% wins are for much more benchmarks. Conclusion: this change does improve compilation time, but the change is tiny.

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 25, 2020

📌 Commit d24ba6d has been approved by matthewjasper

@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 May 25, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 25, 2020
…r=matthewjasper

Perform MIR NRVO even if types don't match

This is the most straightforward way to resolve rust-lang#72428, but it could cause problems in codegen since the type of `_0` may no longer match the return type of the body.
This was referenced May 25, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request May 25, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#72061 (add regression tests for stalled_on const vars)
 - rust-lang#72424 (fix ICE when debug-printing MIR)
 - rust-lang#72450 (Fix ice-rust-lang#72442)
 - rust-lang#72451 (Perform MIR NRVO even if types don't match)
 - rust-lang#72538 (Removed all instances of const_field.)

Failed merges:

r? @ghost
@bors bors merged commit 036688f into rust-lang:master May 26, 2020
@ecstatic-morse ecstatic-morse deleted the nrvo-type-mismatch branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Do NRVO when type of returned local does not match return type
6 participants