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.

Sign up for GitHub

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

Jump to bottom

Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default #82736

Merged
merged 12 commits into from
Mar 6, 2021
Merged

Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default #82736

merged 12 commits into from
Mar 6, 2021

Conversation

spastorino
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 3, 2021
@spastorino spastorino force-pushed the mir-opt-level-perf-changes branch from 3699ed9 to 10d0109 Compare March 3, 2021 22:52
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member

I'm a bit worried about changing meaning of these while retaining numbers - maybe we should rename the various levels (e.g., NoOpt, SoundOpts, UnsoundOpts, ...?)

@spastorino
Copy link
Member Author

I'm a bit worried about changing meaning of these while retaining numbers - maybe we should rename the various levels (e.g., NoOpt, SoundOpts, UnsoundOpts, ...?)

Yes, that's a good idea. There's work around that, that we would need to revive, for instance @oli-obk told me about #77665

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Mar 4, 2021

cc @rust-lang/wg-mir-opt

This is a stopgap solution to quickly be able to test out changing the meaning of mir-opt-level=2 to mean "automatically on in release mode". As already mentioned, the proper fix is to rebase and fix up #77665

@tmiasko
Copy link
Contributor

tmiasko commented Mar 4, 2021

Bumping the mir-opt-level for levels >= 2 could only affect users of mir-opt-level >= 2, so it seems fine to me. Though, if you do so, please update tests as well.

How would a pass manager use mir-opt-level & opt-level to enable passes, and what is stopping us from using equivalent approach now?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 4, 2021

Yea, i do realize now my suggestion is completely orthogonal to changing these levels. It's just more painful/messy to do without the pass manager.

Replacing the numeric levels with an enum sounds like a really good idea.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@spastorino spastorino force-pushed the mir-opt-level-perf-changes branch from 0d3c28e to 0550a6f Compare March 4, 2021 13:35
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor

oli-obk commented Mar 4, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2021

📌 Commit cbb40e1 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2021
@spastorino
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Mar 5, 2021

📌 Commit 18fb025 has been approved by oli-obk

@bjorn3 bjorn3 mentioned this pull request Mar 5, 2021
Closed
@spastorino
Copy link
Member Author

@bors rollup=always

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Mar 5, 2021
@GuillaumeGomez
Rollup merge of rust-lang#82736 - spastorino:mir-opt-level-perf-chang…
be6d958 
…es, r=oli-obk

Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default

r? `@oli-obk`
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 5, 2021
Closed
@GuillaumeGomez
Copy link
Member

Seems like it failed in #82811.

@bors: 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 Mar 5, 2021
@spastorino
Copy link
Member Author

@GuillaumeGomez yeah, checking locally and will push a fix in a couple of minutes ...

@GuillaumeGomez
Copy link
Member

I'll wait before making another rollup then. ;)

@spastorino spastorino force-pushed the mir-opt-level-perf-changes branch from 18fb025 to 11d9390 Compare March 5, 2021 20:41
@spastorino
Copy link
Member Author

@bors r=oli-obk rollup=always

@bors
Copy link
Contributor

bors commented Mar 5, 2021

📌 Commit 11d9390 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 Mar 5, 2021
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 5, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2021
@bors
Auto merge of rust-lang#82816 - GuillaumeGomez:rollup-hxohu2e, r=Guil…
51748a8 
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#80845 (Make ItemKind::ExternCrate looks like hir::ItemKind::ExternCrate to make transition over hir::ItemKind simpler)
 - rust-lang#82708 (Warn on `#![doc(test(...))]` on items other than the crate root and use future incompatible lint)
 - rust-lang#82714 (Detect match arm body without braces)
 - rust-lang#82736 (Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default)
 - rust-lang#82782 (Make rustc shim's verbose output include crate_name being compiled.)
 - rust-lang#82797 (Update tests names to start with `issue-`)
 - rust-lang#82809 (rustdoc: Use substrings instead of split to grab enum variant paths)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 15c148b into rust-lang:master Mar 6, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 6, 2021
@JohnTitor JohnTitor mentioned this pull request Mar 7, 2021
Merged
@camelid camelid added the A-mir-opt Area: MIR optimizations label Mar 8, 2021
@oli-obk oli-obk mentioned this pull request Mar 9, 2021
Merged
bors added a commit to rust-lang/miri that referenced this pull request Mar 9, 2021
@bors
Auto merge of #1737 - rust-lang:oli-obk-patch-1, r=oli-obk
c765eff 
mir-opt-level 4 is the new 3

cc rust-lang/rust#82736
flip1995 pushed a commit to flip1995/rust that referenced this pull request Mar 11, 2021
@GuillaumeGomez
Rollup merge of rust-lang#82736 - spastorino:mir-opt-level-perf-chang…
6b40d12 
…es, r=oli-obk

Bump optimization from mir_opt_level 2 to 3 and 3 to 4 and make "release" be level 2 by default

r? `@oli-obk`
@mati865 mati865 mentioned this pull request Mar 14, 2021
Closed
@wesleywiser wesleywiser mentioned this pull request Oct 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluss bluss bluss left review comments

@oli-obk oli-obk oli-obk approved these changes

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.