New mir-opt pass to simplify gotos with const values #77486

simonvandel · 2020-10-03T13:09:44Z

This pass optimizes the following sequence

bb2: {
    _2 = const true;
    goto -> bb3;
}

bb3: {
    switchInt(_2) -> [false: bb4, otherwise: bb5];
}

into

bb2: {
    _2 = const true;
    goto -> bb5;
}

Note that from the diff it seems to invalidate some of the other passes - not sure how best to handle that.

rust-highfive · 2020-10-03T13:09:48Z

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

simonvandel · 2020-10-03T14:10:41Z

I can't seem to reproduce the CI failing. ./x.py test passes all tests locally.

lcnr · 2020-10-03T14:24:13Z

This optimization looks good to me, it should be quite fast and seems to get triggered suprisingly frequently from what I can tell

r? @oli-obk though, won't review mir opts myself

compiler/rustc_mir/src/transform/const_goto.rs

tmiasko · 2020-10-03T16:15:34Z

The CI failure looks like miscompilation of rustc, you might need to test the second stage: ./x.py test --stage 2 to reproduce.

tmiasko · 2020-10-03T19:12:52Z

The report_inline_asm function is being miscompiled (it unconditionally sets cookie to zero). The SimplifyBranchSame seems to be the optimization responsible, in particular the use of zip without checking that the number of statements is the same looks very suspicious:

rust/compiler/rustc_mir/src/transform/simplify_try.rs

