Convert moves of references to copies in ReferencePropagation #142185

saethlin · 2025-06-08T03:16:12Z

This is a fix for #141101.

The root cause of this miscompile is that the SsaLocals analysis that MIR transforms use is supposed to detect locals that are only written to once, in their single assignment. But that analysis is subtly wrong; it does not consider Operand::Move to be a write even though the meaning ascribed to Operand::Move (at least as a function parameter) by Miri is that the callee may have done arbitrary writes to the caller's Local that the Operand wraps (because Move is pass-by-pointer). So Miri conwiders Operand::Move to be a write but both the MIR visitor system considers it a read, and so does SsaLocals.

I have tried fixing this by changing the PlaceContext that is ascribed to an Operand::Move to a MutatingUseContext but that seems to have borrow checker implications, and changing SsaLocals seems to have wide-ranging regressions in MIR optimizations.

So instead of doing those, this PR adds a new kludge to ReferencePropagation, which follows the same line of thinking as the kludge in CopyProp that solves this same problem inside that pass:

rust/compiler/rustc_mir_transform/src/copy_prop.rs

Lines 65 to 98 in a5584a8

    
           /// `SsaLocals` computed equivalence classes between locals considering copy/move assignments. 
        
           /// 
        
           /// This function also returns whether all the `move?` in the pattern are `move` and not copies. 
        
           /// A local which is in the bitset can be replaced by `move _a`. Otherwise, it must be 
        
           /// replaced by `copy _a`, as we cannot move multiple times from `_a`. 
        
           /// 
        
           /// If an operand copies `_c`, it must happen before the assignment `_d = _c`, otherwise it is UB. 
        
           /// This means that replacing it by a copy of `_a` if ok, since this copy happens before `_c` is 
        
           /// moved, and therefore that `_d` is moved. 
        
           #[instrument(level = "trace", skip(ssa, body))] 
        
           fn fully_moved_locals(ssa: &SsaLocals, body: &Body<'_>) -> DenseBitSet<Local> { 
        
               let mut fully_moved = DenseBitSet::new_filled(body.local_decls.len()); 
        
               for (_, rvalue, _) in ssa.assignments(body) { 
        
                   let (Rvalue::Use(Operand::Copy(place) | Operand::Move(place)) 
        
                   | Rvalue::CopyForDeref(place)) = rvalue 
        
                   else { 
        
                       continue; 
        
                   }; 
        
                   let Some(rhs) = place.as_local() else { continue }; 
        
                   if !ssa.is_ssa(rhs) { 
        
                       continue; 
        
                   } 
        
                   if let Rvalue::Use(Operand::Copy(_)) | Rvalue::CopyForDeref(_) = rvalue { 
        
                       fully_moved.remove(rhs); 
        
                   } 
        
               } 
        
               ssa.meet_copy_equivalence(&mut fully_moved); 
        
               fully_moved 
        
           }

saethlin · 2025-06-08T03:16:27Z

@bors try @rust-timer queue

bors · 2025-06-08T03:17:37Z

⌛ Trying commit e41bd6c with merge 920f4b7...

Convert moves of references to copies in ReferencePropagation This is a fix for #141101. The root cause of this miscompile is that the SsaLocals analysis that MIR transforms use is supposed to detect locals that are only written to once, in their single assignment. But that analysis is subtly wrong; it does not consider `Operand::Move` to be a write even though the meaning ascribed to `Operand::Move` (at least as a function parameter) by Miri is that the callee may have done arbitrary writes to the operand in the caller (because `Move` is pass-by-pointer). So Miri conwiders `Operand::Move` to be a write but both the MIR visitor system considers it a read, and so does SsaLocals. I have tried fixing this by changing the `PlaceContext` that is ascribed to an `Operand::Move` to a `MutatingUseContext` but that seems to have borrow checker implications, and changing SsaLocals seems to have wide-ranging regressions in MIR optimizations. So instead of doing those, this PR adds a new kludge to ReferencePropagation, which follows the same line of thinking as the kludge in CopyProp that solves this same problem inside that pass: https://github.com/rust-lang/rust/blob/a5584a8fe16037dc01782064fa41424a6dbe9987/compiler/rustc_mir_transform/src/copy_prop.rs#L65-L98

