Skip to content

Conversation

beepster4096
Copy link
Contributor

@beepster4096 beepster4096 commented Aug 16, 2025

(split from my WIP #145344)

This PR:

  • Removes Rvalue::CopyForDeref and LocalInfo::DerefTemp from runtime MIR
    • Using a new mir pass EraseDerefTemps
    • CopyForDeref(x) is turned into Use(Copy(x))
    • DerefTemp is turned into Boring
      • Not sure if this part is actually necessary, it made more sense in [WIP] Underefer refactoring #145344 with DerefTemp storing actual data that I wanted to keep from having to be kept in sync with the rest of the body in runtime MIR
  • Checks in validation that CopyForDeref and DerefTemp are only used together
  • Removes special handling for CopyForDeref from many places
  • Removes CopyForDeref from custom_mir reverting Custom MIR: Support Rvalue::CopyForDeref #111587
    • In runtime MIR simple copies can be used instead
    • In post cleanup analysis MIR it was already wrong to use due to the lack of support for creating DerefTemp locals
    • Possibly this should be its own PR?
  • Adds an argument to deref_finder to avoid creating new DerefTemps and CopyForDeref in runtime MIR.
    • Ideally we would just avoid making intermediate derefs instead of fixing it at the end of a pass / during shim building
  • Removes some usages of deref_finder that I found out don't actually do anything

r? oli-obk

@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2025

oli-obk is not on the review rotation at the moment.
They may take a while to respond.

@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 Aug 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 16, 2025

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @vakaras

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to constck

cc @fee1-dead

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Member

(note that oli is on vacation and will be for quite a while)

@rust-log-analyzer

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

@rustbot author
r? ghost

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 17, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@beepster4096 beepster4096 force-pushed the erasedereftemps branch 2 times, most recently from 11f84dd to bfc73ec Compare August 18, 2025 07:07
@beepster4096
Copy link
Contributor Author

r? mir
@rustbot ready

@rustbot rustbot 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 Aug 18, 2025
@davidtwco
Copy link
Member

It's been long enough since I've looked at a involved MIR change that I'm probably not best suited to this

r? mir

@rustbot rustbot assigned saethlin and unassigned davidtwco Aug 21, 2025
@saethlin
Copy link
Member

saethlin commented Sep 2, 2025

This mostly touches MIR stuff that I'm less familiar with. @cjgillot and @tmiasko might see something in this that I don't.

I'm reviewing this over the next week or so, just slowly.

@bors
Copy link
Collaborator

bors commented Sep 17, 2025

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

@rustbot

This comment has been minimized.

@beepster4096
Copy link
Contributor Author

I moved erase coroutine shim dereftemps before the test blessing commits in this force push so I didn't have to use --keep-stage 1 to keep std from ICEing. This order makes a bit more sense too

@rust-log-analyzer

This comment has been minimized.

@beepster4096 beepster4096 force-pushed the erasedereftemps branch 2 times, most recently from d99b187 to d7824f2 Compare September 19, 2025 00:51
@cjgillot cjgillot self-assigned this Sep 19, 2025
@bors
Copy link
Collaborator

bors commented Oct 3, 2025

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

@saethlin
Copy link
Member

saethlin commented Oct 5, 2025

I've finally got my head around the diff, and I think this is conceptually simple but annoying to implement.

I know there are currently merge conflicts, but what happens if I

@bors r+ rollup=iffy (conflict-prone)

@bors
Copy link
Collaborator

bors commented Oct 5, 2025

📌 Commit d7824f2 has been approved by saethlin

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 Oct 5, 2025
@bors
Copy link
Collaborator

bors commented Oct 6, 2025

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout erasedereftemps (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self erasedereftemps --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging tests/mir-opt/inline_coroutine_body.run2-{closure#0}.Inline.panic-unwind.diff
Removing tests/mir-opt/building/custom/projections.copy_for_deref.built.after.mir
Auto-merging compiler/rustc_span/src/symbol.rs
Auto-merging compiler/rustc_mir_transform/src/validate.rs
Auto-merging compiler/rustc_mir_transform/src/ref_prop.rs
Auto-merging compiler/rustc_mir_transform/src/jump_threading.rs
Auto-merging compiler/rustc_mir_transform/src/inline.rs
CONFLICT (content): Merge conflict in compiler/rustc_mir_transform/src/inline.rs
Auto-merging compiler/rustc_mir_transform/src/gvn.rs
Auto-merging compiler/rustc_mir_transform/src/dest_prop.rs
Auto-merging compiler/rustc_mir_transform/src/dataflow_const_prop.rs
Auto-merging compiler/rustc_mir_transform/src/coroutine.rs
Auto-merging compiler/rustc_mir_transform/src/copy_prop.rs
Auto-merging compiler/rustc_middle/src/mir/syntax.rs
Auto-merging compiler/rustc_middle/src/mir/mod.rs
Auto-merging compiler/rustc_const_eval/src/check_consts/qualifs.rs
Auto-merging compiler/rustc_codegen_ssa/src/mir/rvalue.rs
Auto-merging compiler/rustc_codegen_cranelift/src/base.rs
Auto-merging compiler/rustc_borrowck/src/lib.rs
Automatic merge failed; fix conflicts and then commit the result.

@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 Oct 6, 2025
some optimization is behaving slightly differently on box derefs after this change, but the difference is irrelevant
it did not create DerefTemp locals when used, so it was never actually correct.
elaborate drops and inline don't seem to actually need it
@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2025

This PR was rebased onto a different master 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.

@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu-llvm-20-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- [mir-opt] tests/mir-opt/dead-store-elimination/ref.rs stdout ----
13   
14       bb0: {
15 -         StorageLive(_2);
- -         _3 = deref_copy (_1.1: &Foo);
+ -         _3 = copy (_1.1: &Foo);
17 -         _2 = &((*_3).2: i32);
18 +         // DBG: _2 = &((*_3).2: i32);
-           _4 = deref_copy (_1.1: &Foo);
+           _4 = copy (_1.1: &Foo);
20           _0 = copy ((*_4).0: i32);
21 -         StorageDead(_2);
22           return;


thread '[mir-opt] tests/mir-opt/dead-store-elimination/ref.rs' panicked at src/tools/compiletest/src/runtest/mir_opt.rs:84:21:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants