Rework MIR drop tree lowering #71840
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 000824f51e9854bae576802d2e6aefc7e8362820 with merge d4d72c3f314944d906c0b5214cc5062315df842b... |
|
☀️ Try build successful - checks-azure |
|
Queued d4d72c3f314944d906c0b5214cc5062315df842b with parent e5f35df, future comparison URL. |
|
Finished benchmarking try commit d4d72c3f314944d906c0b5214cc5062315df842b, comparison URL. |
|
|
r? @oli-obk |
|
So I'm guessing the perf regression is codegen unit related (rust-lang/compiler-team#281) :/ cc @wesleywiser |
oli-obk
reviewed
May 5, 2020
oli-obk
reviewed
May 5, 2020
oli-obk
reviewed
May 6, 2020
There was a problem hiding this comment.
I like this split. It's much more comprehensible. First we record all the drop sites in a datastructure holding everything we need, then we process that datastructure in one go.
I'm not sure how to actually verify that this doesn't regress anything, I'm going through the MIR diffs as a next step now, and maybe we should do a full crater run just to get some more confidence. The algorithm as written makes sense to me, but I don't know the previous logic well enough to judge whether this changes anything in problematic ways.
| simply calls `diverge_cleanup()` and adds an edge from `p` to the | ||
| result. | ||
| Panics are handled in a similar fashion, except that the drops are added to the | ||
| mir once the rest of the function has finished being lowered. If a terminator |
There was a problem hiding this comment.
Suggested change
| mir once the rest of the function has finished being lowered. If a terminator | |
| MIR once the rest of the function has finished being lowered. If a terminator |
| result. | ||
| Panics are handled in a similar fashion, except that the drops are added to the | ||
| mir once the rest of the function has finished being lowered. If a terminator | ||
| can panic, call `diverge_from(block)` with the block containing the terminator |
There was a problem hiding this comment.
[off topic] I'm having serious come_from flashbacks here 😆
|
I have gone through the mir changes and am confident that this PR only changes the order of basic blocks or deduplicates some of them. Although I must say reviewing diffs of diffs is confusing business, even with side-by-side view, but these were not very drop-interesting diffs anyway, so I don't think I made any mistakes there. So while I realize a 170% build time regression on Reminder: the build time regression is solely due to some functions now having slightly different basic block numbers/layouts, causing our codegen-unit splitting algorithm to throw them into different codegen units, making llvm take up much more time in optimizing them. See rust-lang/compiler-team#281 for details. |
|
I agree that if this patch is good, and I believe based on the discussion we are in agreement on it being largely desirable, the performance regression is acceptable; it seems highly plausible that a small change in where the println being added is placed would have similar effects anyway - and it only affects one of our dozen odd benchmarks, too. |
|
Re: reviewing MIR diffs. It's maybe a little late, but comparing the rendered graphviz output is probably easier than checking the text representation of the MIR. |
|
@bors r+ Let's merge this. It's way too good to let bitrot while we figure out the cgu situation. It's already added to the gist of rust-lang/compiler-team#281 so we've documented it and will look at it in the future. |
|
📌 Commit 1498ef21c70306c19790fcfd392aff24c1958e92 has been approved by |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #72036) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
|
@bors r=oli-obk |
|
📌 Commit b998497 has been approved by |
bors
added
S-waiting-on-bors
RalfJung
added a commit
to RalfJung/rust
that referenced
this pull request
May 10, 2020
Rework MIR drop tree lowering This PR changes how drops are generated in MIR construction. This is the first half of the fix for rust-lang#47949. Rather than generating the drops for a given unwind/break/continue/return/generator drop path as soon as they are needed, the required drops are recorded and get generated later. The motivation for this is * It simplifies the caching scheme, because it's now possible to walk up the currently scheduled drop tree to recover state. * The basic block order for MIR more closely resembles execution order. This PR also: * Highlights cleanup blocks in the graphviz MIR output. * Removes some unnecessary drop flag assignments.
This was referenced May 10, 2020
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 10, 2020
Rollup of 4 pull requests Successful merges: - rust-lang#71840 (Rework MIR drop tree lowering) - rust-lang#71882 (Update the `cc` crate) - rust-lang#71945 (Sort "implementations on foreign types" section in the sidebar) - rust-lang#72043 (Add missing backtick in E0569 explanation) Failed merges: r? @ghost
|
(Would have been better to rollup=never this since it was clear it would show up in perf -- that would have saved the time to do the analysis of figuring out which PR caused it. Lucky enough it was a small rollup.) |
Merged
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Jun 7, 2020
…jasper Revert pr 71840 Revert7 PR rust-lang#71840 to fix issue rust-lang#72470 This will need a backport to beta if we do not want rust-lang#72470 to hit stable.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 5, 2020
…jasper Re-land PR rust-lang#71840 (Rework MIR drop tree lowering) PR rust-lang#71840 was reverted in rust-lang#72989 to fix an LLVM error (rust-lang#72470). That LLVM error no longer occurs with the recent upgrade to LLVM 11 (rust-lang#73526), so let's try re-landing this PR. I've cherry-picked the commits from the original PR (with the exception of the commit blessing test output), making as few modifications as possible. I addressed the rebase fallout in separate commits on top of those. r? `@matthewjasper`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes how drops are generated in MIR construction. This is the first half of the fix for #47949.
Rather than generating the drops for a given unwind/break/continue/return/generator drop path as soon as they are needed, the required drops are recorded and get generated later.
The motivation for this is
This PR also: