Set opt-level to 3 #50329
Set opt-level to 3 #50329
Conversation
rust-highfive
added
the
S-waiting-on-review
|
@Zoxc Do you have information on whether the codegen issue has been fixed? Also, you can just remove the profile section to use the default |
|
previous discussion: #48204 |
|
@bors try |
|
⌛ Trying commit b83883b76f155f15e2a6e9de6ca53f46175d41c7 with merge c692cc25af00b8e958656116efc00c364dfee9a7... |
|
... try what? The point is Windows which needs AppVeyor. |
|
The try build is for a perf run. Locally opt-level=2 passes as many tests as opt-level=3 for me on Windows. I see that the Windows crashes were spurious, so I can try building it a few times. |
|
☀️ Test successful - status-travis |
|
Perf has been queued, but I doubt it'll change much since last time. |
|
In the other PR it's claimed that LLVM 6 landed in the meantime but the segfault on the previous PR happened on a commit tagged for our LLVM 6.0 branch. In that sense it's seems highly unlikely this is fixed? |
|
The wall-time results of the previous run is suspiciously good: http://perf.rust-lang.org/compare.html?start=b1f8e6fb06d7362eeb2065347a7db94e76b1cb2f&end=bd789f974e73497bcb8183e75d270e30ca07fdc3&stat=wall-time @alexcrichton We updated from a version close to 6 to the actual 6 release, and that fixed at least one bug causing segfaults for me. I've built this locally (with |
|
Ok if LLVM fixes landed in the meantime let's see how this pans out on CI @bors: r+ |
|
📌 Commit b83883b has been approved by |
bors
added
S-waiting-on-bors
|
@bors r=alexcrichton |
|
📌 Commit aad9840 has been approved by |
Set opt-level to 3 r? @alexcrichton
|
☀️ Test successful - status-appveyor, status-travis |
|
I feel like I've been seeing a lot of segfaults on Windows CI in the last few days which may be a result of this change. @kennytm do you think you've seen an uptick in Windows segfaults over the past few days? |
|
@alexcrichton This PR was merged at 2018-05-02T10:13:06Z, all spurious errors we've recorded since then are:
As far as the records go, there is no obvious uptick in Windows segfaults. |
|
Despite instruction counts being pretty green across the board this PR interestingly relatively severly regressed wall time in a number of benchmarks, mostly related to NLL |
This reverts commit aad9840. Level 3 (possibly indirectly, the underlying bug might be in XCode’s linker) causes unit tests to sefault when compiled with some versions of XCode: rust-lang#50867 It also appears to cause some segfaults on Windows: rust-lang#50329 (comment) … and regressions in some rustc performance benchmarks: rust-lang#50329 (comment)
Revert "Set opt-level to 3" This reverts commit aad9840. Level 3 (possibly indirectly, the underlying bug might be in XCode’s linker) causes unit tests to sefault when compiled with some versions of XCode: #50867 It also appears to cause some segfaults on Windows: #50329 (comment), and regressions in some rustc performance benchmarks: #50329 (comment)
Set opt-level = 3 the third time. This PR reverts #51165 (set -O2 to fix #50867), which reverted #50329 (set -O3), which was second attempt of #48204 (set -O3, closed due to Windows segfault that is fixed now), which reverted #42123 (set -O2 to fix spurious Windows segfaults), which reverted #41967 (set -O3). Last time we've found that setting -O3 regressed the wall time of NLL (#50329 (comment)), so we may need another perf run to confirm. I'd like to check this *after* the LLVM 7 upgrade #51966 has been merged, so marking this as <kbd>S-blocked</kbd> for now.
Set opt-level = 3 the third time. This PR reverts #51165 (set -O2 for fixing #50867), which reverted #50329 (set -O3), which was second attempt of #48204 (set -O3, closed due to Windows segfault that is fixed now), which reverted #42123 (set -O2 to fix spurious Windows segfaults), which reverted #41967 (set -O3). Since we have found the root cause of #50867, this optimization could be tried again. Last time we've found that setting -O3 regressed the wall time of NLL (#50329 (comment)), so we may need another perf run to confirm. I'd like to check this *after* the LLVM 7 upgrade #51966 has been merged, so marking this as <kbd>S-blocked</kbd> for now.
r? @alexcrichton