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

Update LLVM used for building rustc in CI for x64 #95171

Merged
merged 2 commits into from
Apr 28, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Mar 21, 2022

LLVM 14 was tagged. It is needed for building BOLT, and it also improves max RSS quite a lot.

GCC was bumped to allow building the new LLVM version. The changes in building LLVM were done to speed up the build a bit by ignoring unused parts and to turn off parts that were problematic to compile even with the bumped GCC.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2022
@cuviper
Copy link
Member

cuviper commented Mar 23, 2022

FYI, llvmorg-14.0.0 was tagged yesterday, and see also #95247 for the submodule update (independent of CI tools).

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 24, 2022

Ok, let's test this bad boy! Could someone do a perf. run please?

(actually, let's see if I can do it since being added to wg-compiler-performance)

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2022
@bors
Copy link
Collaborator

bors commented Mar 24, 2022

⌛ Trying commit ce1a59d57650ff3c53576b784fe9b6191ac14164 with merge 67acb7e3ffcc11d67abfd29c390aac629f3c0e97...

@Kobzol Kobzol marked this pull request as ready for review March 24, 2022 07:56
@bors
Copy link
Collaborator

bors commented Mar 24, 2022

☀️ Try build successful - checks-actions
Build commit: 67acb7e3ffcc11d67abfd29c390aac629f3c0e97 (67acb7e3ffcc11d67abfd29c390aac629f3c0e97)

@rust-timer
Copy link
Collaborator

Queued 67acb7e3ffcc11d67abfd29c390aac629f3c0e97 with parent 6970f88, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (67acb7e3ffcc11d67abfd29c390aac629f3c0e97): comparison url.

Summary: This benchmark run did not return any relevant results. 29 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2022
@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 24, 2022

Icounts and cycles do not seem very interesting, but max RSS was improved quite a lot.

@Mark-Simulacrum Do you want me to investigate what is the source of this improvement (e.g. by trying to use GCC 7.5 with old LLVM) or should we merge this so that I can continue working on integrating BOLT?

@Mark-Simulacrum
Copy link
Member

I think it would be good to know so we know where to poke if it regresses in a future bump like this. But I wouldn't spend more than a few hours or so on it (i.e. not that important to know).

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 24, 2022

Ok. Since the new LLVM cannot be built with older GCC (at least not out of the box), the only thing that comes to mind is benchmarking just the GCC update. Let's see what happens.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Collaborator

bors commented Mar 24, 2022

⌛ Trying commit 7aff6125ec092caa3b6d8f395063e033205bc5c5 with merge 171dabe2f735f47079811f53d4168a86f15c7a0c...

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2022
@bors
Copy link
Collaborator

bors commented Mar 24, 2022

☀️ Try build successful - checks-actions
Build commit: 171dabe2f735f47079811f53d4168a86f15c7a0c (171dabe2f735f47079811f53d4168a86f15c7a0c)

@rust-timer
Copy link
Collaborator

Queued 171dabe2f735f47079811f53d4168a86f15c7a0c with parent d2df372, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (171dabe2f735f47079811f53d4168a86f15c7a0c): comparison url.

Summary: This benchmark run did not return any relevant results. 16 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 25, 2022
@lqd
Copy link
Member

lqd commented Mar 25, 2022

max-rss is improved by a bit, and much less than the previous run: so the new LLVM was indeed the main cause of that ?

@Kobzol
Copy link
Contributor Author

Kobzol commented Mar 25, 2022

Indeed it seems that not bumping LLVM doesn't provide as much improvement.

@Mark-Simulacrum Do you want me to investigate anything else?

@bors
Copy link
Collaborator

bors commented Apr 27, 2022

⌛ Testing commit 24c31694dc60956ffacfb539e04bc08cd1deb904 with merge d5bf8c45923c1a067d31cccb77e4f3a1869f5bb8...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Apr 27, 2022

💔 Test failed - checks-actions

@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 27, 2022
@Mark-Simulacrum
Copy link
Member

It looks like that's the same issue as before?

@nikic
Copy link
Contributor

nikic commented Apr 27, 2022

Yeah, looks like the fix was not sufficient in some way :(

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 28, 2022

Oh, I see. The fix was done on the "library" side of LLD. Therefore we also need to update the embedded version of llvm-project (we need to update what is being compiled, not the compiler itself) inside the rustc repository for it to compile. I'll check locally to make sure that it's correct.

What's the process here? Should I cherry-pick the backported commit to the embedded LLVM?

@nikic
Copy link
Contributor

nikic commented Apr 28, 2022

Oh, I see. The fix was done on the "library" side of LLD. Therefore we also need to update the embedded version of llvm-project (we need to update what is being compiled, not the compiler itself) inside the rustc repository for it to compile. I'll check locally to make sure that it's correct.

Oh right, that makes sense.

What's the process here? Should I cherry-pick the backported commit to the embedded LLVM?

I've merged current upstream release/14.x into our fork, so you can just update the submodule reference here.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 28, 2022

Hmm, it conflicts. I'll rebase then.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 28, 2022

I built LLVM 14.0.2 locally and used it to cross-compile the embedded LLVM to the i386 target. It didn't compile with the old submodule commit, but compiled with the new commit. It should now thus also work on CI (:crossed_fingers:).

@nikic
Copy link
Contributor

nikic commented Apr 28, 2022

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Apr 28, 2022

📌 Commit 4472d4c has been approved by nikic

@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 28, 2022
@bors
Copy link
Collaborator

bors commented Apr 28, 2022

⌛ Testing commit 4472d4c with merge 1388b38...

@bors
Copy link
Collaborator

bors commented Apr 28, 2022

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 1388b38 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 28, 2022
@bors bors merged commit 1388b38 into rust-lang:master Apr 28, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 28, 2022
@bors bors mentioned this pull request Apr 28, 2022
8 tasks
@Kobzol Kobzol deleted the llvm-ci-update branch April 28, 2022 20:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (1388b38): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 0 1 0 1
mean2 N/A N/A -0.5% N/A -0.5%
max N/A N/A -0.5% N/A -0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 28, 2022

Lots of very small icount improvements, but mainly quite nice RSS wins. 🎉

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