Skip to content

Reenable opt-level 3 - #41967

Merged
bors merged 1 commit into
rust-lang:masterfrom
ishitatsuyuki:patch-1
May 20, 2017
Merged

Reenable opt-level 3#41967
bors merged 1 commit into
rust-lang:masterfrom
ishitatsuyuki:patch-1

Conversation

@ishitatsuyuki

@ishitatsuyukiishitatsuyuki commented May 13, 2017

Copy link
Copy Markdown
Contributor

This comment is quite old, let's see what would happen with current MSVC.

Since there's no AppVeyor test for PR, the best way is to try if it get through homu. I don't recommend doing this in roll-up.

@ishitatsuyuki
Reenable opt-level 3
30383b2
This comment is quite old, let's see what would happen with current MSVC.
@rust-highfive

rust-highfive commented May 13, 2017

Copy link
Copy Markdown
Contributor

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov

petrochenkov commented May 13, 2017

Copy link
Copy Markdown
Contributor

Tested this locally, everything looks ok, lets's see what AppVeyor says.
@bors r+

@bors

bors commented May 13, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 30383b2 has been approved by petrochenkov

@ishitatsuyuki

ishitatsuyuki commented May 13, 2017

Copy link
Copy Markdown
ContributorAuthor

Uh, by the way this would probably fail due to a bug hidden by different optimization levels resulting in different metadata. (Fixed in #41639) Anyway, let's see how AppVeyor behaves, if we were lucky enough to not get it cancelled.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 13, 2017
@frewsxcv
Rollup merge ofrust-lang#41967- ishitatsuyuki:patch-1, r=petrochenkov
30a6e38
Reenable opt-level 3
This comment is quite old, let's see what would happen with current MSVC.
Since there's no AppVeyor test for PR, the best way is to try if it get through homu. I don't recommend doing this in roll-up.
@frewsxcvfrewsxcv mentioned this pull request May 13, 2017
Closed
bors added a commit that referenced this pull request May 13, 2017
@bors
Auto merge of#41972- frewsxcv:rollup, r=frewsxcv
2a03443
Rollup of 5 pull requests
- Successful merges: #41826, #41919, #41946, #41967, #41969
- Failed merges: #41939
@alexcrichton

alexcrichton commented May 13, 2017

Copy link
Copy Markdown
Member

@bors: r-

Oh actually with the findings in #41639 I think this is likely to cause regressions in the ecosystem for crates that, for example, do:

externcrate proc_macro;externcrate bitflags;

Could we hold off on landing this until after #41639 lands?

@Mark-SimulacrumMark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 14, 2017
@ishitatsuyuki

ishitatsuyuki commented May 18, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

r? @alexcrichton

@ishitatsuyuki
ishitatsuyuki deleted the patch-1 branch May 18, 2017 08:42
@alexcrichton

alexcrichton commented May 18, 2017

Copy link
Copy Markdown
Member

@ishitatsuyuki did you mean to close this?

@ishitatsuyuki
ishitatsuyuki restored the patch-1 branch May 18, 2017 22:21
@alexcrichton

alexcrichton commented May 19, 2017

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented May 19, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 30383b2 has been approved by alexcrichton

@Mark-SimulacrumMark-Simulacrum added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 20, 2017
@bors

bors commented May 20, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 30383b2 with merge 0bd9e1f...

bors added a commit that referenced this pull request May 20, 2017
@bors
Auto merge of#41967- ishitatsuyuki:patch-1, r=alexcrichton
0bd9e1f
Reenable opt-level 3
This comment is quite old, let's see what would happen with current MSVC.
Since there's no AppVeyor test for PR, the best way is to try if it get through homu. I don't recommend doing this in roll-up.
@bors

bors commented May 20, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0bd9e1f to master...

@bors
bors merged commit 30383b2 into rust-lang:masterMay 20, 2017
@ishitatsuyuki
ishitatsuyuki deleted the patch-1 branch May 20, 2017 05:28
@ishitatsuyuki

ishitatsuyuki commented May 20, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Heh, as what I expected, this is slightly slower probably due to CPU cache blowing. If we're going to lower the default optimize level, the discussion should be done separately, and the change should apply to all projects.

EDIT: I'm not sure if the slowdown is caused by simply running more LLVM passes. We should confirm this with the perf dashboard results.

@Mark-SimulacrumMark-Simulacrum mentioned this pull request May 20, 2017
Merged
bors added a commit that referenced this pull request May 20, 2017
@bors
Auto merge of#42123- Mark-Simulacrum:revert-41967, r=alexcrichton
1cda810
Revert "Reenable opt-level 3"
This reverts commit 30383b2, from #41967.
We believe that this is causing the failures when compiling rustc on 64 bit (which are probably segfaults).
cc @ishitatsuyuki
@kennytmkennytm mentioned this pull request Feb 22, 2018
Closed
@kennytmkennytm mentioned this pull request Jul 10, 2018
Merged
bors added a commit that referenced this pull request Jul 11, 2018
@bors
Auto merge of#52212- kennytm:o3-again, r=<try>
25ff830
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.
bors added a commit that referenced this pull request Jul 14, 2018
@bors
Auto merge of#52212- kennytm:o3-again, r=alexcrichton
1731f0a
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.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@brsonbrson

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ishitatsuyuki@rust-highfive@petrochenkov@bors@alexcrichton@brson@Mark-Simulacrum