10000 Validate CopyForDeref and DerefTemps better and remove them from runtime MIR by beepster4096 · Pull Request #145513 · rust-lang/rust · GitHub
[go: up one dir, main page]
Skip to content

Validate CopyForDeref and DerefTemps better and remove them from runtime MIR #145513

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
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Validate CopyForDeref and DerefTemps better and remove them from runtime MIR #145513

wants to merge 6 commits into from

Conversation

beepster4096
Copy link
Contributor
@beepster4096 beepster4096 commented Aug 16, 2025 •
edited
Loading

(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.

Sign in to view
@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.

Sign in to view
@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.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@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 8000
bors commented Sep 17, 2025

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

@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
@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

This comment has been minimized.

Sign in to view
@beepster4096
Copy link
Contributor Author

@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 Oct 10, 2025
@cjgillot
Copy link
Contributor

@bors r=saethlin,cjgillot

@bors
Copy link
Collaborator
bors commented Oct 10, 2025

📌 Commit 3acaac4 has been approved by saethlin,cjgillot

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

⌛ Testing commit 3acaac4 with merge 1d9150a...

bors added a commit that referenced this pull request Oct 10, 2025
@bors
Auto merge of #145513 - beepster4096:erasedereftemps, r=saethlin,cjgi…
1d9150a 
…llot

Validate CopyForDeref and DerefTemps better and remove them from runtime MIR

(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 #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 #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 `DerefTemp`s 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
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator
bors commented Oct 10, 2025

💔 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 Oct 10, 2025
@beepster4096 beepster4096 force-pushed the erasedereftemps branch 2 times, most recently from c85f 9E88 cc5 to 272b076 Compare October 11, 2025 02:43
@beepster4096
slightly less trivial blessings
1bd4901 
some optimization is behaving slightly differently on box derefs after this change, but the difference is irrelevant
@beepster4096
remove copyforderef from custom_mir
2da55cd 
it did not create DerefTemp locals when used, so it was never actually correct.
@beepster4096
remove some deref_finder uses
033474b 
elaborate drops and inline don't seem to actually need it
@beepster4096
Copy link
Contributor Author

panic=abort mir-opt tests :(

@rustbot ready

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

Reviewers

No reviews

Assignees

@cjgillot cjgillot

@saethlin saethlin

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@beepster4096 @rustbot @rust-log-analyzer @WaffleLapkin @davidtwco @saethlin @bors @cjgillot @oli-obk
0