Assorted bootstrap LLVM refactors (part 1/N) - #160645
Conversation
|
This PR modifies If appropriate, please update This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
(Will take a look later today) |
There was a problem hiding this comment.
Thanks, r=me after a rebase.
@bors rollup=never note="bootstrap llvm refactors"
|
@rustbot author |
At this point in the code, the LLVM config could not have been set by `download-ci-llvm` yet, so we don't have to check it.
To make it consistent with `GccOutput`
So that it can be used explicitly in bootstrap, rather than depending on `builder.llvm_out`.
To remove dependency on implicit paths.
And replace it with an explicit `FileCheck` step
To avoid someone depending on implicit paths. Instead, the step should be executed and the path should be taken from its output.
…-llvm` is enabled Note: this commit removed reading `FileCheck` from `<artifact-dir>/build/<profile>/bin`, and instead reads it from `<artifact-dir>/bin` directly. It should still work with MSVC and without Ninja.
|
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. |
|
@bors r=jieyouxu |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing ea06042 (parent) -> ef20314 (this PR) Test differencesNo test diffs found Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard ef20314466010b8b9259ec5f86230c530ca08661 --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (ef20314): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -2.9%, secondary 0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 453.486s -> 455.478s (0.44%) |
Assorted bootstrap LLVM refactors (part 2/N) 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 compiler` does not build or even checkout the LLVM submodule. That being said, there might be some cases where calling `get_llvm_build_status` might now checkout the LLVM submodule (but it shouldn't happen for `x 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 to `false`/`true`). The next thing after this is to try getting rid of the `llvm_out` function, though that will be probably quite painful. And also explicitly depending on the output of `LlvmFromCi`, rather than reading `llvm_ci_mode`. I printed some step traces using commands like this: ``` BOOTSTRAP_TRACING=STEP=trace BOOTSTRAP_TRACING_SKIP_TIME=1 ./x build compiler 2> post-cg_default-build-compiler.txt ``` with this PR and on `main`. The `BOOTSTRAP_TRACING_SKIP_TIME` thing 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
This PR continues my LLVM and
download-ci-llvmbootstrap refactors (it took me almost a year to get back to them, lol), with the goals of:download-ci-llvm.Llvmstep, and reduce overall knowledge between locally built and downloaded LLVM. Ideally, most of bootstrap shouldn't know about whether it uses a LLVM that was built locally or downloaded. But that is of course not the case today, because there are many places in bootstrap that ad-hoc work with some LLVM paths, and do things that depend on some part of bootstrap randomly modifying a config here or there, or doing some side effect, which then makes the other activity "work". But of course, this is very fragile, as we know.download-ci-rustc. Those two features are quite interrelated, and sadly both are scattered across the codebase. I actually first wanted to start with refactoringdownload-ci-rustc, but I couldn't find a way to do that without first improvingdownload-ci-llvm.This PR contains a bunch of commits that slowly move us towards these goals. The refactors were quite explanatory, they are mostly a preparation for larger refactors that I had in mind, so nothing major. But as always, step by step.
As usually, best reviewed commit-by-commit.
Some notes for review:
download-ci-llvm. As per the experiment on Zulip, this should be fine. And I'm not sure if anyone actually uses MSVC without Ninja in bootstrap, it seems to be quite broken.FileCheckstep if you want).r? jieyouxu