Assorted bootstrap LLVM refactors (part 3/N) - #161247
Conversation
|
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.
953c4f0 to
cf840fa
Compare
There was a problem hiding this comment.
Thanks, r=me after pr ci green
@bors rollup=never note="bootstrap llvm rework just in case"
|
@bors r=jieyouxu |
|
@bors p=6 scheduling |
This comment has been minimized.
This comment has been minimized.
Assorted bootstrap LLVM refactors (part 3/N) Continuation of #160916. This PR removes the `llvm_out` function, and makes it explicit when we need to build the host LLVM. Before, bootstrap used to just arbitrarily run a `llvm-config` binary, even though it might not have been executable on the given host. Now, if we need to execute it, the host LLVM will always be explicitly built. It is possible that there are some cases where this will build LLVM where it didn't before, but that should only happen if you were on a target A, built LLVM for target B, and by sheer luck A could execute code for B (where A != B). In any case, it is now explicit, so if we encounter such situation, we can fix it without depending on implicit assumptions (well, there are still thousands of other assumptions, but you get my point). After this, I'll work on centralizing the sanity checking of downloading LLVM inside the `LlvmFromCi` step, which will allow us to log the exact reason why `download-ci-llvm` might not have been applied, and also enable downloading LLVM from CI for non-host targets. r? jieyouxu
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 55e7b47 failed: CI. Failed job:
|
|
@bors try jobs=dist-aarch64-freebsd |
This comment has been minimized.
This comment has been minimized.
Assorted bootstrap LLVM refactors (part 3/N) try-job: dist-aarch64-freebsd
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Assorted bootstrap LLVM refactors (part 3/N) try-job: dist-aarch64-freebsd
This comment has been minimized.
This comment has been minimized.
|
💔 Test for ad9d7cd failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
@bors r- |
|
This pull request was unapproved. |
|
@bors try jobs=dist-aarch64-freebsd |
This comment has been minimized.
This comment has been minimized.
Assorted bootstrap LLVM refactors (part 3/N) try-job: dist-aarch64-freebsd
|
@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 347a1db (parent) -> ac62df9 (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard ac62df9b49f9b9036af2a4957db70bf3850785e1 --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 (ac62df9): 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)This perf run didn't have relevant results for this metric. CyclesResults (primary 0.2%, secondary 3.4%)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: 468.927s -> 475.143s (1.33%) |
Assorted bootstrap LLVM refactors (part 4/N) Continuation on #161247. This PR completely removes handling of git changes or LLVM downloads from config parsing, and moves it into the `LlvmFromCi` step. Thanks to that, we now also allow downloading LLVM for non-host targets. There is one annoyance related to that, and that is that `download-ci-llvm` now applies to all targets for which you try to build LLVM (d'uh), but that also means that if (for whatever reason) LLVM fails to be downloaded from CI, the build will fail. So if you build for target T2 from target T1: - If you want to download T1, but build T2, that's not possible to express. - If T2 fails to be downloaded, the build fails, even if it could be built locally. I think that we mostly have four options how to deal with this: 1. Just ignore it and wait to see if someone complains. 2. Revert the change and always download only for the host target. Worked so far. However, downloading LLVM for non-host targets would be quite useful for further bootstrap improvements and refactorings, because the current logic around sysroots and libdirs is.. convoluted, to say the last, and making cross-compilation easier would help with that a lot. 3. Allow specifying `download-ci-llvm` *per target* in the target config section. So that you can say that you want to download for T1, but build for T2. 4. Make download failures non-fatal, and cause them to trigger a local build. This would also help with removing the hacky `is_ci_llvm_available_for_target` logic, which hard-codes a bunch of targets to "know" which ones offer LLVM and which don't. We could just try to download, and if the result is 404, then we print a warning and continue with building (but this is slightly orthogonal, we can do this even if we don't make LLVM build failures non-fatal). I think that 3 or 4 would be the best solution, perhaps slightly opting for 4. If we get a 404, there's no way we can download, so we build instead. If we get a different error, we still make the failed download fail the build. And only if someone has a use-case for 3, we'd add the new config. Already before this PR, we did this: ``` // If download-ci-llvm=true we also want to check that CI llvm is available b && llvm::is_ci_llvm_available_for_target(&dwn_ctx.host_target, asserts) ``` so if LLVM wasn't available, we just silently reverted from `download-ci-llvm=true` to `download-ci-llvm=false`. The 4. proposal would just generalize that, to actually check whether the LLVM files are present on the CDN or not. The problem with 4. is that you can't really set any custom build options for LLVM though, because if you also enable `download-ci-llvm`, the config sanity check will tell you to GTFO :( So we would probably need to make some changes there. r? jieyouxu
View all comments
Continuation of #160916.
This PR removes the
llvm_outfunction, and makes it explicit when we need to build the host LLVM.Before, bootstrap used to just arbitrarily run a
llvm-configbinary, even though it might not have been executable on the given host. Now, if we need to execute it, the host LLVM will always be explicitly built. It is possible that there are some cases where this will build LLVM where it didn't before, but that should only happen if you were on a target A, built LLVM for target B, and by sheer luck A could execute code for B (where A != B).In any case, it is now explicit, so if we encounter such situation, we can fix it without depending on implicit assumptions (well, there are still thousands of other assumptions, but you get my point).
After this, I'll work on centralizing the sanity checking of downloading LLVM inside the
LlvmFromCistep, which will allow us to log the exact reason whydownload-ci-llvmmight not have been applied, and also enable downloading LLVM from CI for non-host targets.r? jieyouxu