Line 633 in 738d4a7

    
           bb_l.statements.iter().zip(&bb_r.statements).try_fold(StatementEquality::TrivialEqual, |acc,(l,r)| {

--- rustc_codegen_llvm.back-write-report_inline_asm.005-010.SimplifyBranchSame.before.mir
+++ rustc_codegen_llvm.back-write-report_inline_asm.005-010.SimplifyBranchSame.after.mir
@@ -1,11 +1,11 @@
-// MIR for `report_inline_asm` before SimplifyBranchSame
+// MIR for `report_inline_asm` after SimplifyBranchSame
 
 fn report_inline_asm(_1: &CodegenContext<LlvmCodegenBackend>, _2: std::string::String, _3: llvm_::ffi::DiagnosticLevel, _4: u32, _5: Option<(std::string::String, Vec<InnerSpan>)>) -> () {
     debug cgcx => _1;                    // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:244:5: 244:9
     debug msg => _2;                     // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:245:5: 245:8
     debug level => _3;                   // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:246:5: 246:10
     debug cookie => _4;                  // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:247:5: 247:15
     debug source => _5;                  // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:248:5: 248:11
     let mut _0: ();                      // return place in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:249:3: 249:3
     let mut _6: isize;                   // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:27: 253:35
     let _7: rustc_errors::Level;         // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:9: 256:14
@@ -15,80 +15,76 @@
     let mut _11: u32;                    // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:261:40: 261:53
     let mut _12: std::string::String;    // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:261:55: 261:58
     let mut _13: rustc_errors::Level;    // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:261:60: 261:65
     let mut _14: std::option::Option<(std::string::String, std::vec::Vec<rustc_span::InnerSpan>)>; // in scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:261:67: 261:73
     scope 1 {
         debug level => _7;               // in scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:256:9: 256:14
     }
 
     bb0: {
         _6 = discriminant(((*_1).2: rustc_session::config::Lto)); // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:27: 253:35
-        switchInt(move _6) -> [1_isize: bb3, 3_isize: bb3, otherwise: bb2]; // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:27: 253:35
+        goto -> bb2;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:27: 253:35
     }
 
     bb1 (cleanup): {
         resume;                          // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:243:1: 262:2
     }
 
     bb2: {
-        goto -> bb4;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:5: 255:6
-    }
-
-    bb3: {
         _4 = const 0_u32;                // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:254:9: 254:19
-        goto -> bb4;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:5: 255:6
+        goto -> bb3;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:253:5: 255:6
     }
 
-    bb4: {
+    bb3: {
         StorageLive(_7);                 // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:9: 256:14
         _8 = discriminant(_3);           // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:257:9: 257:37
-        switchInt(move _8) -> [0_isize: bb6, 1_isize: bb7, 2_isize: bb8, 3_isize: bb8, otherwise: bb5]; // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:257:9: 257:37
+        switchInt(move _8) -> [0_isize: bb5, 1_isize: bb6, 2_isize: bb7, 3_isize: bb7, otherwise: bb4]; // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:257:9: 257:37
     }
 
-    bb5: {
+    bb4: {
         unreachable;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:23: 256:28
     }
 
-    bb6: {
+    bb5: {
         discriminant(_7) = 2;            // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:257:41: 257:53
-        goto -> bb9;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:17: 260:6
+        goto -> bb8;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:17: 260:6
     }
 
-    bb7: {
+    bb6: {
         discriminant(_7) = 3;            // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:258:43: 258:57
-        goto -> bb9;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:17: 260:6
+        goto -> bb8;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:17: 260:6
     }
 
-    bb8: {
+    bb7: {
         discriminant(_7) = 4;            // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:259:72: 259:83
-        goto -> bb9;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:17: 260:6
+        goto -> bb8;                     // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:256:17: 260:6
     }
 
-    bb9: {
+    bb8: {
         StorageLive(_9);                 // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:5: 261:74
         StorageLive(_10);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:5: 261:22
         _10 = &((*_1).20: rustc_codegen_ssa::back::write::SharedEmitter); // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:5: 261:22
         StorageLive(_11);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:40: 261:53
         _11 = _4;                        // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:40: 261:46
         StorageLive(_12);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:55: 261:58
         _12 = move _2;                   // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:55: 261:58
         StorageLive(_13);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:60: 261:65
         _13 = _7;                        // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:60: 261:65
         StorageLive(_14);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:67: 261:73
         _14 = move _5;                   // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:67: 261:73
-        _9 = SharedEmitter::inline_asm_error(move _10, move _11, move _12, move _13, move _14) -> [return: bb10, unwind: bb1]; // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:5: 261:74
+        _9 = SharedEmitter::inline_asm_error(move _10, move _11, move _12, move _13, move _14) -> [return: bb9, unwind: bb1]; // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:5: 261:74
                                          // mir::Constant
                                          // + span: compiler/rustc_codegen_llvm/src/back/write.rs:261:23: 261:39
                                          // + literal: Const { ty: for<'r> fn(&'r rustc_codegen_ssa::back::write::SharedEmitter, u32, std::string::String, rustc_errors::Level, std::option::Option<(std::string::String, std::vec::Vec<rustc_span::InnerSpan>)>) {rustc_codegen_ssa::back::write::SharedEmitter::inline_asm_error}, val: Value(Scalar(<ZST>)) }
     }
 
-    bb10: {
+    bb9: {
         StorageDead(_14);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:73: 261:74
         StorageDead(_13);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:73: 261:74
         StorageDead(_12);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:73: 261:74
         StorageDead(_11);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:73: 261:74
         StorageDead(_10);                // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:73: 261:74
         StorageDead(_9);                 // scope 1 at compiler/rustc_codegen_llvm/src/back/write.rs:261:74: 261:75
         _0 = const ();                   // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:249:3: 262:2
         StorageDead(_7);                 // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:262:1: 262:2
         return;                          // scope 0 at compiler/rustc_codegen_llvm/src/back/write.rs:262:2: 262:2
     }

compiler/rustc_mir/src/transform/const_goto.rs

simonvandel · 2020-10-03T20:43:53Z

The SimplifyBranchSame seems to be the optimization responsible, in particular the use of zip without checking that the number of statements is the same looks very suspicious:

Yeah, that does look sketchy. I'll open a separate PR for that.

simonvandel · 2020-10-03T23:13:32Z

CI is green now with a miscompile fix for SimplifyBranchSame included.

tmiasko · 2020-10-04T18:55:57Z

@Mark-Simulacrum I would suggest backporting fix to SimplifyBranchSame from this PR to beta.

Mark-Simulacrum · 2020-10-04T20:29:33Z

I think this is just a new optimization? IIRC we recently disabled some passes on beta, I would rather not backport mir opt changes, rather disabling them if they're buggy.

tmiasko · 2020-10-04T20:35:48Z

This is a bug in an existing optimization, affecting beta but not stable. Fix is in 2da1c13. Disabling the optimization altogether also sounds fine to me.

Mark-Simulacrum · 2020-10-04T20:45:23Z

Could you extract that to a separate PR so we can land it (more) quickly? cc @rust-lang/wg-mir-opt -- it sounds like there's potentially another soundness bug with the SimplifyBranchSame pass? I've not looked at the test in detail though

bors · 2020-10-04T21:02:14Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

tmiasko · 2020-10-04T21:22:57Z

I opened #77549.

…li-obk Fix miscompile in SimplifyBranchSame Cherry-picked from rust-lang#77486, but with a different test case that used to be compiled incorrectly on both master & beta branches.

simonvandel · 2020-10-11T16:44:49Z

Rebased on master. Ready for review/merge.

As the test shows, this seems to fix #77355. However, running RUSTC_LOG=rustc_mir::transform::const_goto ./x.py build --stage 2 > stage2.log yields very few instances of "SUCCESS:", which was a bit surprising to me.

oli-obk · 2020-10-20T10:03:45Z

compiler/rustc_mir/src/transform/const_goto.rs

+        // if we applied optimizations, we potentially have some cfg to cleanup to
+        // make it easier for further passes
+        if should_simplify {
+            simplify_cfg(body);


cc @rust-lang/wg-mir-opt we need to figure out a way to dump the mir of an optimizations both before its internal cleanup and after. The test diffs are very noisy if we do the cleanup together with the optimization.

Maybe we could not do these immediate cleanups, and instead leave a flag in the mir body that states that it needs a simplification and the following cleanup optimizations just skip if the flag is not set?

src/test/mir-opt/not_equal_false.opt.InstCombine.diff

compiler/rustc_mir/src/transform/const_goto.rs

The previous test's diff was changed by the const_goto pass such that the Ne(_1, false) pattern did not show anymore

simonvandel · 2020-10-25T19:21:45Z

Rebased and resolved review comments.
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

bors · 2020-10-27T01:53:34Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

camelid · 2020-11-20T03:59:22Z

@simonvandel Could you resolve the conflicts or would you like a new review first?

simonvandel · 2020-11-20T06:04:17Z

@camelid it probably makes most sense to fix the conflicts first. I haven't looked at this for a while, so I'll need to see what actually remains to do.

JohnCSimon · 2020-12-07T07:34:19Z

ping from triage - @simonvandel can you post your status on this PR and fix the merge conflicts?

Dylan-DPC-zz · 2020-12-08T09:51:30Z

@simonvandel thanks for taking the time to contribute. I have to close this due to inactivity. If you wish and you have the time you can open a new PR with these changes and we'll take it from there. Thanks

simonvandel · 2020-12-29T14:04:29Z

Hi @Dylan-DPC
I fixed the merge conflicts and the code on my branch is ready for feedback again. Can this PR be reopened or should I create a new PR? Reusing the PR could make sense to continue discussions etc.

bjorn3 · 2020-12-29T14:10:49Z

You can re-open it, but you must first force-push the commit at which the branch was when you closed it. This is a github limitation.

Dylan-DPC-zz · 2020-12-29T14:27:21Z

@simonvandel better to create a new pr

New mir-opt pass to simplify gotos with const values (reopening rust-lang#77486) Reopening PR rust-lang#77486 Fixes rust-lang#77355 This pass optimizes the following sequence ```rust bb2: { _2 = const true; goto -> bb3; } bb3: { switchInt(_2) -> [false: bb4, otherwise: bb5]; } ``` into ```rust bb2: { _2 = const true; goto -> bb5; } ```

rust-highfive assigned lcnr Oct 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 3, 2020

rust-highfive assigned oli-obk and unassigned lcnr Oct 3, 2020

tmiasko reviewed Oct 3, 2020

View reviewed changes