rebuild LLVM when bootstrap.toml config changes

[folkertdev]

View all comments

When using a local build of LLVM, previously changes in bootstrap.toml would not trigger an LLVM rebuild (e.g. when adding an additional target, or enabling assertions). Create a cache key based on the config, and incorporate it into the hash that is used to decide when to rebuild.

r? Kobzol

[rustbot]

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

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

[rustbot]

Kobzol is not on the review rotation at the moment.
They may take a while to respond.

[Kobzol]

I tried to add a target to llvm.targets (on main), and it did trigger a rebuild 😆 But it was only because I stopped the following Rust compilation in the middle, once I let it finish, it indeed does not rebuild.

While this cache busting is not perfect (e.g. it won't detect changing the host CC/CXX toolchain), it sounds very reasonabe to me.

One slightly annoying thing is that if you go from config A to config B and back, LLVM is still re-linked and rustc is rebuilt from rustc_llvm, because we only keep the latest hash. Before, you could modify the config arbitrarily and had a manual way to break the cache.

I'm also not sure why is it happening, but sometimes if I change the config, and then execute ./x build compiler, it will also relink LLVM on the second execution, rather than just on the first (without changing anything in the meantime). Does that also happen to you? It was kinda hard to reproduce for me though, maybe I did something wrong.

CC @nikic Would you be okay with bootstrap rebuilding LLVM when it detects a LLVM-relevant bootstrap config change?

View changes since this review

[nikic]

CC @nikic Would you be okay with bootstrap rebuilding LLVM when it detects a LLVM-relevant bootstrap config change?

Sounds fine to me

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Kobzol]

Thank you, let's try!

@bors r+

[rust-bors]

📌 Commit 48e7c28 has been approved by Kobzol

It is now in the queue for this repository.

[panstromek]

For #158228

@rust-timer build 26cf92e

[rust-timer]

Finished benchmarking commit (26cf92e): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

Results (primary 3.7%, secondary 4.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.7%	[0.5%, 10.2%]	159
Regressions ❌ (secondary)	4.9%	[0.8%, 11.7%]	273
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.7%	[0.5%, 10.2%]	159

Cycles

Results (primary 4.4%, secondary 4.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	4.4%	[2.0%, 8.3%]	66
Regressions ❌ (secondary)	4.9%	[2.1%, 8.7%]	44
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-6.6%, -3.1%]	3
All ❌✅ (primary)	4.4%	[2.0%, 8.3%]	66

Binary size

Results (secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 480.522s -> 503.535s (4.79%)
Artifact size: 391.37 MiB -> 353.07 MiB (-9.79%)

[panstromek]

Hey, this PR seems to have caused pretty mysterious perf results in #158228:

• Max RSS regressions across many benchmarks
• Wall time regressions across many benchmarks
• close to 5% regression on bootstrap
• big improvements in artifact size, the most significant is 20% reduction in LibLLVM.so

This indicates that we probably do something suspicious with LLVM, maybe we unintentionaly rebuild it on CI with different config or something like that?

I'll re-add the label for now. The bot removed it because it's not visible in instruction counts, but it's pretty clear on other metrics that something suspicious is going on.

@rustbot label +perf-regression

[folkertdev]

That any of the code here runs in CI is surprising to me. We don't actually build LLVM in CI right? Building llvm_cache_key should be pretty quick, and otherwise none of the code should run.

[nikic]

Based on the results, I'd guess this broke BOLT optimizations.

We don't actually build LLVM in CI right?

Not sure what you mean by this, but we do build LLVM on all the dist builders, even if it did not change.

[panstromek]

Given that we don't really understand what's going on and the regression looks pretty broad, I think we should probably revert this for now?

[Kobzol]

Ah, crap, yeah, this will break the assumptions made by opt-dist about LLVM not being rebuilt in the final step, I didn't realize that. Yeah, this should be reverted ASAP (I can't do it atm though, as I'm only on phone this week).

[folkertdev]

I'll set up the revert, and we can think about how to work around that issue when you're back.