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.

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

Turn copy into moves during DSE. #113758

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Turn copy into moves during DSE. #113758

merged 3 commits into from
Jul 20, 2023

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jul 16, 2023

Dead store elimination computes whether removing a direct store to an unborrowed place is allowed.
Where removing a store is allowed, writing uninit is too.

This means that we can use this pass to transform copy operands into move operands. This is only interesting in call terminators, so we only handle those.

Special care is taken for the use_both(_1, _1) case:

  • moving the second argument is ok, as _1 is not live after the call;
  • moving the first argument is not, as the second argument reads _1.

Fixes #75993
Fixes #108068

r? @RalfJung
cc @JakobDegen

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 16, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@RalfJung
Copy link
Member

Sorry, I can't review MIR transforms. I can help judge their soundness given a high-level description of what they do with representative examples, but reverse engineering that from a rustc implementation of a MIR transform is a lot of work that I don't have the time for.

r? mir-opt

@rustbot rustbot assigned oli-obk and unassigned RalfJung Jul 16, 2023
bb0: {
StorageLive(_2);
- _2 = use_both(_1, _1) -> [return: bb1, unwind continue];
+ _2 = use_both(_1, move _1) -> [return: bb1, unwind continue];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RalfJung IMO, the thorny question is here. It this transformation ok?

Copy link
Member

Choose a reason for hiding this comment

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

That's a very good question. It is okay under the move semantics I implemented in Miri recently (assuming _1 is not read from again later), because there we do left-to-right evaluation so the first _1 is copied before the move action. But I don't know if there's a risk of codegen backends doing things that would cause problems here. Also move semantics are far from clear; I don't think we already have the semantics we want here -- we just have something that justifies what codegen does.

I think, similar to how we avoid making assumptions about the aliasing model, we should be conservative and avoid this if any of the other arguments (or the return value) could alias the newly moved argument.

Cc @bjorn3

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine given that the first _1 is copied before the call passes the address of the second argument. It may even be ok to do use_both(move _1, _1) with current backends, but I don't think whatever semantics we give to move arguments should allow that as semantically the move of the first arg happens before the copy of the second arg.

@cjgillot cjgillot added the A-mir-opt Area: MIR optimizations label Jul 16, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jul 17, 2023

@bors r=JakobDegen,oli-obk

@bors
Copy link
Contributor

bors commented Jul 17, 2023

📌 Commit 3af8610f5a91c003a7fdb5df5353774c8ca0431a has been approved by JakobDegen,oli-obk

It is now in the queue for this repository.

@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 Jul 17, 2023
@cjgillot
Copy link
Contributor Author

Just in case there is a perf effect (I doubt it)
@bors rollup=never

@bors
Copy link
Contributor

bors commented Jul 17, 2023

⌛ Testing commit 3af8610f5a91c003a7fdb5df5353774c8ca0431a with merge 32322a067c781aef41fc5ea61450820258d956c4...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 18, 2023

💔 Test failed - checks-actions

@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 Jul 18, 2023
@cjgillot
Copy link
Contributor Author

Changed the codegen test to run with mir opts.
@bors r=JakobDegen,oli-obk

@bors
Copy link
Contributor

bors commented Jul 19, 2023

📌 Commit 6d9c12730114034367fa73c394823285515c508e has been approved by JakobDegen,oli-obk

It is now in the queue for this repository.

@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 Jul 19, 2023
@bors
Copy link
Contributor

bors commented Jul 19, 2023

⌛ Testing commit 6d9c12730114034367fa73c394823285515c508e with merge a751a70cc37827cad61f1c2b5558990838050cc0...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 19, 2023

💔 Test failed - checks-actions

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jul 19, 2023
@bors
Copy link
Contributor

bors commented Jul 20, 2023

📌 Commit 254bf60 has been approved by JakobDegen,oli-obk

It is now in the queue for this repository.

@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 Jul 20, 2023
@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit 254bf60 with merge 002a2388e0bd2a9ed50f460984b0c3523bd1ff71...

@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Jul 20, 2023

💔 Test failed - checks-actions

@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 Jul 20, 2023
@lqd
Copy link
Member

lqd commented Jul 20, 2023

@bors retry rust-lang/crates.io#6850

@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 Jul 20, 2023
@bors
Copy link
Contributor

bors commented Jul 20, 2023

⌛ Testing commit 254bf60 with merge 06a53dd...

@bors
Copy link
Contributor

bors commented Jul 20, 2023

☀️ Test successful - checks-actions
Approved by: JakobDegen,oli-obk
Pushing 06a53dd to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (06a53dd): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
9.4% [9.4%, 9.4%] 1
Regressions ❌
(secondary)
0.6% [0.6%, 0.6%] 1
Improvements ✅
(primary)
-1.0% [-2.1%, -0.2%] 14
Improvements ✅
(secondary)
-0.8% [-1.3%, -0.2%] 2
All ❌✅ (primary) -0.3% [-2.1%, 9.4%] 15

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.9% [1.3%, 2.2%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-6.3% [-10.5%, -3.8%] 6
Improvements ✅
(secondary)
-1.9% [-2.7%, -1.1%] 2
All ❌✅ (primary) -1.9% [-10.5%, 2.2%] 13

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
8.8% [8.8%, 8.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.8% [-0.8%, -0.8%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.0% [-0.8%, 8.8%] 2

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 0.2%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.2% [-0.5%, -0.0%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.5%, 0.2%] 12

Bootstrap: 648.618s -> 649.387s (0.12%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 20, 2023
@cjgillot cjgillot deleted the move-dse branch July 20, 2023 21:26
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 2023
…-errors

Do not convert copies of packed projections to moves.

This code path was introduced in rust-lang#113758

After seeing https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Packed.20fields.20and.20in-place.20function.20argument.2Freturn.20passing, this may be UB, so should be disallowed.

This should not appear in normally-built MIR, which introduces temporary copies for packed projections.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Aug 26, 2023
Do not convert copies of packed projections to moves.

This code path was introduced in rust-lang/rust#113758

After seeing https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Packed.20fields.20and.20in-place.20function.20argument.2Freturn.20passing, this may be UB, so should be disallowed.

This should not appear in normally-built MIR, which introduces temporary copies for packed projections.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
Do not convert copies of packed projections to moves.

This code path was introduced in rust-lang/rust#113758

After seeing https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Packed.20fields.20and.20in-place.20function.20argument.2Freturn.20passing, this may be UB, so should be disallowed.

This should not appear in normally-built MIR, which introduces temporary copies for packed projections.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Do not convert copies of packed projections to moves.

This code path was introduced in rust-lang/rust#113758

After seeing https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/Packed.20fields.20and.20in-place.20function.20argument.2Freturn.20passing, this may be UB, so should be disallowed.

This should not appear in normally-built MIR, which introduces temporary copies for packed projections.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants