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

Implement multiple return terminator optimization #74839

Merged
merged 4 commits into from
Oct 1, 2020

Conversation

alarsyo
Copy link
Contributor

@alarsyo alarsyo commented Jul 27, 2020

Closes #72022

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2020

r? @oli-obk

@Muirrum Muirrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020
@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 14, 2020
@Dylan-DPC-zz
Copy link

@alarsyo any updates on this?

@alarsyo alarsyo force-pushed the multiple_return_terminators branch 2 times, most recently from b155f46 to 06a8ac5 Compare August 15, 2020 17:37
@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 15, 2020

Found the time to work on this, should be ready for a first basic review :)

@alarsyo alarsyo marked this pull request as ready for review August 15, 2020 17:53
@alarsyo alarsyo force-pushed the multiple_return_terminators branch from 1fcc274 to 449f158 Compare August 18, 2020 13:05
@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 18, 2020

Rebased on master

@oli-obk
Copy link
Contributor

oli-obk commented Aug 18, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 18, 2020

📌 Commit 779f52272d56c5ead356a1d17cb9cd6ca3fc304f has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 18, 2020
@tmandry
Copy link
Member

tmandry commented Aug 19, 2020

There doesn't seem to be consensus on #72022 that this is a good idea, are we sure we want this?

Also, there may be perf impact to this change so it probably shouldn't be rolled up.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2020

@bors rollup=never r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 19, 2020
@oli-obk oli-obk added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 19, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2020

@alarsyo there are multiple failing codegen tests, can you adjust them?

@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 28, 2020

Just to make sure I haven't made any mistake: when generating the IR in this comment, I had to remove this line from the file for the test to run, presumably because I'm building the compiler in debug mode. That doesn't affect the optimization level of the generated IR, right?

So if I understand correctly, this test doesn't pass because something in libstd/libcore changed when building them with this new optimization, and somehow makes it so that the IR doesn't get optimized to a constant by LLVM/rustc (not sure which one should optimize this part)?

How would I go about putting this behind mir-opt-level 3 ? I'm seeing multiple checks for mir_opt_level > 0 in librustc_mir/transform/mod.rs, but nothing for level 3

@oli-obk
Copy link
Contributor

oli-obk commented Aug 28, 2020

Here's an example for mir-opt-level 3:

if tcx.sess.opts.debugging_opts.mir_opt_level < 3 {

Just to make sure I haven't made any mistake: when generating the IR in this comment, I had to remove this line from the file for the test to run, presumably because I'm building the compiler in debug mode. That doesn't affect the optimization level of the generated IR, right?

Oh, that's a curious situation. it's still release mode, but with debug assertions, can you try adding // compile-flags: -Cdebug-assertions=off to the test (and remove the // ignore-debug comment) and see if that works?

I won't be able to review again until the 7th, cc @rust-lang/wg-mir-opt can someone have a look at this?

@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 28, 2020

Oh, that's a curious situation. it's still release mode, but with debug assertions, can you try adding // compile-flags: -Cdebug-assertions=off to the test (and remove the // ignore-debug comment) and see if that works?

That works, the test runs even though I'm in debug mode, thanks! I guess I shouldn't include this change in this PR though, right?

@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 28, 2020

Whoops, forgot to bless the mir-opt tests with the opt behind opt-level 3.

@alarsyo alarsyo force-pushed the multiple_return_terminators branch from 79708fa to 56eeca2 Compare August 28, 2020 19:38
@alarsyo
Copy link
Contributor Author

alarsyo commented Aug 29, 2020

Hmm, I can't reproduce the two failing tests by running ./x.py test src/test/mir-opt; they pass just fine on my computer.

@alarsyo alarsyo force-pushed the multiple_return_terminators branch from 56eeca2 to 889dd98 Compare August 29, 2020 18:57
@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 7, 2020

in order for the 32 bit versions to get updated you need to run with --target i686-unknown-linux-gnu

@crlf0710
Copy link
Member

@alarsyo Ping from triage, any updates on this?

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2020
@alarsyo
Copy link
Contributor Author

alarsyo commented Sep 25, 2020

@crlf0710 I've been pretty busy the last few weeks, nothing new for now :( I have to get back on this, hopefully by the next weekend

@alarsyo alarsyo force-pushed the multiple_return_terminators branch from 889dd98 to 268f786 Compare October 1, 2020 08:09
@alarsyo
Copy link
Contributor Author

alarsyo commented Oct 1, 2020

@oli-obk rebased on master, I'll try to make progress :)

@alarsyo
Copy link
Contributor Author

alarsyo commented Oct 1, 2020

Everything passes 🎉

@oli-obk
Copy link
Contributor

oli-obk commented Oct 1, 2020

@bors r+

awesome

@bors
Copy link
Contributor

bors commented Oct 1, 2020

📌 Commit 46c0bd3 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 1, 2020
@bors
Copy link
Contributor

bors commented Oct 1, 2020

⌛ Testing commit 46c0bd3 with merge 9cba260...

@bors
Copy link
Contributor

bors commented Oct 1, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 9cba260 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2020
@bors bors merged commit 9cba260 into rust-lang:master Oct 1, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 1, 2020
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mir-opt] optimization idea: bubble return terminators to predecessor blocks
10 participants