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

Move BOLT from bootstrap to opt-dist #113592

Merged
merged 3 commits into from
Jul 31, 2023
Merged

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Jul 11, 2023

Currently, we use BOLT to optimize LLVM for x64 Linux. The BOLT instrumentation and optimization step is implemented in bootstrap, but it was always quite hacky, because BOLT works quite differently than PGO. Rather than building an instrumented artifact, it takes an already built artifact and instruments it in-place. This is not a good fit for the bootstrap caching mechanism, and it meant that we had to run BOLT "on-the-fly" when packaging LLVM artifacts into the created sysroot.

The BOLT code was also really only used by the PGO script (now called opt-dist) and nothing else, so it was quite hardcoded for this one single usage. And even if someone wanted to use the --llvm-bolt-profile-[use/generate] bootstrap flags outside of the PGO script, they would also need to implement profile gathering, as this is not performed by bootstrap anyway.

I think that it could be more practical to move the BOLT logic to the opt-dist tool instead. This simplifies bootstrap, removes unneeded implementation of BOLT caching (we will now do it exactly once - no need to check if it has been performed already when bootstrap copies libLLVM.so around multiple times) and removes two BOLT-specific bootstrap flags, and also one special case for building LLVM (instead I pass the linker flags with --set llvm.ldflags from opt-dist now).

There are also a few disadvantages to this new approach:

  • We have to guess/find the path to the built libLLVM.so file (but currently this is quite easy, it's just in stage2/lib).
  • We have to provide the BOLT profile externally to bootstrap, so that it is packaged into the reproducible artifacts archive. Doesn't seem like a big deal to me.
  • We are depending on some inner behavior of boostrap in opt-dist (namely, that libLLVM.so is hardlinked). But we do that for many other things in the opt-dist tool anyway, it's tied quite closely to bootstrap.

I would like to go back to my attempts to also use BOLT for librustc_driver.so, and I think that it might be a bit simpler if I also do it from the opt-dist tool, so this is the first step towards that.

Anyway, let me know what you think about this. It's just a refactoring in a way, no big deal.

r? bootstrap

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@jyn514 jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 12, 2023
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 12, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 12, 2023
@bors
Copy link
Contributor

bors commented Jul 12, 2023

⌛ Trying commit 09bc296902727636018054eadc15588c2f4d4826 with merge 055c6de761070321adc987bdf8e6fcd5ccff7fa9...

@onur-ozkan
Copy link
Member

I also want to see Mark's review on this.

r? @Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 12, 2023

☀️ Try build successful - checks-actions
Build commit: 055c6de761070321adc987bdf8e6fcd5ccff7fa9 (055c6de761070321adc987bdf8e6fcd5ccff7fa9)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (055c6de761070321adc987bdf8e6fcd5ccff7fa9): comparison URL.

Overall result: no relevant changes - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 657.52s -> 659.403s (0.29%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 12, 2023
@Kobzol Kobzol force-pushed the pgo-script-bolt branch from 09bc296 to 58bc8af Compare July 17, 2023 13:52
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 17, 2023

Rebased.

@bors
Copy link
Contributor

bors commented Jul 29, 2023

☔ The latest upstream changes (presumably #114141) made this pull request unmergeable. Please resolve the merge conflicts.

@Kobzol Kobzol force-pushed the pgo-script-bolt branch from 58bc8af to dcf7413 Compare July 29, 2023 15:30
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 29, 2023

Rebased to incorporate #114141.

@rust-log-analyzer

This comment has been minimized.

@Kobzol Kobzol force-pushed the pgo-script-bolt branch from dcf7413 to ced630a Compare July 29, 2023 15:46
@bors
Copy link
Contributor

bors commented Jul 31, 2023

☔ The latest upstream changes (presumably #114126) made this pull request unmergeable. Please resolve the merge conflicts.

@Kobzol Kobzol force-pushed the pgo-script-bolt branch from ced630a to 142995f Compare July 31, 2023 06:54
@Kobzol
Copy link
Contributor Author

Kobzol commented Jul 31, 2023

Gentle ping @Mark-Simulacrum, just to make sure that this isn't forgotten.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jul 31, 2023

📌 Commit 142995f has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 Jul 31, 2023
@bors
Copy link
Contributor

bors commented Jul 31, 2023

⌛ Testing commit 142995f with merge 3114eb1...

@bors
Copy link
Contributor

bors commented Jul 31, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 3114eb1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2023
@bors bors merged commit 3114eb1 into rust-lang:master Jul 31, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 31, 2023
@Kobzol Kobzol deleted the pgo-script-bolt branch July 31, 2023 16:30
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3114eb1): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.1% [-1.2%, -1.1%] 4
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)
-3.0% [-3.0%, -3.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.0% [-3.0%, -3.0%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 652.068s -> 649.988s (-0.32%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants