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

[mir-opt] Optimize calls to CopyNonOverlapping #83785

Closed
wants to merge 1 commit into from
Closed

[mir-opt] Optimize calls to CopyNonOverlapping #83785

wants to merge 1 commit into from

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Apr 2, 2021 •
edited
Loading

No description provided.

@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 2, 2021
@bors
Copy link
Contributor

bors commented Apr 2, 2021

⌛ Trying commit 98ee843a1bd2bd448f1302fa0c5e7c36555f750f with merge 00b94879e7876a43a12e5ded56e58d3d89406d74...

@bors
Copy link
Contributor

bors commented Apr 2, 2021

☀️ Try build successful - checks-actions
Build commit: 00b94879e7876a43a12e5ded56e58d3d89406d74 (00b94879e7876a43a12e5ded56e58d3d89406d74)

@rust-timer
Copy link
Collaborator

Queued 00b94879e7876a43a12e5ded56e58d3d89406d74 with parent a207871, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (00b94879e7876a43a12e5ded56e58d3d89406d74): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 2, 2021
@wesleywiser wesleywiser changed the title WIP - Optimize calls to CopyNonOverlapping [mir-opt] Optimize calls to CopyNonOverlapping Apr 8, 2021
@wesleywiser wesleywiser mentioned this pull request Apr 8, 2021
Closed
@wesleywiser
Copy link
Member Author

r? @oli-obk

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

RalfJung commented Apr 8, 2021

Oli is on reduced availability currently; can anyone else from the WG take over review?

@oli-obk oli-obk added the A-mir-opt Area: MIR optimizations label Apr 22, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 29, 2021
@bors
Copy link
Contributor

bors commented Apr 29, 2021

⌛ Trying commit b645894 with merge d47385f6d489305d533e77d71f332d73b96c22d4...

@bors
Copy link
Contributor

bors commented Apr 29, 2021

☀️ Try build successful - checks-actions
Build commit: d47385f6d489305d533e77d71f332d73b96c22d4 (d47385f6d489305d533e77d71f332d73b96c22d4)

@rust-timer
Copy link
Collaborator

Queued d47385f6d489305d533e77d71f332d73b96c22d4 with parent 78c9639, future comparison URL.

@JohnCSimon JohnCSimon added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 12, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2021
@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@jackh726
Copy link
Member

I am not sure if this should be merged as is. On one hand this improves codegen for cg_clif. On the other hand it regresses codegen for cg_llvm in debug mode in some cases.

c.c. @rust-lang/compiler for thoughts

@oli-obk
Copy link
Contributor

oli-obk commented Aug 19, 2021

I think we should merge it. perf regressions are <1% and likely just due arbitrary threshold changes in llvm opts.

@RalfJung
Copy link
Member

We should probably re-run the benchmarks anyway... @wesleywiser can you rebase and resolve the conflicts?

@jackh726 jackh726 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 12, 2021
@JohnCSimon
Copy link
Member

Triage: @wesleywiser

RalfJung commented 23 days ago
We should probably re-run the benchmarks anyway... @wesleywiser can you rebase and resolve the conflicts?

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 27, 2021
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021
bjorn3
bjorn3 reviewed Oct 20, 2021
View reviewed changes
compiler/rustc_mir/src/transform/lower_intrinsics.rs
Comment on lines +63 to +66
StatementKind::Assign(box (
tcx.mk_place_deref(dst),
Rvalue::Use(Operand::Copy(tcx.mk_place_deref(src))),
))
Copy link
Member

@bjorn3 bjorn3 Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an interesting conversation on zulip around the semantics of copy_nonoverlapping. If it does an "untyped copy", lowering it to a regular assignment is not a sound optimization, so this probably needs to wait on a decision if copy_nonoverlapping does a typed or untyped copy. https://rust-lang.zulipchat.com/#narrow/stream/136281-t-lang.2Fwg-unsafe-code-guidelines/topic/copy_nonoverlapping.20and.20provenance/near/258410065

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for digging this up, I knew this proposal was living somewhere... (and I think I was advocating for it, not realizing what equating copy_nonoverlapping and assignment would entail).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If copy_nonoverlapping does an untyped copy does that mean that the implementation of ptr::read needs to change so that it does a typed read?

Copy link
Member

@RalfJung RalfJung Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is correct to replace a typed by an untyped copy, just not the other way around.

So, read purporting to be typed but actually doing an untyped operation under the hood is fine. (Typedness is force on it anyway because the result is returned by-value at type T.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but that may pessimize the read. See #73258

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copy_nonoverlapping does an untyped copy as per #97712, so this lowering is off the table now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we close it then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be made an assignment at type MaybeUninit<T>.

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2021
@mati865
Copy link
Contributor

mati865 commented Jan 9, 2022

I have rebased this PR locally but added test fails in a way suggesting this pass is not being ran. Could somebody instruct me how to debug it?

@oli-obk
Copy link
Contributor

oli-obk commented Jan 12, 2022

I have rebased this PR locally but added test fails in a way suggesting this pass is not being ran. Could somebody instruct me how to debug it?

the pass definitely runs, it is an unconditional pass.

Beyond adding tracing statements and activating those while running the failing tests, I don't know how to debug this.

@mati865
Copy link
Contributor

mati865 commented Jan 13, 2022

I have badly chosen words, I meant the optimisation exit somewhere early.
I have debugged it to resolve_rust_intrinsic returning None for relevant test lines. I'll take deeper look when I have a bit more time by the end of the week.

@mati865
Copy link
Contributor

mati865 commented Jan 14, 2022

Turns out #86003 turned copy_nonoverlapping back to normal functions and #86699 is required to make it intrinsic again.

@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2022
@Dylan-DPC
Copy link
Member

@wesleywiser any updates on this?

@mati865 mati865 mentioned this pull request May 29, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@oli-obk oli-obk oli-obk left review comments

@jrmuizel jrmuizel jrmuizel left review comments

@mati865 mati865 mati865 left review comments

@bjorn3 bjorn3 bjorn3 left review comments

@tmiasko tmiasko tmiasko left review comments

Assignees

@bjorn3 bjorn3

Labels
A-mir-opt Area: MIR optimizations S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet