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.

@beepster4096
Copy link
Contributor Author

panic=abort mir-opt tests :(

@rustbot ready

@saethlin
Copy link
Member

@bors try jobs=test-various

rust-bors bot added a commit that referenced this pull request Oct 11, 2025
Validate CopyForDeref and DerefTemps better and remove them from runtime MIR

try-job: test-various
@rust-bors

This comment has been minimized.

@rust-bors
Copy link

rust-bors bot commented Oct 11, 2025

☀️ Try build successful (CI)
Build commit: c757d3d (c757d3d5f78506f1f6c8c5fa615ad774d49f20c7, parent: 442288534b6cf9ec4899b00c4332433b17760d96)

@saethlin
Copy link
Member

@bors r=saethlin,cjgillot

@bors
Copy link
Collaborator

bors commented Oct 11, 2025

📌 Commit 033474b 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 11, 2025
@bors
Copy link
Collaborator

bors commented Oct 12, 2025

⌛ Testing commit 033474b with merge 3be6803...

@bors
Copy link
Collaborator

bors commented Oct 12, 2025

☀️ Test successful - checks-actions
Approved by: saethlin,cjgillot
Pushing 3be6803 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 12, 2025
@bors bors merged commit 3be6803 into rust-lang:master Oct 12, 2025
12 checks passed
@rustbot rustbot added this to the 1.92.0 milestone Oct 12, 2025
@github-actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing be0ade2 (parent) -> 3be6803 (this PR)

Test differences

Show 64 test diffs

64 doctest diffs were found. These are ignored, as they are noisy.

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 3be68033b67dfc2aa3ae4cfe735aa5805aebae43 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. dist-aarch64-linux: 6101.0s -> 8824.7s (44.6%)
  2. dist-apple-various: 4101.9s -> 3150.1s (-23.2%)
  3. x86_64-rust-for-linux: 2584.4s -> 3024.4s (17.0%)
  4. x86_64-gnu-distcheck: 6645.3s -> 7664.0s (15.3%)
  5. aarch64-msvc-2: 4934.9s -> 5593.4s (13.3%)
  6. aarch64-apple: 7663.7s -> 8661.8s (13.0%)
  7. aarch64-gnu-llvm-20-2: 2277.7s -> 2543.2s (11.7%)
  8. dist-x86_64-apple: 7150.9s -> 6326.7s (-11.5%)
  9. dist-aarch64-msvc: 5757.4s -> 6396.4s (11.1%)
  10. x86_64-gnu-aux: 6079.8s -> 6750.0s (11.0%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3be6803): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 4
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 3.7%, secondary -1.1%)

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

mean range count
Regressions ❌
(primary)
10.2% [10.2%, 10.2%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.9% [-2.9%, -2.9%] 1
Improvements ✅
(secondary)
-1.1% [-1.8%, -0.6%] 3
All ❌✅ (primary) 3.7% [-2.9%, 10.2%] 2

Cycles

Results (primary 2.6%, secondary 0.9%)

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

mean range count
Regressions ❌
(primary)
2.6% [2.6%, 2.6%] 1
Regressions ❌
(secondary)
2.8% [2.6%, 3.1%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) 2.6% [2.6%, 2.6%] 1

Binary size

Results (primary 0.0%, secondary 0.1%)

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

mean range count
Regressions ❌
(primary)
0.1% [0.0%, 0.3%] 9
Regressions ❌
(secondary)
0.1% [0.0%, 0.3%] 14
Improvements ✅
(primary)
-0.2% [-0.2%, -0.1%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [-0.2%, 0.3%] 12

Bootstrap: 472.888s -> 472.207s (-0.14%)
Artifact size: 388.10 MiB -> 388.10 MiB (-0.00%)

samueltardieu added a commit to samueltardieu/rust that referenced this pull request Oct 17, 2025
…hlin

Undo CopyForDeref assertion in const qualif

Fixes rust-lang#147733 caused by rust-lang#145513

This code in fact does not run only on runtime MIR.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 18, 2025
…hlin

Undo CopyForDeref assertion in const qualif

Fixes rust-lang#147733 caused by rust-lang#145513

This code in fact does not run only on runtime MIR.
rust-timer added a commit that referenced this pull request Oct 18, 2025
Rollup merge of #147764 - beepster4096:oopsies_sorry, r=saethlin

Undo CopyForDeref assertion in const qualif

Fixes #147733 caused by #145513

This code in fact does not run only on runtime MIR.
makai410 pushed a commit to makai410/rust that referenced this pull request Nov 8, 2025
…hlin,cjgillot

Validate CopyForDeref and DerefTemps better and remove them from runtime MIR

(split from my WIP rust-lang#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 rust-lang#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 rust-lang#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
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Nov 8, 2025
…hlin,cjgillot

Validate CopyForDeref and DerefTemps better and remove them from runtime MIR

(split from my WIP rust-lang#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 rust-lang#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 rust-lang#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
makai410 pushed a commit to makai410/rust that referenced this pull request Nov 10, 2025
…hlin,cjgillot

Validate CopyForDeref and DerefTemps better and remove them from runtime MIR

(split from my WIP rust-lang#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 rust-lang#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 rust-lang#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors This PR was explicitly merged by bors. 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. 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.

10 participants