bors · 2025-06-08T05:45:14Z

☀️ Try build successful - checks-actions
Build commit: 920f4b7 (920f4b72087ccff7231e54c1928f5f1ce0a2b98e)

rust-timer · 2025-06-08T08:02:35Z

Finished benchmarking commit (920f4b7): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 1.1%, secondary 9.7%)

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.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	9.7%	[9.7%, 9.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Cycles

Results (secondary 0.7%)

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
Regressions ❌ (secondary)	4.3%	[4.3%, 4.3%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 752.018s -> 753.039s (0.14%)
Artifact size: 372.42 MiB -> 372.38 MiB (-0.01%)

rustbot · 2025-06-13T22:10:33Z

r? @lcnr

rustbot has assigned @lcnr.
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

rustbot · 2025-06-13T22:10:34Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

lcnr · 2025-06-16T11:59:45Z

r? wg-mir-opt

bors · 2025-07-03T17:44:37Z

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

dianqk · 2025-07-04T03:23:31Z

Is it reasonable to only change the locals we actually propagate to copies?

cjgillot · 2025-07-03T22:31:56Z

compiler/rustc_mir_transform/src/ref_prop.rs

@@ -87,11 +88,38 @@ impl<'tcx> crate::MirPass<'tcx> for ReferencePropagation {
    }
 }

+fn move_to_copy_pointers<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {


Could you add a doc-comment explaining why we need to transform those moves? A summary of your PR comment for instance?

Doc comment added!

cjgillot · 2025-07-04T12:50:34Z

Is it reasonable to only change the locals we actually propagate to copies?

This does not bother me much. This only change pointers, which we want codegen to pass by value anyway.

cjgillot · 2025-07-04T16:06:58Z

@bors r+

bors · 2025-07-04T16:07:00Z

📌 Commit cbe175f has been approved by cjgillot

It is now in the queue for this repository.

bors · 2025-07-04T22:35:32Z

💔 Test failed - checks-actions

Convert moves of references to copies in ReferencePropagation This is a fix for rust-lang#141101. The root cause of this miscompile is that the SsaLocals analysis that MIR transforms use is supposed to detect locals that are only written to once, in their single assignment. But that analysis is subtly wrong; it does not consider `Operand::Move` to be a write even though the meaning ascribed to `Operand::Move` (at least as a function parameter) by Miri is that the callee may have done arbitrary writes to the caller's Local that the Operand wraps (because `Move` is pass-by-pointer). So Miri conwiders `Operand::Move` to be a write but both the MIR visitor system considers it a read, and so does SsaLocals. I have tried fixing this by changing the `PlaceContext` that is ascribed to an `Operand::Move` to a `MutatingUseContext` but that seems to have borrow checker implications, and changing SsaLocals seems to have wide-ranging regressions in MIR optimizations. So instead of doing those, this PR adds a new kludge to ReferencePropagation, which follows the same line of thinking as the kludge in CopyProp that solves this same problem inside that pass: https://github.com/rust-lang/rust/blob/a5584a8fe16037dc01782064fa41424a6dbe9987/compiler/rustc_mir_transform/src/copy_prop.rs#L65-L98

jdonszelmann · 2025-07-15T08:21:44Z

@bors r- this one seems to have failed arm ci last week, and no changes happened? Don't see why we should try again now. Feel free to r+ again if I got that wrong, sorry in that case

saethlin · 2025-07-15T11:24:51Z

Oh. It should have gotten a r- the first time it failed oops.

saethlin · 2025-08-12T03:17:11Z

@bors try

Convert moves of references to copies in ReferencePropagation try-job: aarch64-apple

rust-bors · 2025-08-12T04:46:08Z

☀️ Try build successful (CI)
Build commit: 762eb51 (762eb510753bd18415e1b9c3f45645203a0e167d, parent: a1531335fe2807715fff569904d99602022643a7)

saethlin · 2025-08-22T19:27:32Z

@bors r=cjgillot rollup=iffy

bors · 2025-08-22T19:27:35Z

📌 Commit 9aa8cfa has been approved by cjgillot

It is now in the queue for this repository.

Convert moves of references to copies in ReferencePropagation This is a fix for rust-lang#141101. The root cause of this miscompile is that the SsaLocals analysis that MIR transforms use is supposed to detect locals that are only written to once, in their single assignment. But that analysis is subtly wrong; it does not consider `Operand::Move` to be a write even though the meaning ascribed to `Operand::Move` (at least as a function parameter) by Miri is that the callee may have done arbitrary writes to the caller's Local that the Operand wraps (because `Move` is pass-by-pointer). So Miri conwiders `Operand::Move` to be a write but both the MIR visitor system considers it a read, and so does SsaLocals. I have tried fixing this by changing the `PlaceContext` that is ascribed to an `Operand::Move` to a `MutatingUseContext` but that seems to have borrow checker implications, and changing SsaLocals seems to have wide-ranging regressions in MIR optimizations. So instead of doing those, this PR adds a new kludge to ReferencePropagation, which follows the same line of thinking as the kludge in CopyProp that solves this same problem inside that pass: https://github.com/rust-lang/rust/blob/a5584a8fe16037dc01782064fa41424a6dbe9987/compiler/rustc_mir_transform/src/copy_prop.rs#L65-L98

Rollup of 20 pull requests Successful merges: - #132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type) - #137396 (Recover `param: Ty = EXPR`) - #142185 (Convert moves of references to copies in ReferencePropagation) - #144443 (Make target pointer width in target json an integer) - #144648 (Implementation: `#[feature(nonpoison_rwlock)]`) - #144897 (print raw lifetime idents with r#) - #145218 ([Debuginfo] improve enum value formatting in LLDB for better readability) - #145380 (Add codegen-llvm regression tests) - #145573 (Add an experimental unsafe(force_target_feature) attribute.) - #145597 (resolve: Remove `ScopeSet::Late`) - #145641 (On E0277, point at type that doesn't implement bound) - #145669 (rustdoc-search: GUI tests check for `//` in URL) - #145695 (Introduce ProjectionElem::try_map.) - #145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64) - #145726 (Experiment: Reborrow trait) - #145731 (Make raw pointers work in type-based search) - #145736 (triagebot: Update style team reviewers) - #145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.) - #145743 (doc: fix some typos in comment) - #145745 (tests: Ignore basic-stepping.rs on LoongArch) Failed merges: - #145670 (port `sanitize` attribute to the new parsing infrastructure) r? `@ghost` `@rustbot` modify labels: rollup

Rollup of 28 pull requests Successful merges: - #132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type) - #137396 (Recover `param: Ty = EXPR`) - #137457 (Fix host code appearing in Wasm binaries) - #142185 (Convert moves of references to copies in ReferencePropagation) - #144648 (Implementation: `#[feature(nonpoison_rwlock)]`) - #144897 (print raw lifetime idents with r#) - #145218 ([Debuginfo] improve enum value formatting in LLDB for better readability) - #145380 (Add codegen-llvm regression tests) - #145573 (Add an experimental unsafe(force_target_feature) attribute.) - #145597 (resolve: Remove `ScopeSet::Late`) - #145633 (Fix some typos in LocalKey documentation) - #145641 (On E0277, point at type that doesn't implement bound) - #145669 (rustdoc-search: GUI tests check for `//` in URL) - #145695 (Introduce ProjectionElem::try_map.) - #145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64) - #145726 (Experiment: Reborrow trait) - #145731 (Make raw pointers work in type-based search) - #145736 (triagebot: Update style team reviewers) - #145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.) - #145742 (rustdoc js: Even more typechecking improvments ) - #145743 (doc: fix some typos in comment) - #145745 (tests: Ignore basic-stepping.rs on LoongArch) - #145747 (Refactor lint buffering to avoid requiring a giant enum) - #145751 (fix(lexer): Allow '-' in the frontmatter infostring continue set) - #145761 (Add aarch64_be-unknown-hermit target) - #145762 (convert strings to symbols in attr diagnostics) - #145763 (Ship LLVM tools for the correct target when cross-compiling) - #145765 (Revert suggestions for missing methods in tuples) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #142185 - saethlin:refprop-moves, r=cjgillot Convert moves of references to copies in ReferencePropagation This is a fix for #141101. The root cause of this miscompile is that the SsaLocals analysis that MIR transforms use is supposed to detect locals that are only written to once, in their single assignment. But that analysis is subtly wrong; it does not consider `Operand::Move` to be a write even though the meaning ascribed to `Operand::Move` (at least as a function parameter) by Miri is that the callee may have done arbitrary writes to the caller's Local that the Operand wraps (because `Move` is pass-by-pointer). So Miri conwiders `Operand::Move` to be a write but both the MIR visitor system considers it a read, and so does SsaLocals. I have tried fixing this by changing the `PlaceContext` that is ascribed to an `Operand::Move` to a `MutatingUseContext` but that seems to have borrow checker implications, and changing SsaLocals seems to have wide-ranging regressions in MIR optimizations. So instead of doing those, this PR adds a new kludge to ReferencePropagation, which follows the same line of thinking as the kludge in CopyProp that solves this same problem inside that pass: https://github.com/rust-lang/rust/blob/a5584a8fe16037dc01782064fa41424a6dbe9987/compiler/rustc_mir_transform/src/copy_prop.rs#L65-L98

Rollup of 28 pull requests Successful merges: - rust-lang/rust#132087 (Fix overly restrictive lifetime in `core::panic::Location::file` return type) - rust-lang/rust#137396 (Recover `param: Ty = EXPR`) - rust-lang/rust#137457 (Fix host code appearing in Wasm binaries) - rust-lang/rust#142185 (Convert moves of references to copies in ReferencePropagation) - rust-lang/rust#144648 (Implementation: `#[feature(nonpoison_rwlock)]`) - rust-lang/rust#144897 (print raw lifetime idents with r#) - rust-lang/rust#145218 ([Debuginfo] improve enum value formatting in LLDB for better readability) - rust-lang/rust#145380 (Add codegen-llvm regression tests) - rust-lang/rust#145573 (Add an experimental unsafe(force_target_feature) attribute.) - rust-lang/rust#145597 (resolve: Remove `ScopeSet::Late`) - rust-lang/rust#145633 (Fix some typos in LocalKey documentation) - rust-lang/rust#145641 (On E0277, point at type that doesn't implement bound) - rust-lang/rust#145669 (rustdoc-search: GUI tests check for `//` in URL) - rust-lang/rust#145695 (Introduce ProjectionElem::try_map.) - rust-lang/rust#145710 (Fix the ABI parameter inconsistency issue in debug.rs for LoongArch64) - rust-lang/rust#145726 (Experiment: Reborrow trait) - rust-lang/rust#145731 (Make raw pointers work in type-based search) - rust-lang/rust#145736 (triagebot: Update style team reviewers) - rust-lang/rust#145738 (Uplift rustc_mir_transform::coverage::counters::union_find to rustc_data_structures.) - rust-lang/rust#145742 (rustdoc js: Even more typechecking improvments ) - rust-lang/rust#145743 (doc: fix some typos in comment) - rust-lang/rust#145745 (tests: Ignore basic-stepping.rs on LoongArch) - rust-lang/rust#145747 (Refactor lint buffering to avoid requiring a giant enum) - rust-lang/rust#145751 (fix(lexer): Allow '-' in the frontmatter infostring continue set) - rust-lang/rust#145761 (Add aarch64_be-unknown-hermit target) - rust-lang/rust#145762 (convert strings to symbols in attr diagnostics) - rust-lang/rust#145763 (Ship LLVM tools for the correct target when cross-compiling) - rust-lang/rust#145765 (Revert suggestions for missing methods in tuples) r? `@ghost` `@rustbot` modify labels: rollup

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 8, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 8, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 8, 2025

This comment has been minimized.

Sign in to view

saethlin force-pushed the refprop-moves branch from a36e0de to d432379 Compare June 8, 2025 17:47

saethlin marked this pull request as ready for review June 13, 2025 22:10

rustbot assigned lcnr Jun 13, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2025

rustbot assigned JakobDegen and unassigned lcnr Jun 16, 2025

saethlin force-pushed the refprop-moves branch from d432379 to d2ed164 Compare July 3, 2025 22:26

cjgillot approved these changes Jul 4, 2025

View reviewed changes

saethlin force-pushed the refprop-moves branch from d2ed164 to cbe175f Compare July 4, 2025 15:49

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 4, 2025

This comment has been minimized.

Sign in to view

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 4, 2025

Kobzol mentioned this pull request Jul 14, 2025

Rollup of 14 pull requests #143946

Closed

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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2025

saethlin force-pushed the refprop-moves branch from cbe175f to e6ef54b Compare July 15, 2025 11:30

Convert moves of references to copies in RefProp

9aa8cfa

saethlin force-pushed the refprop-moves branch from e6ef54b to 9aa8cfa Compare August 12, 2025 03:16

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Aug 12, 2025

Auto merge of #142185 - saethlin:refprop-moves, r=<try>

762eb51

Convert moves of references to copies in ReferencePropagation try-job: aarch64-apple

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 Aug 22, 2025

jhpratt mentioned this pull request Aug 22, 2025

Rollup of 20 pull requests #145764

Closed

jhpratt mentioned this pull request Aug 23, 2025

Rollup of 28 pull requests #145773

Merged

bors merged commit bc4a643 into rust-lang:master Aug 23, 2025
11 checks passed

rustbot added this to the 1.91.0 milestone Aug 23, 2025

	/// `SsaLocals` computed equivalence classes between locals considering copy/move assignments.
	///
	/// This function also returns whether all the `move?` in the pattern are `move` and not copies.
	/// A local which is in the bitset can be replaced by `move _a`. Otherwise, it must be
	/// replaced by `copy _a`, as we cannot move multiple times from `_a`.
	///
	/// If an operand copies `_c`, it must happen before the assignment `_d = _c`, otherwise it is UB.
	/// This means that replacing it by a copy of `_a` if ok, since this copy happens before `_c` is
	/// moved, and therefore that `_d` is moved.
	#[instrument(level = "trace", skip(ssa, body))]
	fn fully_moved_locals(ssa: &SsaLocals, body: &Body<'_>) -> DenseBitSet<Local> {
	let mut fully_moved = DenseBitSet::new_filled(body.local_decls.len());

	for (_, rvalue, _) in ssa.assignments(body) {
	let (Rvalue::Use(Operand::Copy(place) \| Operand::Move(place))
	\| Rvalue::CopyForDeref(place)) = rvalue
	else {
	continue;
	};

	let Some(rhs) = place.as_local() else { continue };
	if !ssa.is_ssa(rhs) {
	continue;
	}

	if let Rvalue::Use(Operand::Copy(_)) \| Rvalue::CopyForDeref(_) = rvalue {
	fully_moved.remove(rhs);
	}
	}

	ssa.meet_copy_equivalence(&mut fully_moved);

	fully_moved
	}

Convert moves of references to copies in ReferencePropagation #142185

Convert moves of references to copies in ReferencePropagation #142185

Conversation

saethlin commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saethlin commented Jun 8, 2025

Uh oh!

This comment has been minimized.

bors commented Jun 8, 2025

Uh oh!

bors commented Jun 8, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Jun 8, 2025

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

This comment has been minimized.

rustbot commented Jun 13, 2025

Uh oh!

rustbot commented Jun 13, 2025

Uh oh!

lcnr commented Jun 16, 2025

Uh oh!

bors commented Jul 3, 2025

Uh oh!

dianqk commented Jul 4, 2025

Uh oh!

cjgillot Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

saethlin Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Jul 4, 2025

Uh oh!

cjgillot commented Jul 4, 2025

Uh oh!

bors commented Jul 4, 2025

Uh oh!

This comment has been minimized.

bors commented Jul 4, 2025

Uh oh!

jdonszelmann commented Jul 15, 2025

Uh oh!

saethlin commented Jul 15, 2025

Uh oh!

saethlin commented Aug 12, 2025

Uh oh!

This comment has been minimized.

rust-bors bot commented Aug 12, 2025

Uh oh!

saethlin commented Aug 22, 2025

Uh oh!

bors commented Aug 22, 2025

Uh oh!

Uh oh!

Uh oh!

saethlin commented Jun 8, 2025 •

edited

Loading