Skip to content

Enable const propagation into operands at mir_opt_level=2 - #77107

Merged
bors merged 1 commit into
rust-lang:masterfrom
bugadani:perf
Sep 27, 2020
Merged

Enable const propagation into operands at mir_opt_level=2#77107
bors merged 1 commit into
rust-lang:masterfrom
bugadani:perf

Conversation

@bugadani

@bugadanibugadani commented Sep 23, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

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

wesleywiser commented Sep 23, 2020

Copy link
Copy Markdown
Member

@bors try @rust-timer queue

@rust-timer

rust-timer commented Sep 23, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@bors

bors commented Sep 23, 2020

Copy link
Copy Markdown
Collaborator

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

@bors

bors commented Sep 23, 2020

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Sep 23, 2020

Copy link
Copy Markdown
Collaborator

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

@rust-timer

rust-timer commented Sep 23, 2020

Copy link
Copy Markdown
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

oli-obk commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

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

@jyn514

jyn514 commented Sep 23, 2020

Copy link
Copy Markdown
Member

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

@jyn514jyn514 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

bugadani commented Sep 23, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@oli-obk

oli-obk commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

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

mati865 commented Sep 23, 2020

Copy link
Copy Markdown
Member

maybe because rustc is more optimized itself.

It seems faster in some places but slower in other.

@oli-obk

oli-obk commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

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

wesleywiser commented Sep 23, 2020

Copy link
Copy Markdown
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

bugadani commented Sep 23, 2020

Copy link
Copy Markdown
ContributorAuthor

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

wesleywiser commented Sep 23, 2020

Copy link
Copy Markdown
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?

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

bugadani commented Sep 23, 2020

Copy link
Copy Markdown
ContributorAuthor

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

oli-obk commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

@bors r+

that's great. Thanks!

@bors

bors commented Sep 23, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 90c7731 has been approved by oli-obk

@borsbors 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

ecstatic-morse commented Sep 23, 2020

Copy link
Copy Markdown
Contributor

@bors rollup-

This was referenced Sep 25, 2020
Closed
Closed
Closed
Closed
@jonas-schievinkjonas-schievink mentioned this pull request Sep 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2020
@bors
Auto merge ofrust-lang#77247- jonas-schievink:rollup-r6ehh8h, r=jon…
1ec980d
…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:masterSep 27, 2020
@rustbotrustbot 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

A-LLVMArea: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues.I-compiletimeIssue: Problems and improvements with respect to compile times.S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

9 participants

@bugadani@wesleywiser@rust-timer@bors@oli-obk@jyn514@mati865@ecstatic-morse@rustbot