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

Enable const propagation into operands at mir_opt_level=2 #77107

Merged
merged 1 commit into from
Sep 27, 2020

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Sep 23, 2020

Feature was added in #74507 but gated with mir_opt_level>=3 because of compile time regressions. Let's see whether the LLVM 11 update solves that.

As the perf results show, enabling this optimization results in a lot less regression as before.

cc @oli-obk

r? @ghost

@wesleywiser
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 23, 2020

⌛ Trying commit 6ebd9dcb210c82a59ce07351dcead6f255632b8a with merge 8ead925df872b31e6ed360837751ac75e873642b...

@bors
Copy link
Contributor

bors commented Sep 23, 2020

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

@rust-timer
Copy link
Collaborator

Queued 8ead925df872b31e6ed360837751ac75e873642b with parent 33aa8be, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (8ead925df872b31e6ed360837751ac75e873642b): 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

@oli-obk
Copy link
Contributor

oli-obk commented Sep 23, 2020

Looks like this doesn't cause additional codegen units anymore, but still causes a slowdown in llvm optimizing

@jyn514
Copy link
Member

jyn514 commented Sep 23, 2020

The slowdown seems not too bad, and it looks like some of the performance actually increased, maybe because rustc is more optimized itself.

@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. labels Sep 23, 2020
@bugadani
Copy link
Contributor Author

How can you tell there's a slowdown wrt LLVM?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 23, 2020

You can expand the entry and then click on the percentage. That opens a new view: https://perf.rust-lang.org/detailed-query.html?commit=8ead925df872b31e6ed360837751ac75e873642b&base_commit=33aa8be8b5fa6872186a94d9e1fa5088da386e4b&benchmark=regex-opt&run_name=full

@mati865
Copy link
Contributor

mati865 commented Sep 23, 2020

maybe because rustc is more optimized itself.

It seems faster in some places but slower in other.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 23, 2020

This shows that at the very least we should set it as mir-opt-level=2, there's no reason to keep in in level 3 anymore

@wesleywiser
Copy link
Member

The tests that improved the most are ctfe (compile time function evaluation) stress tests. Most of the real-world tests didn't show as much improvement so, as @oli-obk said, this seems like a good candidate for mir-opt-level=2.

@bugadani
Copy link
Contributor Author

Is it okay if I do it in this PR, just by setting in the same place as the current diff, or is there something else to do? Also, what are the criteria for the different mir-opt-levels? It's not really well documented as far as I can see.

@wesleywiser
Copy link
Member

Yes, it's fine to do in this PR, I'd just adjust the PR title to make it clear what's going on.

The criteria we're using in for mir-opt-levels comes from this compiler-team Major Change Proposal: rust-lang/compiler-team#319. We should figure out how to document that better, perhaps by upstreaming that policy to the rustc-guide or something?

@bugadani bugadani changed the title Measure the impact of const propagation into operands after LLVM 11. Enable const propagation into operands at mir_opt_level=2 Sep 23, 2020
@bugadani
Copy link
Contributor Author

Thank you. I've adjusted the comments as well to indicate that the current level of slowdown is not as severe as before, but still exists, I hope that's all right.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 23, 2020

@bors r+

that's great. Thanks!

@bors
Copy link
Contributor

bors commented Sep 23, 2020

📌 Commit 90c7731 has been approved by oli-obk

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 23, 2020
@ecstatic-morse
Copy link
Contributor

@bors rollup-

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2020
…as-schievink

Rollup of 10 pull requests

Successful merges:

 - rust-lang#76917 (Add missing code examples on HashMap types)
 - rust-lang#77107 (Enable const propagation into operands at mir_opt_level=2)
 - rust-lang#77129 (Update cargo)
 - rust-lang#77167 (Fix FIXME in core::num test: Check sign of zero in min/max tests.)
 - rust-lang#77184 (Rust vec bench import specific rand::RngCore)
 - rust-lang#77208 (Late link args order)
 - rust-lang#77209 (Fix documentation highlighting in ty::BorrowKind)
 - rust-lang#77231 (Move helper function for `missing_const_for_fn` out of rustc to clippy)
 - rust-lang#77235 (pretty-print-reparse hack: Rename some variables for clarity)
 - rust-lang#77243 (Test more attributes in test issue-75930-derive-cfg.rs)

Failed merges:

r? `@ghost`
@bors bors merged commit 6b8fb3f into rust-lang:master Sep 27, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 27, 2020
@bugadani bugadani deleted the perf branch September 27, 2020 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. I-compiletime Issue: Problems and improvements with respect to compile times. 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