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

Don't run various MIR optimizations at mir-opt-level=0 #70073

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

wesleywiser
Copy link
Member

Add missing checks for mir-opt-level to non-essential MIR passes.

I verified that this can still bootstrap even with these passes disabled.

r? @oli-obk cc @RalfJung

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

oli-obk commented Mar 17, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2020

📌 Commit d158587eb76428d060bb08170813e8b9ada881aa 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 17, 2020
@wesleywiser
Copy link
Member Author

@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 17, 2020
src/librustc_mir/transform/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/mod.rs Outdated Show resolved Hide resolved
Comment on lines 356 to 334
// Deaggregation isn't technically an optimization but it unlocks opportunities
// for later optimization passes.
&deaggregator::Deaggregator,
Copy link
Member

Choose a reason for hiding this comment

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

But then why is it not run earlier?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know but I think the only passes that would be affected by this are CopyPropagation and SimplifyLocals which both run after this one.

Copy link
Member

Choose a reason for hiding this comment

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

@eddyb said "people just probably didn't care much about it and added passes before and after".

So I'd vote for moving it up to the other lowering passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall someone mentioning that drop elaboration relies on aggregates still being intact, and the generator transform performs drop elaboration. Not very sure though.

@bors

This comment has been minimized.

@joelpalmer joelpalmer 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 Mar 30, 2020
@Dylan-DPC-zz
Copy link

@wesleywiser you have conflicts to resolve

@wesleywiser
Copy link
Member Author

wesleywiser commented Mar 30, 2020

I can resolve them but we're really waiting on some feedback from @oli-obk 🙂

@jonas-schievink jonas-schievink added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 6, 2020
@joelpalmer joelpalmer 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 Apr 16, 2020
@wesleywiser wesleywiser 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 Apr 16, 2020
@wesleywiser
Copy link
Member Author

@bors retry

@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 Apr 26, 2020
@bors
Copy link
Contributor

bors commented Apr 26, 2020

⌛ Testing commit f4dfb99 with merge fadd2a48ae5148f9181745a3956dee1523d291c5...

@bors
Copy link
Contributor

bors commented Apr 26, 2020

💔 Test failed - checks-azure

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

Setting up python3-setuptools (20.7.0-1) ...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (7) Couldn't connect to server
tar: This does not look like a tar archive
tar: Exiting with failure status due to previous errors

##[error]Bash exited with code '2'.

Network failure.

@bors retry

@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 Apr 26, 2020
@bors
Copy link
Contributor

bors commented Apr 27, 2020

⌛ Testing commit f4dfb99 with merge d81f559...

@bors
Copy link
Contributor

bors commented Apr 27, 2020

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing d81f559 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 27, 2020
@bors bors merged commit d81f559 into rust-lang:master Apr 27, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 28, 2020
…chievink

smoke-test for async fn with mir-opt-level=0

MIR opt levels heavily influence which MIR transformations run, and we barely test non-default opt levels. I am particularly worried about `async fn` lowering and how it might (not) work when the set of preceding MIR passes changes -- see rust-lang#70073.

This adds some basic smoke testing, where at least a few `async fn` `run-pass` test are ensured to also work with mir-opt-level=0.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 11, 2020
…leywiser

move Deaggregate pass to post_borrowck_cleanup

Reopen of rust-lang#71946

Only the second commit is from this PR, the other commit is a bugfix that's in the process of getting merged. I'll rebase once that's done

In rust-lang#70073 MIR pass handling got reorganized, but with the goal of not changing behavior (except for disabling some optimizations on opt-level = 0). But there we realized that the Deaggregator pass, while conceptually more of a "cleanup" pass (and one that should be run before optimizations), was run in the middle of the optimization chain. Likely this is an accident of history, so I suggest we try and clean that up by making it a proper cleanup pass.

This does change mir-opt output, because deaggregation now runs before const-prop instead of after.

r? @wesleywiser @rust-lang/wg-mir-opt

cc @RalfJung
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.

10 participants