mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in `EarlyOtherwiseBranch` #130786

dianqk · 2024-09-24T14:31:07Z

r? @cjgillot or mir-opt

rustbot · 2024-09-24T14:31:17Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

dianqk · 2024-09-24T14:32:18Z

compiler/rustc_middle/src/mir/mod.rs

@@ -1337,8 +1337,8 @@ pub struct BasicBlockData<'tcx> {
 }

 impl<'tcx> BasicBlockData<'tcx> {
-    pub fn new(terminator: Option<Terminator<'tcx>>) -> BasicBlockData<'tcx> {
-        BasicBlockData { statements: vec![], terminator, is_cleanup: false }
+    pub fn new(terminator: Option<Terminator<'tcx>>, is_cleanup: bool) -> BasicBlockData<'tcx> {


This should help remind us to set is_cleanup.

wesleywiser · 2024-09-26T14:26:08Z

tests/mir-opt/early_otherwise_branch.rs

 fn main() {
    opt1(None, Some(0));
    opt2(None, Some(0));
    opt3(Option2::None, Option2::Some(false));
    opt4(Option2::None, Option2::Some(0));
+    opt5(0, 0);
+    opt5_failed(0, 0);


Can we also include a version of the reproducer found here as a test case? #130769 (comment)

I have added this: https://github.com/rust-lang/rust/pull/130786/files#diff-50c7b6495288a4628220c4cd9bba0ccd732c336f03738681353d6b64dc0cfa79R29-R43. But, I think the current fn unwind test case is sufficient.

dianqk · 2024-10-30T14:54:14Z

:) ping?

dianqk · 2024-11-02T02:24:55Z

@cjgillot The actual changes in this PR are small, so it should be easy to review. Feel free to re-roll if you like.

dianqk · 2024-11-26T00:23:44Z

@cjgillot three week ping :p

cjgillot · 2024-11-26T00:42:45Z

compiler/rustc_mir_transform/src/early_otherwise_branch.rs

+                        targets: eq_targets,
+                    },
+                }),
+                bbs[parent].is_cleanup,


Should we just skip trying to optimize parent if it's a cleanup block?

Skipping is conservative and fine, but I don't see how optimizing a cleanup BB would be a problem.

But optimizing a cleanup BB here has no obvious improvement. What do you think?

This previously caused a regression, so I have skipped it.

dianqk · 2024-12-17T01:54:00Z

Three weeks ping

r? cjgillot
r? oli-obk

rustbot · 2024-12-17T01:54:02Z

Could not assign reviewer from: cjgillot.
User(s) cjgillot are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

bors · 2024-12-17T15:22:45Z

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

oli-obk · 2024-12-18T10:11:12Z

r=me

…ch_scalar, r=cjgillot" This reverts commit 16a0266.

dianqk · 2024-12-18T14:21:05Z

I have rebased.

@bors r=oli-obk

bors · 2024-12-18T14:21:08Z

📌 Commit d08738c has been approved by oli-obk

It is now in the queue for this repository.

Rollup of 11 pull requests Successful merges: - rust-lang#130786 ( mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in `EarlyOtherwiseBranch`) - rust-lang#133926 (Fix const conditions for RPITITs) - rust-lang#134161 (Overhaul token cursors) - rust-lang#134253 (Overhaul keyword handling) - rust-lang#134394 (Clarify the match ergonomics 2024 migration lint's output) - rust-lang#134399 (Do not do if ! else, use unnegated cond and swap the branches instead) - rust-lang#134420 (refactor: replace &PathBuf with &Path to enhance generality) - rust-lang#134436 (tests/assembly/asm: Remove uses of rustc_attrs and lang_items features by using minicore) - rust-lang#134444 (Fix `x build --stage 1 std` when using cg_cranelift as the default backend) - rust-lang#134452 (fix(LazyCell): documentation of get[_mut] was wrong) - rust-lang#134460 (Merge some patterns together) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130786 - DianQK:early_otherwise_branch_cleanup, r=oli-obk mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in `EarlyOtherwiseBranch` Fixes rust-lang#130769. r? `@cjgillot` or mir-opt

rustbot assigned cjgillot Sep 24, 2024

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 Sep 24, 2024

dianqk commented Sep 24, 2024

View reviewed changes

dianqk mentioned this pull request Sep 24, 2024

Panic while compiling http-body v0.4.6 with rustc 1.83.0-nightly (7042c269c 2024-09-23) #130769

Closed

dianqk force-pushed the early_otherwise_branch_cleanup branch from 04d03e2 to 04f61e3 Compare September 24, 2024 14:51

jieyouxu added the A-mir-opt Area: MIR optimizations label Sep 24, 2024

dianqk force-pushed the early_otherwise_branch_cleanup branch from 04f61e3 to 42f1deb Compare September 24, 2024 23:14

wesleywiser reviewed Sep 26, 2024

View reviewed changes

dianqk force-pushed the early_otherwise_branch_cleanup branch from 42f1deb to 2237f02 Compare September 27, 2024 12:30

dianqk force-pushed the early_otherwise_branch_cleanup branch from 2237f02 to d98c1d0 Compare October 30, 2024 14:46

cjgillot reviewed Nov 26, 2024

View reviewed changes

dianqk force-pushed the early_otherwise_branch_cleanup branch from d98c1d0 to 3ba1001 Compare November 27, 2024 00:00

rustbot assigned oli-obk and unassigned cjgillot Dec 17, 2024

dianqk assigned cjgillot Dec 17, 2024

oli-obk approved these changes Dec 18, 2024

View reviewed changes

dianqk added 4 commits December 18, 2024 20:43

Reapply "Auto merge of rust-lang#129047 - DianQK:early_otherwise_bran…

7a0b78d

…ch_scalar, r=cjgillot" This reverts commit 16a0266.

mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB

15fa788

mir: require is_cleanup when creating BasicBlockData

93aea1d

mir-opt: Do not handle the cleanup BB in the EarlyOtherwiseBranch

d08738c

dianqk force-pushed the early_otherwise_branch_cleanup branch from 3ba1001 to d08738c Compare December 18, 2024 12:52

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 Dec 18, 2024

jieyouxu mentioned this pull request Dec 18, 2024

Rollup of 11 pull requests #134470

Merged

bors merged commit 2d08e94 into rust-lang:master Dec 18, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Dec 18, 2024

dianqk deleted the early_otherwise_branch_cleanup branch December 18, 2024 22:20

mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in EarlyOtherwiseBranch #130786

mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in EarlyOtherwiseBranch #130786

Uh oh!

Conversation

dianqk commented Sep 24, 2024

Uh oh!

rustbot commented Sep 24, 2024

Uh oh!

dianqk Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

wesleywiser Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

dianqk Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

dianqk commented Oct 30, 2024

Uh oh!

dianqk commented Nov 2, 2024

Uh oh!

dianqk commented Nov 26, 2024

Uh oh!

cjgillot Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dianqk Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dianqk Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

dianqk Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

dianqk commented Dec 17, 2024

Uh oh!

rustbot commented Dec 17, 2024

Uh oh!

bors commented Dec 17, 2024

Uh oh!

oli-obk commented Dec 18, 2024

Uh oh!

dianqk commented Dec 18, 2024

Uh oh!

bors commented Dec 18, 2024

Uh oh!

Uh oh!

Uh oh!

mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in `EarlyOtherwiseBranch` #130786

mir-opt: a sub-BB of a cleanup BB must also be a cleanup BB in `EarlyOtherwiseBranch` #130786