Skip to content

Correct and document semantics of yield terminator#156875

Merged
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
cjgillot:yield-drop-resume
May 30, 2026
Merged

Correct and document semantics of yield terminator#156875
rust-bors[bot] merged 3 commits into
rust-lang:mainfrom
cjgillot:yield-drop-resume

Conversation

@cjgillot
Copy link
Copy Markdown
Contributor

@cjgillot cjgillot commented May 24, 2026

View all comments

The current implementation of dataflow for the yield terminator confuses the drop target with unwinding. Which is wrong, in particular when implementing async drops.

r? @RalfJung

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 24, 2026

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

This PR changes MIR

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

@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. labels May 24, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 24, 2026

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

@rust-log-analyzer

This comment has been minimized.

@cjgillot cjgillot force-pushed the yield-drop-resume branch from c2fa5ba to 4141c4c Compare May 24, 2026 12:27
Copy link
Copy Markdown
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I don't know nearly enough about async lowering to review this.

@rustbot reroll
(Or maybe you know a good reviewer?)

View changes since this review

Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
@rustbot rustbot assigned adwinwhite and unassigned RalfJung May 24, 2026
@adwinwhite
Copy link
Copy Markdown
Contributor

@rustbot reroll

@rustbot rustbot assigned oli-obk and unassigned adwinwhite May 24, 2026
Comment thread compiler/rustc_middle/src/mir/syntax.rs Outdated
Copy link
Copy Markdown
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with doc review changes applied.

I was thinking and playing a bit with writing a run test, but it's not really worth it. It's an extreme corner case.

A mir-opt test would still be useful showing how the unwinding targets change now

View changes since this review

@rustbot rustbot 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 May 26, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 26, 2026

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

@cjgillot
Copy link
Copy Markdown
Contributor Author

I was thinking and playing a bit with writing a run test, but it's not really worth it. It's an extreme corner case.

A mir-opt test would still be useful showing how the unwinding targets change now

The only difference I could see in tests is the size of async-drop coroutine state. On the current (wrong) semantics, I got a size increase when trying to change async drop expansion: the resume argument was (wrongly) saved in coroutine state.

@cjgillot cjgillot force-pushed the yield-drop-resume branch from ce3102b to 9ef4f52 Compare May 29, 2026 20:24
@rustbot

This comment has been minimized.

@cjgillot
Copy link
Copy Markdown
Contributor Author

@bors r=oli-obk

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 30, 2026

📌 Commit 9ef4f52 has been approved by oli-obk

It is now in the queue for this repository.

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@rust-bors rust-bors Bot 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 May 30, 2026
rust-bors Bot pushed a commit that referenced this pull request May 30, 2026
…uwer

Rollup of 12 pull requests

Successful merges:

 - #154591 (Remove `will_cache_on_disk_for_key_fn`)
 - #156672 (Misc improvements to coroutine transform code)
 - #157027 (HIR ty lowering: Move some things into submodules)
 - #157051 (Allow two object files for a single CGU in CompiledModule)
 - #157100 (Some more per owner things)
 - #153497 (Use `trait_object_dummy_self` more & heavily fix+update related docs)
 - #155638 (Fix tupled closure signature in `AsyncFn` arg mismatch diagnostic)
 - #156826 (style: Clarify nullary call and `()` no-break rule applies past max width)
 - #157004 (Remove unused functions in `value_analysis.rs`)
 - #157032 (Fixed more &x ->&mut x suggestions)
 - #157033 (Note irrefutable while let in loop type errors)
 - #157139 (compiler: `ops::RangeInclusive` → `range::RangeInclusive`)

Failed merges:

 - #156875 (Correct and document semantics of `yield` terminator)
@rust-bors rust-bors Bot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 30, 2026
@rust-bors

This comment has been minimized.

@rust-bors rust-bors Bot removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 30, 2026
@cjgillot cjgillot force-pushed the yield-drop-resume branch from 9ef4f52 to b542677 Compare May 30, 2026 14:46
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented May 30, 2026

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
Copy link
Copy Markdown
Contributor Author

@bors r=oli-obk

@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors Bot commented May 30, 2026

📌 Commit b542677 has been approved by oli-obk

It is now in the queue for this repository.

@rust-bors rust-bors Bot 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 May 30, 2026
rust-bors Bot pushed a commit that referenced this pull request May 30, 2026
…uwer

Rollup of 8 pull requests

Successful merges:

 - #156863 (Make hint::cold_path #[cold] so that it works even if the MIR inliner can't inline it)
 - #156875 (Correct and document semantics of `yield` terminator)
 - #157115 ([rustdoc] Fix foreign items macro expansion)
 - #157150 (Revert "drop derive helpers during ast lowering" )
 - #156887 (Rename `-Zdebuginfo-for-profiling` switch)
 - #157039 (rustdoc: correctly propagate cfgs for glob reexports)
 - #157125 (Rewrite the `#[repr]` attribute parser)
 - #157154 (Revert workaround used to select the gcc codegen in the coretests CI)
@rust-bors rust-bors Bot merged commit 95a2369 into rust-lang:main May 30, 2026
12 checks passed
@rustbot rustbot added this to the 1.98.0 milestone May 30, 2026
rust-timer added a commit that referenced this pull request May 30, 2026
Rollup merge of #156875 - cjgillot:yield-drop-resume, r=oli-obk

Correct and document semantics of `yield` terminator

The current implementation of dataflow for the yield terminator confuses the `drop` target with unwinding. Which is wrong, in particular when implementing async drops.

r? @RalfJung
@cjgillot cjgillot deleted the yield-drop-resume branch May 30, 2026 21:15
@JonathanBrouwer
Copy link
Copy Markdown
Contributor

@rust-timer build bb4f119

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (bb4f119): comparison URL.

Overall result: ❌ regressions - 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.2% [0.1%, 0.2%] 16
Regressions ❌
(secondary)
0.5% [0.1%, 2.4%] 18
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.1%, 0.2%] 16

Max RSS (memory usage)

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

Cycles

Results (secondary 6.7%)

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

Binary size

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

Bootstrap: 515.367s -> 513.684s (-0.33%)
Artifact size: 400.82 MiB -> 400.81 MiB (-0.00%)

@rustbot rustbot added the perf-regression Performance regression. label May 31, 2026
@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented May 31, 2026

(thanks Jonathan!)

Looks like this PR caused a mild regression in check builds. Maybe because of the new Box? Is there some common number of return statements, maybe that could be a SmallVec?

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

Labels

perf-regression Performance regression. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants