Conversation
|
|
Thanks! @bors r+ |
|
📌 Commit c1d9423 has been approved by |
|
⌛ Testing commit c1d9423 with merge 3b49a39fd2d0e933e23d2a0a6e1d6e1baf56e27a... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
|
This assertion fails: At a guess, maybe those functions got merged and thus have the same address? Do Rust semantics require that distinct functions have distinct addresses? |
|
Change probably caused by llvm/llvm-project@38399ce, which removes the assumption that distinct unnamed_addr globals have different addresses (as they may be merged). |
There is no such requirement, no. This assertion is bogus. |
This fixed bug 47090, filed by @RalfJung. (cc #73722) I've added |
|
Making the functions different seems fine to me. @bors r+ |
|
📌 Commit 9756838 has been approved by |
Rollup of 10 pull requests Successful merges: - rust-lang#78901 (diagnostics: Note capturing closures can't be coerced to fns) - rust-lang#79588 (Provide more information for HRTB lifetime errors involving closures) - rust-lang#80232 (Remove redundant def_id lookups) - rust-lang#80662 (Added support for i386-unknown-linux-gnu and i486-unknown-linux-gnu) - rust-lang#80736 (use Once instead of Mutex to manage capture resolution) - rust-lang#80796 (Update to LLVM 11.0.1) - rust-lang#80859 (Fix --pretty=expanded with --remap-path-prefix) - rust-lang#80922 (Revert "Auto merge of rust-lang#76896 - spastorino:codegen-inline-fns2) - rust-lang#80924 (Fix rustdoc --test-builder argument parsing) - rust-lang#80935 (Rename `rustc_middle::lint::LevelSource` to `LevelAndSource`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Original message: Rollup merge of rust-lang#80796 - cuviper:llvm-11.0.1, r=nikic Update to LLVM 11.0.1 This updates to a new LLVM branch, rebased on the upstream `llvmorg-11.0.1`. All our patches applied cleanly except the fortanix unwind changes, which just needed a small adjustment in cmake files. r? `@nikic` Fixes rust-lang#73722
|
@cuviper It does seem that this PR was responsible for the perf regressions in the rollup where it was merged (#80960) as can be seen here in #81207. I'm guessing it's probably not worth actually addressing this, but we might want to get in the habit of doing perf runs when upgrading LLVM. Thoughts? |
|
Huh, I didn't expect this to happen for a patch version bump. Those are some pretty huge changes. The fact that the main impact is on check builds implies that rustc itself is being optimized worse, in a fairly significant way. |
|
More broadly, we should definitely not be rolling up LLVM bumps I think. |
|
Though possibly I'm misunderstanding the results. Is the link at #81207 meaningful, or should I be looking at https://perf.rust-lang.org/compare.html?start=058a71016553f267ae80b90276ef79956457d51a&end=3ab423f5ac44d394fb4d47db2b4042b4a25e8b1d&stat=instructions%3Au instead (which looks about neutral)? |
|
Yeah, it's surprising that a patch release had such impact. We can make a point of separating these from rollups though, sure. |
|
That said, I think the performance results are unlikely to be due to this PR - the rust-timer generated perf test PR seems to be buggy, as it includes more than just this PR's changes. I've filed rust-lang/rustc-perf#832 to track that. I think it is plausible there were some LLVM regressions, but the regex regression in the rollup's merge results are likely due to #80922 (Revert "Auto merge of #76896 - spastorino:codegen-inline-fns2) which specifically reverted debug-related codegen changes. |
This updates to a new LLVM branch, rebased on the upstream
llvmorg-11.0.1. All our patches applied cleanly except the fortanix unwind changes, which just needed a small adjustment in cmake files.r? @nikic
Fixes #73722