Update to LLVM 10 #67759
Update to LLVM 10 #67759
Conversation
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
First optimization regression, eventually leading to failure to eliminate bounds checks: https://bugs.llvm.org/show_bug.cgi?id=44419 |
|
Second optimization regression: https://bugs.llvm.org/show_bug.cgi?id=44423 |
|
While previously was rotated to we now get which does not get rotated, and thus not simplified subsequently. The reason why we get a switch here (created by SimplifyCFG) is that CVP eliminated a masking operation, namely instruction I'm not yet sure how to address this issue. |
|
Actually, CVP might be both the problem and the solution here ... if we relax https://github.com/llvm/llvm-project/blob/a58da1a2ff039dd3bb4c43db3919995cf4a74cc7/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp#L315 to allow phis, then condition |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit d6773bf with merge d5ca0b366ee2e90c658ca0334740b4907ac1ce69... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-azure |
bors
added
the
S-waiting-on-author
|
@bors try |
[WIP] Update to LLVM 10 LLVM 10 is going to be branched soon, so it's a good time to start finding all those tasty new miscompiles and performance regressions ;) r? @ghost
|
☀️ Try build successful - checks-azure |
|
Queued 97588ae with parent c5840f9, future comparison URL. |
|
Finished benchmarking try commit 97588ae, comparison URL. |
|
Those are some pretty hefty compile-time regressions. Some local numbers for the largest diffs in LLVM timings on clap-rs (via this script): Haven't tested how stable these numbers are, but we clearly have a big regression in "Global Variable Optimizer" (first time I hear about this pass), some regression in X86 ISel (this tends to get slower with each release, but this seems an unusually large jump) and we now have a lot more MemorySSA runs. Previously there was just one, now there are 5 (!!). Not sure if this is an analysis preservation fail or supposed to be that way. |
|
Looks like the five MemorySSA runs are expected, as MemorySSA is now enabled for loop passes: https://reviews.llvm.org/D58311 The GlobalOpt regression is likely https://reviews.llvm.org/D68298 and hopefully easy to address with a revert. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 6011237 with merge 1b6633b46cd7705ec813cfd1cd7083e92f9da903... |
|
☀️ Try build successful - checks-azure |
|
Queued 1b6633b46cd7705ec813cfd1cd7083e92f9da903 with parent 79cf5e4, future comparison URL. |
|
Finished benchmarking try commit 1b6633b46cd7705ec813cfd1cd7083e92f9da903, comparison URL. |
|
Okay, that improved things a bit, reclaiming about 5% in the best case. Still pretty bad compile-time regression overall. |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
Sorry, I'm not clear on this, but do we have perf results from this change? It seems like we ought to check back and discuss, perhaps at the next compiler team triage meeting. A quick glance at perf.rust-lang.org didn't show anything yet, but it may not have taken into account the effects of this PR. cc @rust-lang/wg-prioritization -- can we add this to our next agenda to check back in? |
|
Personally, I would be happy to spend 10% longer compiling at |
|
Final perf results from the landing are here. Lots of red. Interestingly, |
|
The net results compared to 45d050c ( |
|
The
Is that comparing the nightly against the most recent beta? It won't look so dire because we've made a bunch of speedups recently, particularly for debug builds. If you filter down to And compare it against this, which is the comparison of that beta against the revision just before the LLVM 10 upgrade. Again, filter down to Or just look at the individual |
|
I don't doubt at all that this PR slowed things down. I was trying to see what the overall story from 1.44 to 1.45 will look like, in spite of this. |
@nikomatsakis: Any update here? The current dev cycle is almost over and the upgrade to LLVM 10 is going to cause a sizeable perf regression without any upside that I'm aware of. |
|
There's a maintenance upside in bugs that may be fixed in LLVM 10, without Rust having to encounter them and try to backport fixes. For instance, I just got done dealing with an LLVM 9 miscompilation in s390x, rhbz1837660, which was thankfully already fixed in LLVM 10. Of course there's a risk of LLVM regressions too, even apart from performance, but let's hope for forward progress. |
|
Personally I think our stance in general should be to accept slower opt builds, especially if it results in better code generation (anything else is swimming against the tide for LLVM). For major regressions and regressions to debug builds, we ideally should report to upstream and work with them to identify the cause. For this change specifically, most regressions are for opt builds, but there are some for debug builds. But sticking to a >6mo old version of LLVM is going to cause a lot more pain than it would solve at this point, IMO. Conversely, rolling to new versions of LLVM more frequently makes it far easier to catch and identify regressions like this. |
See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
Rename "cyclone" to "apple-a7" per changes in upstream LLVM It looks like they intended to keep "cyclone" as a legacy option, but removed it from the list of subtarget features. This created a flood of warnings when targeting aarch64-apple-ios, and probably also created incorrectly optimized artifacts. See: https://reviews.llvm.org/D70779 https://reviews.llvm.org/D70779#C1703593NL568 LLVM 10 merged into master at: rust-lang#67759
LLVM 10 is going to be branched soon, so it's a good time to start finding all those tasty new miscompiles and performance regressions ;)
Status:
fhahn proposed https://reviews.llvm.org/D72214.fhahn has reverted the patch.