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

[needs perf run] Try to improve LLVM pass ordering #46739

Merged
merged 2 commits into from
Jan 5, 2018
Merged

[needs perf run] Try to improve LLVM pass ordering #46739

merged 2 commits into from
Jan 5, 2018

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Dec 15, 2017 •
edited
Loading

Fixes #45466

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 15, 2017

@bors try

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@bors
Copy link
Contributor

bors commented Dec 15, 2017

⌛ Trying commit c3763ed with merge 1e8a44a...

bors added a commit that referenced this pull request Dec 15, 2017
@bors
Auto merge of #46739 - arielb1:simple-loops, r=<try>
1e8a44a 
[needs perf run] Simplify CFG after IndVarSimplify

Fixes #45466
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 15, 2017

Let's check the perf impact of this

@bors
Copy link
Contributor

bors commented Dec 15, 2017

☀️ Test successful - status-travis
State: approved= try=True

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 15, 2017

@Mark-Simulacrum

Could you do a perf run on this?

@Mark-Simulacrum
Copy link
Member

Not quite right now as I don't have a connection to the perf machine, I'm waiting on @alexcrichton for it... hopefully next week after all hands, though.

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2017
@alexcrichton
Copy link
Member

Yes unfortunately the perf machine is offline right now, we'll have access to it again this coming Monday at the latest.

@kennytm
Copy link
Member

kennytm commented Dec 20, 2017

@Mark-Simulacrum @alexcrichton Is the perf machine ready now?

@alexcrichton
Copy link
Member

Ah yes, it is indeed back online.

@Mark-Simulacrum
Copy link
Member

Perf run queued.

@kennytm
Copy link
Member

kennytm commented Dec 22, 2017

http://perf.rust-lang.org/compare.html?start=2f351415e53b9214d9a536f84f232a58f46698b6&end=1e8a44a33d146447b90c16be3dd9d95aa69bcfd3&stat=instructions%3Au

Almost no impact in terms of instructions:u (all values within ±2%).

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 22, 2017

@kennytm

I do see a clear 1-2% regression in compile-time for opt builds

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 23, 2017

Now with a more aggressive try.

I think this needs a merge of my PR on rust-lang/llvm, and then I can try?

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 25, 2017 •
edited
Loading

@Mark-Simulacrum

Can I have a new perf run on this more aggressive pass ordering?

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 25, 2017 •
edited
Loading

This also seems to fix #46542. Looks like the earlier IndVarSimplify is really helping with ranges.

@arielb1 arielb1 changed the title [needs perf run] Simplify CFG after IndVarSimplify [needs perf run] Try to improve LLVM pass ordering Dec 25, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 25, 2017

@bors try

@bors
Copy link
Contributor

bors commented Dec 25, 2017

⌛ Trying commit 6163df4 with merge 8a453b2...

bors added a commit that referenced this pull request Dec 25, 2017
@bors
Auto merge of #46739 - arielb1:simple-loops, r=<try>
8a453b2 
[needs perf run] Try to improve LLVM pass ordering

Fixes #45466
@bors
Copy link
Contributor

bors commented Dec 25, 2017

☀️ Test successful - status-travis
State: approved= try=True

@Mark-Simulacrum
Copy link
Member

Perf queued.

@kennytm
Copy link
Member

kennytm commented Dec 27, 2017

http://perf.rust-lang.org/compare.html?start=4a7c072fa61b42f96d8b75c37fc1edfd71172695&end=8a453b274af5c133ed55a5813e5b36f87afe33d3&stat=instructions%3Au

deep-vector-opt is 8% slower (6% more instructions), regex-opt is ~3% slower

@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 3, 2018
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 3, 2018 •
edited
Loading

Nominating for discussion in the @rust-lang/compiler meeting. This causes a small regression in compile time, but enables us to optimize #45466 again. Should we land? I think yes.

@nikomatsakis
Copy link
Contributor

@bors r+

After discussion in the compiler meeting we decided to go forward with this PR. However, there is a sense that we need to develop a more comprehensive runtime perf suite so that we can have a better idea of the overall impact of changes like this.

@bors
Copy link
Contributor

bors commented Jan 4, 2018

📌 Commit 6163df4 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jan 4, 2018

⌛ Testing commit 6163df4 with merge 4fd62f2...

bors added a commit that referenced this pull request Jan 4, 2018
@bors
Auto merge of #46739 - arielb1:simple-loops, r=nikomatsakis
4fd62f2 
[needs perf run] Try to improve LLVM pass ordering

Fixes #45466
@bors
Copy link
Contributor

bors commented Jan 4, 2018

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Jan 4, 2018

@bors retry #46903 3 hour timeout

It is happening very frequently recently 😞

@bors
Copy link
Contributor

bors commented Jan 5, 2018

⌛ Testing commit 6163df4 with merge 5e66887...

bors added a commit that referenced this pull request Jan 5, 2018
@bors
Auto merge of #46739 - arielb1:simple-loops, r=nikomatsakis
5e66887 
[needs perf run] Try to improve LLVM pass ordering

Fixes #45466
@bors
Copy link
Contributor

bors commented Jan 5, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 5e66887 to master...

@bors bors merged commit 6163df4 into rust-lang:master Jan 5, 2018
@remexre remexre mentioned this pull request Jan 23, 2018
Closed
@alexcrichton alexcrichton mentioned this pull request Jan 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@arielb1 @rust-highfive @bors @Mark-Simulacrum @alexcrichton @kennytm @nikomatsakis