Assorted bootstrap LLVM refactors (part 2/N) - #160916
Conversation
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. This PR modifies If appropriate, please update |
This comment has been minimized.
This comment has been minimized.
7c61c77 to
7caa3d5
Compare
This comment has been minimized.
This comment has been minimized.
To make it easier to compare tracing step outputs.
This allows explicitly depending on LLVM from CI in other parts of bootstrap.
So that the function can run steps.
To avoid implicitly depending on the directory where LLVM CI is extracted.
So that we can later introduce `prebuilt_llvm_config` again, which will never try to checkout the LLVM submodule.
… available Without checking the LLVM submodule.
02c1b49 to
53bd738
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. |
There was a problem hiding this comment.
Makes sense to me overall, just a few nits
@rustbot author
| // FIXME: llvm_ci_mode should eventually represent what was used in the config, not the | ||
| // dynamic value used for determining whether it is actually available. | ||
| let llvm_ci_mode = | ||
| if llvm_from_ci { LlvmCiMode::DownloadFromCi } else { LlvmCiMode::BuildLocally }; |
There was a problem hiding this comment.
Remark: hm, right, there's two concerns here:
- What LLVM CI mode was requested via config
- Whether CI LLVM is actually elligble wrt. the config
There was a problem hiding this comment.
Yeah there are even more of those :D
- What did you request?
- Is it compatible with your config?
- Is LLVM CI available for your target?
- Is LLVM CI available for your value of assertions?
- Is the LLVM target the host target? (if not, download-ci-llvm doesn't do anything right now)
- Was it actually possible to download it from the servers?
- If you have
if-unchanged, do you have local modifications?
|
|
|
@rustbot ready |
There was a problem hiding this comment.
Thanks
@bors r+ rollup=never note="CI LLVM refactors"
Continuation of #160645.
This PR removes dependence on implicit LLVM CI paths, and generally tries to make depending on various LLVM things more explicit wherever possible. One nice outcome of that is that we no longer override the LLVM config during config parsing when using
download-ci-llvm🎉 We also no longer dynamically download LLVM from CI when someone asks what is the LLVM linkage mode, lol (well, we still need to do it in a few places, but now at least it is explicit).I tried to ensure that
x check compilerdoes not build or even checkout the LLVM submodule. That being said, there might be some cases where callingget_llvm_build_statusmight now checkout the LLVM submodule (but it shouldn't happen forx test mir-opt --bless, which was fixed in #130918 - that PR introduced a boolean flag, but it was difficult to understand where it should be set tofalse/true).The next thing after this is to try getting rid of the
llvm_outfunction, though that will be probably quite painful. And also explicitly depending on the output ofLlvmFromCi, rather than readingllvm_ci_mode.I printed some step traces using commands like this:
with this PR and on
main.The
BOOTSTRAP_TRACING_SKIP_TIMEthing was added in the first commit to make it easier to compare step traces. But later I realized that we should also normalize source code locations of the steps, otherwise if you modify something in a given file with the step, its location will change and it will perturb the diff. Anyway, I can drop the first commit if you want.Best reviewed commit-by-commit (in fact I don't think it can be really reviewed in another way, lol).
r? jieyouxu