rebuild LLVM when bootstrap.toml config changes#157836
Conversation
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. This PR modifies If appropriate, please update |
rustbot
added
S-waiting-on-review
|
|
There was a problem hiding this comment.
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?
|
|
||
| static STAMP_HASH_MEMO: OnceLock<String> = OnceLock::new(); | ||
| let smart_stamp_hash = STAMP_HASH_MEMO.get_or_init(|| { | ||
| // Key LLVM on the commit hash and the configuration from bootstrap.toml. |
There was a problem hiding this comment.
I wonder if we should do this also for offload, compiler-rt and other LLVM subprojects built in this file using the same mechanism? I think it would make sense (in that case the keying logic should be extracted to a separate function).
There was a problem hiding this comment.
I'm not sure what you have in mind here? offload is already part of the LLVM config struct so toggling that will rebuild. For compiler-rt I'm not sure how to hook it up. If it hasn't been a problem so far the people using that feature can add it later?
There was a problem hiding this comment.
I meant things like this. We compute the stamp hash in 4 places in this file currently (LLVM, enzyme, offload, compiler-rt), but only currently the config only affects the LLVM build.
In particular, I'm not sure if it's OK if LLVM rebuilds (due to a bootstrap config change), but then Enzyme or offload wouldn't be rebuilt - would they still work? @ZuseZ4 Any thoughts on that? :)
There was a problem hiding this comment.
When we rebuild LLVM, we should rebuild offload and enzyme afterwards, otherwise it's a bit of a coin flip whether they continue to work, it will depend on what changed.
There was a problem hiding this comment.
Okay, that makes sense. In that case, could you please extract the hash stamp logic for all usages in the llvm.rs file and reuse it also for compiler-rt, enzyme and offload, Folkert? Thank you!
Sounds fine to me |
folkertdev
force-pushed
the
rebuild-llvm-on-bootstrap-change
branch
from
89fb475 to
0fb4a0c
Compare
This comment has been minimized.
This comment has been minimized.
folkertdev
force-pushed
the
rebuild-llvm-on-bootstrap-change
branch
from
0fb4a0c to
48e7c28
Compare
|
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. |
5 participants
When using a local build of LLVM, previously changes in
bootstrap.tomlwould not trigger an LLVM rebuild (e.g. when adding an additional target, or enablingassertions). Create a cache key based on the config, and incorporate it into the hash that is used to decide when to rebuild.r? Kobzol