Skip to content

Mark more locals as moved to avoid building drops for them.#158281

Open
cjgillot wants to merge 4 commits into
rust-lang:mainfrom
cjgillot:drop-moved-locals
Open

Mark more locals as moved to avoid building drops for them.#158281
cjgillot wants to merge 4 commits into
rust-lang:mainfrom
cjgillot:drop-moved-locals

Conversation

@cjgillot

@cjgillot cjgillot commented Jun 23, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

View all comments

MIR building skips generating drops for moved-from locals in the topmost scope. This was only used for call terminators, but can be generalized to many other moves. This PR generalizes this to aggregate construction and many other assignments.

This avoids generating drops that would then be removed by drop elaboration.

This PR changes borrowck behaviour: some programs that were rejected are now accepted, see the last commit. That particular case was wrongly rejected.

Fixes #156713

Based on #158279 to remove a lint false-positive

@rustbot

rustbot commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 23, 2026
@rustbot

rustbot commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 73 candidates
  • Random selection from 21 candidates

@theemathas

theemathas commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Does this have user-visible behavior changes? e.g., see #156713

@cjgillot

cjgillot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Yes. With this change, both cases in #156713 pass. We also have a few user-visible consequences with the drop order lint.

@theemathas theemathas added T-lang Relevant to the language team needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 23, 2026
@JohnTitor

JohnTitor commented Jun 27, 2026

Copy link
Copy Markdown
Member

@rustbot reroll

@rustbot rustbot assigned jackh726 and unassigned JohnTitor Jun 27, 2026
@cjgillot cjgillot force-pushed the drop-moved-locals branch from 51861b3 to d128dba Compare July 1, 2026 23:37
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot cjgillot 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 Jul 2, 2026
@cjgillot cjgillot force-pushed the drop-moved-locals branch from a5dd21c to 4ff8c26 Compare July 5, 2026 03:19
@cjgillot cjgillot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2026
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the drop-moved-locals branch from 4ff8c26 to 90e4893 Compare July 5, 2026 11:28
@rust-bors

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the drop-moved-locals branch from 90e4893 to a851b9e Compare July 8, 2026 23:26
@rustbot

This comment has been minimized.

Sign in to view
@cjgillot

cjgillot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 9, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Jul 9, 2026
@bors
Auto merge of #158281 - cjgillot:drop-moved-locals, r=<try>
9ef0dc4 
Mark more locals as moved to avoid building drops for them.
@theemathas

theemathas commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Since this PR has insta-stable changes, I think this will need to go through an FCP. Could you describe what the user-facing changes are, and lang-nominate the PR?

@rust-bors

rust-bors Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 9ef0dc4 (9ef0dc4705f7d34e7ff65cb13ffa27f4263fedbf)
Base parent: 14cae68 (14cae681329a63c622a6e1fbe1d30f9374bc51d8)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (9ef0dc4): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 1.0% [0.2%, 16.0%] 150
Regressions ❌
(secondary)		 33.2% [0.1%, 740.0%] 177
Improvements ✅
(primary)		 -0.4% [-0.6%, -0.2%] 11
Improvements ✅
(secondary)		 -0.4% [-2.0%, -0.1%] 16
All ❌✅ (primary) 0.9% [-0.6%, 16.0%] 161

Max RSS (memory usage)

Results (primary -1.2%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.7% [3.7%, 3.7%] 1
Improvements ✅
(primary)		 -1.2% [-2.7%, -0.4%] 4
Improvements ✅
(secondary)		 -0.9% [-1.0%, -0.8%] 2
All ❌✅ (primary) -1.2% [-2.7%, -0.4%] 4

Cycles

Results (primary 4.5%, secondary 75.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 4.5% [2.0%, 7.2%] 9
Regressions ❌
(secondary)		 90.1% [0.8%, 234.3%] 22
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.4% [-9.4%, -2.3%] 4
All ❌✅ (primary) 4.5% [2.0%, 7.2%] 9

Binary size

Results (primary 0.0%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 0.0% [0.0%, 0.1%] 38
Regressions ❌
(secondary)		 0.0% [0.0%, 0.1%] 23
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 0.0% [0.0%, 0.1%] 38

Bootstrap: 488.761s -> 487.76s (-0.20%)
Artifact size: 388.78 MiB -> 385.17 MiB (-0.93%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 9, 2026
cjgillot added 4 commits July 11, 2026 18:19
@cjgillot
Add test.
a992b59
@cjgillot
Mark locals used to construct Aggregates as moved.
865273d 
And some more cases along the way.

We have a specific optimization to avoid generating useless drops, use it.
In particular, aggregate construction are very similar to function calls
for which this is designed.
@cjgillot
Add test.
5d95e6e
@cjgillot
Use a GrowableBitSet for moved_locals.
650bed1
@cjgillot cjgillot force-pushed the drop-moved-locals branch from a851b9e to 650bed1 Compare July 11, 2026 18:43
@rustbot

rustbot commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@cjgillot

cjgillot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 11, 2026
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors Bot pushed a commit that referenced this pull request Jul 11, 2026
@bors
Auto merge of #158281 - cjgillot:drop-moved-locals, r=<try>
68a2829 
Mark more locals as moved to avoid building drops for them.
@rust-bors

rust-bors Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: 68a2829 (68a28293b11549460b78874559bdb450ef9105e7)
Base parent: acf6a00 (acf6a00aff5d8aa8892aadff0b48364b50f97cf4)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer

rust-timer commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (68a2829): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.2% [0.0%, 0.4%] 5
Improvements ✅
(primary)		 -0.4% [-0.9%, -0.2%] 20
Improvements ✅
(secondary)		 -0.5% [-0.8%, -0.0%] 16
All ❌✅ (primary) -0.4% [-0.9%, -0.2%] 20

Max RSS (memory usage)

Results (primary 1.7%, secondary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 4.4% [4.4%, 4.4%] 1
Regressions ❌
(secondary)		 3.1% [0.6%, 4.7%] 3
Improvements ✅
(primary)		 -0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)		 -0.9% [-0.9%, -0.9%] 1
All ❌✅ (primary) 1.7% [-0.9%, 4.4%] 2

Cycles

Results (secondary 6.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 6.5% [2.8%, 15.0%] 8
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.809s -> 489.874s (0.01%)
Artifact size: 389.31 MiB -> 389.84 MiB (0.14%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 11, 2026
@rust-bors

rust-bors Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jackh726 jackh726

Labels

needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. perf-regression Performance regression. 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. T-lang Relevant to the language team

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inconsistent and nonsensical borrow-checking error from unwind path of destructor of moved-out variable

7 participants

@cjgillot @rustbot @theemathas @JohnTitor @rust-log-analyzer @rust-timer @jackh726