Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emit noalias on &mut parameters by default #50744

Merged
merged 1 commit into from
May 19, 2018
Merged

Emit noalias on &mut parameters by default #50744

merged 1 commit into from
May 19, 2018

Conversation

nikic
Copy link
Contributor

@nikic nikic commented May 14, 2018

This used to be disabled due to LLVM bugs in the handling of
noalias information in conjunction with unwinding. However,
according to #31681 all known LLVM bugs have been fixed by
LLVM 6.0, so it's probably time to reenable this optimization.

-Z no-mutable-noalias is left as an escape-hatch to debug problems
suspected to stem from this change.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2018
@nikic nikic force-pushed the mutable-noalias branch from d34842d to 8b4ee21 Compare May 14, 2018 14:11
@eddyb
Copy link
Member

eddyb commented May 14, 2018

Shouldn't this be gated on the LLVM version? I think we still support distros using 3.9.
cc @alexcrichton @gankro

@nikic
Copy link
Contributor Author

nikic commented May 14, 2018

@eddyb Good point, I've conditioned the default behavior on LLVM >= 6.0 now.

@alexcrichton
Copy link
Member

@bors: r+

Nice!

@bors
Copy link
Contributor

bors commented May 14, 2018

📌 Commit 0c965a6 has been approved by alexcrichton

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2018
@steveklabnik
Copy link
Member

It'd be very cool to see any before/after benchmarks for this. I'm really curious as to how much of an impact it actually has.

@Gankra
Copy link
Contributor

Gankra commented May 14, 2018

This is removing the panic=abort stable opt in. Is firefox on a new enough llvm that it won't regress?

@nikic
Copy link
Contributor Author

nikic commented May 14, 2018

@gankro I've added back the panic=abort check just in case. Apparently Firefox is stuck on old Rust due to LLVM 6 regression in #49873.

@Gankra
Copy link
Contributor

Gankra commented May 14, 2018

Thanks!

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 14, 2018
@bors
Copy link
Contributor

bors commented May 14, 2018

📌 Commit 8acc992 has been approved by alexcrichton

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 14, 2018
@bors
Copy link
Contributor

bors commented May 14, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented May 14, 2018

📌 Commit 8acc992 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 17, 2018

☔ The latest upstream changes (presumably #50807) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 17, 2018
@nikic nikic force-pushed the mutable-noalias branch from 8acc992 to dbf2108 Compare May 17, 2018 08:37
@bors
Copy link
Contributor

bors commented May 17, 2018

☔ The latest upstream changes (presumably #50615) made this pull request unmergeable. Please resolve the merge conflicts.

@nikic
Emit noalias on &mut parameters by default
1230813 
This used to be disabled due to LLVM bugs in the handling of
noalias information in conjunction with unwinding. However,
according to rust-lang#31681 all known LLVM bugs have been fixed by
LLVM 6.0, so it's probably time to reenable this optimization.

Noalias annotations will not be emitted by default if either
-C panic=abort (as previously) or LLVM >= 6.0 (new).

-Z mutable-noalias=no is left as an escape-hatch to allow
debugging problems suspected to stem from this change.
@nikic nikic force-pushed the mutable-noalias branch from dbf2108 to 1230813 Compare May 17, 2018 20:28
@michaelwoerister
Copy link
Member

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented May 18, 2018

📌 Commit 1230813 has been approved by alexcrichton

@bors bors 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 18, 2018
@bors
Copy link
Contributor

bors commented May 19, 2018

⌛ Testing commit 1230813 with merge d4cb245d65e63ab49eef3db792070fafd6c7d048...

@bors
Copy link
Contributor

bors commented May 19, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 19, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
@kennytm kennytm mentioned this pull request May 19, 2018
Closed
@rust-lang rust-lang deleted a comment from rust-highfive May 19, 2018
@kennytm
Copy link
Member

kennytm commented May 19, 2018

@bors retry #50887

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2018
@bors
Copy link
Contributor

bors commented May 19, 2018

⌛ Testing commit 1230813 with merge bdace29...

bors added a commit that referenced this pull request May 19, 2018
@bors
Auto merge of #50744 - nikic:mutable-noalias, r=alexcrichton
bdace29 
Emit noalias on &mut parameters by default

This used to be disabled due to LLVM bugs in the handling of
noalias information in conjunction with unwinding. However,
according to #31681 all known LLVM bugs have been fixed by
LLVM 6.0, so it's probably time to reenable this optimization.

-Z no-mutable-noalias is left as an escape-hatch to debug problems
suspected to stem from this change.
@bors
Copy link
Contributor

bors commented May 19, 2018

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

@bors bors merged commit 1230813 into rust-lang:master May 19, 2018
@leonardo-m
Copy link

Is this changing the performance of some standard benchmark?

@scottmcm
Copy link
Member

http://perf.rust-lang.org/compare.html?start=8319ef5b78a10b3a8de4109bb8b0e6d23fbe4de1&end=bdace29de04af4fe9e4317b73c3f7d6418a33de1&stat=instructions%3Au

Looks like the Rust part of the compiler is faster, but the LLVM part spends more time optimizing. So it seems promising for runtime perf; hopefully lolbench will confirm that.

@nikic nikic mentioned this pull request May 21, 2018
Closed
@daboross daboross mentioned this pull request Jun 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: 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.