Expand async drops during drop elaboration#156649
Conversation
|
This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @vakaras Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 The Cranelift subtree was changed cc @bjorn3 Some changes occurred to the CTFE machinery |
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
1ed6528 to
20114b0
Compare
|
Is there some relation to #156495 or would that PR be orthogonal to this one? |
This comment has been minimized.
This comment has been minimized.
It should be orthogonal. |
This comment has been minimized.
This comment has been minimized.
|
uuh |
1a5cdbb to
a4336bc
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a4336bc to
7fe62e4
Compare
|
@rustbot reroll |
7fe62e4 to
a3420ce
Compare
This comment has been minimized.
This comment has been minimized.
a3420ce to
49f7a10
Compare
This comment has been minimized.
This comment has been minimized.
|
@cjgillot: looks like this is still evolving. Can you mark it as a draft? Or otherwise make it clear when it is ready for review? Thanks. |
|
This is ready for review. Just rebasing regularly. |
|
Ok! Thanks. |
This comment has been minimized.
This comment has been minimized.
elaborate_drop: Avoid as_mut.unwrap dance, empty vectors are cheap. Nit I found while working on rust-lang#156649
| patch.apply(&mut body); | ||
| } | ||
|
|
||
| // We did not bother respectig deref separation, do it here. |
There was a problem hiding this comment.
*respecting
| ), | ||
| ); | ||
|
|
||
| // We did not bother respectig deref separation, do it here. |
There was a problem hiding this comment.
*respecting
We generate a `*(_1.0)` projection in the `needs_sync_drop` case. Ensure that `deref_finder` is called on all paths.
We have `deref_finder` that introduces locals as needed. This allows to avoid bothering when writing the shim.
This makes code generation simpler, as we do not need to verify that successor blocks are only used once. This makes code generation simpler. The resulting MIR increase can easily be simplified by `SimplifyCfg`.
Same rationale as previous commit.
Drop elaboration runs early in the MIR pipeline. That pipeline is designed to work with unresolved function calls. Instance resolution during will call `codegen_select_candidate` when appropriate.
And also reduce the amount of intermediary locals. This shifts code to be mostly in reverse execution order.
This lets `StateTransform` focus on removing `Yield` terminators without adding new ones.
This is now unused.
49f7a10 to
d0e0cea
Compare
|
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. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Expand async drops during drop elaboration
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (c26a28f): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary 6.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeResults (primary -0.2%, secondary -0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 513.507s -> 510.839s (-0.52%) |
|
@nnethercote I added a few lines in each commit message. I'd like your review on those. And I added an extra commit with comment changes. |
|
The expanded commit messages look great! Thanks. @bors r+ |
This comment has been minimized.
This comment has been minimized.
Expand async drops during drop elaboration Async drops implemented by #123948 are expanded in await loops inside `StateTransform`. I find this odd, as `StateTransform` is tasked with removing yields from MIR. This PR moves this `async drop` -> loop transformation to drop elaboration. The first few commits create many mir-opt tests to gauge the effect of following commits. I can easily trim those. The next few commits are from #156422. Then come a few commits performing elementary simplifications to drop elaboration code. The most important commit is `Expand async drops during drop elaboration.` It performs the main change and attempts to document what I had to understand doing that. The last commit is a cleanup. This is a large PR, but most of the diff is in generated files. I can easily split them, each commit compiles and passes tests on its own. cc @zetanumbers
|
💔 Test for 6387066 failed: CI. Failed job:
|
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
View all comments
Async drops implemented by #123948 are expanded in await loops inside
StateTransform. I find this odd, asStateTransformis tasked with removing yields from MIR.This PR moves this
async drop-> loop transformation to drop elaboration.The first few commits create many mir-opt tests to gauge the effect of following commits. I can easily trim those.
The next few commits are from #156422.
Then come a few commits performing elementary simplifications to drop elaboration code.
The most important commit is
Expand async drops during drop elaboration.It performs the main change and attempts to document what I had to understand doing that.The last commit is a cleanup.
This is a large PR, but most of the diff is in generated files. I can easily split them, each commit compiles and passes tests on its own.
cc @zetanumbers