Update cc-rs to 1.2.39 - #146186
Conversation
|
Failed to set assignee to
|
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_codegen_ssa |
|
@bors try jobs=aarch64-msvc-1,aarch64-apple,test-various,dist-apple-various |
This comment has been minimized.
This comment has been minimized.
Update cc-rs to 1.2.33 try-job: aarch64-msvc-1 try-job: aarch64-apple try-job: test-various try-job: dist-apple-various
|
Let me know @dpaoliello if you want to try with |
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
|
I switched to @rustbot ready |
This comment has been minimized.
This comment has been minimized.
|
The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging. |
|
@bors r=davidtwco,jieyouxu |
|
☀️ Test successful - checks-actions |
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 c9537a9 (parent) -> bc1d727 (this PR) Test differencesShow 2 test diffs2 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard bc1d7273dfbc6f8a11c0086fa35f6748a13e8d3c --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 (bc1d727): comparison URL. Overall result: ❌ regressions - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.1%, secondary -3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -1.9%, secondary 2.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 474.143s -> 475.525s (0.29%) |
|
... I did not expect that, feel free to post a revert if this isn't noise, won't have chance to investigate until EOD tmrw. |
|
@dpaoliello this PR wasn’t expected to impact compile-time performance this much, right? |
|
No, I wouldn't have thought so. I may have missed a change that would affect it. |
Revert "Auto merge of #146186 - dpaoliello:cc, r=jieyouxu"
|
I investigated the regression a bit, I don't think it's A hunch could be that Alternatively it might be |
|
We saw similar-ish wins (in the opposite direction) when -flto started being passed to jemalloc. Maybe that regressed now? |
Revert "Auto merge of #146186 - dpaoliello:cc, r=jieyouxu" This reverts #146186. This will mean regressing whichever `cc` fixes were needed for > fixes when compiling the Rust compiler for Arm64EC. It's not clear which `cc` change causes the across-the-board perf regression, I suspect it's a change in how certain compiler flags are handed. But I can immediately tell this is a rabbit hole to investigate, so let's revert for now to return-to-baseline and alleviate time pressure.
Revert "Auto merge of #146186 - dpaoliello:cc, r=jieyouxu" This reverts rust-lang/rust#146186. This will mean regressing whichever `cc` fixes were needed for > fixes when compiling the Rust compiler for Arm64EC. It's not clear which `cc` change causes the across-the-board perf regression, I suspect it's a change in how certain compiler flags are handed. But I can immediately tell this is a rabbit hole to investigate, so let's revert for now to return-to-baseline and alleviate time pressure.
chore: bump to cc@1.4.3 for rustc_llvm and library ### What is this? Old cc-rs derives this from the `-Clto` rustflag on its own. `cc@1.2.39` starts gating that behind `-Clinker-plugin-lto`, which bootstrap doesn't pass. Therefore, we need to pass this flag explicitly to keep LTO mode Previous efforts: * #146186 * #155438 I personally want this because of cc 1.3.0+ has the support of Cargo `-Ztrim-paths`, which helps what I am experimenting in <#161049>. ### How to review Commit by commit. To keep commits bisect-able, I added the first commit without cc bump. clang should be fine with duplicate `-flto` flags. One thing I am not certain is whether we should probe `-flto` flag in this case, or just make this fail if `-flto` isn't supported. (I assume `-flto` is quote widely supported) r? Kobzol --- 🤖 **LLM disclosure:** I used LLM for the experiment of <#161049>, but not the bootstrap LTO change in this PR.
chore: bump to cc@1.4.3 for rustc_llvm and library ### What is this? Old cc-rs derives this from the `-Clto` rustflag on its own. `cc@1.2.39` starts gating that behind `-Clinker-plugin-lto`, which bootstrap doesn't pass. Therefore, we need to pass this flag explicitly to keep LTO mode Previous efforts: * #146186 * #155438 I personally want this because of cc 1.3.0+ has the support of Cargo `-Ztrim-paths`, which helps what I am experimenting in <#161049>. ### How to review Commit by commit. To keep commits bisect-able, I added the first commit without cc bump. clang should be fine with duplicate `-flto` flags. One thing I am not certain is whether we should probe `-flto` flag in this case, or just make this fail if `-flto` isn't supported. (I assume `-flto` is quote widely supported) r? Kobzol --- 🤖 **LLM disclosure:** I used LLM for the experiment of <#161049>, but not the bootstrap LTO change in this PR.
chore: bump to cc@1.4.3 for rustc_llvm and library ### What is this? Old cc-rs derives this from the `-Clto` rustflag on its own. `cc@1.2.39` starts gating that behind `-Clinker-plugin-lto`, which bootstrap doesn't pass. Therefore, we need to pass this flag explicitly to keep LTO mode Previous efforts: * #146186 * #155438 I personally want this because of cc 1.3.0+ has the support of Cargo `-Ztrim-paths`, which helps what I am experimenting in <#161049>. ### How to review Commit by commit. To keep commits bisect-able, I added the first commit without cc bump. clang should be fine with duplicate `-flto` flags. One thing I am not certain is whether we should probe `-flto` flag in this case, or just make this fail if `-flto` isn't supported. (I assume `-flto` is quote widely supported) r? Kobzol --- 🤖 **LLM disclosure:** I used LLM for the experiment of <#161049>, but not the bootstrap LTO change in this PR.
chore: bump to cc@1.4.3 for rustc_llvm and library ### What is this? Old cc-rs derives this from the `-Clto` rustflag on its own. `cc@1.2.39` starts gating that behind `-Clinker-plugin-lto`, which bootstrap doesn't pass. Therefore, we need to pass this flag explicitly to keep LTO mode Previous efforts: * rust-lang/rust#146186 * rust-lang/rust#155438 I personally want this because of cc 1.3.0+ has the support of Cargo `-Ztrim-paths`, which helps what I am experimenting in <rust-lang/rust#161049>. ### How to review Commit by commit. To keep commits bisect-able, I added the first commit without cc bump. clang should be fine with duplicate `-flto` flags. One thing I am not certain is whether we should probe `-flto` flag in this case, or just make this fail if `-flto` isn't supported. (I assume `-flto` is quote widely supported) r? Kobzol --- 🤖 **LLM disclosure:** I used LLM for the experiment of <rust-lang/rust#161049>, but not the bootstrap LTO change in this PR.
For my purposes, contains fixes when compiling the Rust compiler for Arm64EC.
Checked the commits since 1.2.16, and I don't see anything else that may affect Rust?
find-msvc-toolswas also factored out fromccto allow updating the use inrustc_codegen_ssa(finding the linker when running the Rust compiler) to be separate from the use inrustc_llvm(building LLVM as part of the Rust compiler).