change opt-level 2 to 3 in bootstrap rustflags #48204
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
matthiaskrgr
force-pushed
the
RUSTFLAGS_O2_to_O3
branch
from
f64d722 to
07033a4
Compare
pietroalbini
added
the
S-waiting-on-review
|
You can just remove those lines and it will default to level 3. |
|
Also, opt-level=3 regressed runtime speed last time, so we need to measure it if the Windows issue is resolved too. The Windows crashes are spurious but frequent, so at least testing 3 times on AppVeyor is recommended. |
|
We're dealing with a lot of spurious failures right now so I'd personally prefer to not throw this into the mix when we're not sure if it'll actually work. This is of course fine to do in theory and will just have a lot of potential bugs we need to shake out. I'm going to be gone for the next week and don't have time to shepherd this, so... r? @kennytm |
|
IIRC the cause is unknown, and there's no tracking issue as well. |
|
@kennytm seems like this PR needs a review from you! |
|
Thanks for the PR @matthiaskrgr. The 2→3 change was previously attempted at #41967 (AppVeyor 1.0.3326) and reverted by #42123 (AppVeyor 1.0.3334). The segfaults on I've built this PR locally twice and no segfault happened in rustc nor the compile-fail test suite, so I think this is good to go. But before r+-ing, I'd like to ensure if increasing the opt-level is actually beneficial (has performance improvement of ≥1%). If not we may not want to risk the potential miscompilation. @bors try (cc @Mark-Simulacrum) |
|
⌛ Trying commit 07033a4 with merge bd789f974e73497bcb8183e75d270e30ca07fdc3... |
|
💥 Test timed out |
|
bors is still going rogue. Anyway, the try build is successful ( |
|
The performance improvement looks good (-1.1% on average). Now let's hope AppVeyor will catch the hidden segfault if any 😄 @bors r+ (Please do not rollup.) |
|
📌 Commit 07033a4 has been approved by |
bors
added
S-waiting-on-bors
…kennytm change opt-level 2 to 3 in bootstrap rustflags Since llvm got updated, maybe this also resolved unexpected crashes. See discussion here: rust-lang@409d40f Is it possible to give this a test-run? //cc @Mark-Simulacrum
|
@bors r- It seems to cause segfault on Windows again in the rollup #48526. Because it is a rollup it's unknown whether this PR is the real cause (I explicitly wrote (please do not rollup) 😒), but this PR being the cause is highly probable given the rest of the rollup looks quite irrelevant. Given this it seems -O3 is still very unsafe. Could we transform this PR to a documentation change by keeping the opt-level at 2, and point to the discussion here for an up-to-date rationale? |
bors
added
S-waiting-on-author
|
Hmm... |
|
As this is Windows-specific, it's likely not a rustc bug. Anyway, you should debug it by checking out this branch on Windows and try to reproduce. |
|
I don't have any windows computers here. :/ |
|
Not to be rude, but I recommend you to close this PR; without locating the root cause of segfault, we can't enable opt-level 3 on Windows. Period. |
|
Of course, I fully agree that this cannot be merged right now. |
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.
Since llvm got updated, maybe this also resolved unexpected crashes.
See discussion here: 409d40f
Is it possible to give this a test-run?
//cc @Mark-Simulacrum