Auto merge of #111752 - dingxiangfei2009:lower-or-pattern, r=cjgillot

Lower `Or` pattern without allocating place

cc `@azizghuloum` `@cjgillot`

Related to #111583 and #111644

While reviewing #111644, it occurs to me that while we directly lower conjunctive predicates, which are connected with `&&`, into the desirable control flow, today we don't directly lower the disjunctive predicates, which are connected with `||`, in the similar fashion. Instead, we allocate a place for the boolean temporary to hold the result of evaluating the `||` expression.

Usually I would expect optimization at later stages to "inline" the evaluation of boolean predicates into simple CFG, but #111583 is an example where `&&` is failing to be optimized away and the assembly shows that both the expensive operands are evaluated. Therefore, I would like to make a small change to make the CFG a bit more straight-forward without invoking the `as_temp` machinery, and plus avoid allocating the place to hold the boolean result as well.

Author: bors

compiler/rustc_mir_build/src/build/expr/into.rs

@@ -159,52 +159,44 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 }
             }
             ExprKind::LogicalOp { op, lhs, rhs } => {
-                // And:
-                //
-                // [block: If(lhs)] -true-> [else_block: dest = (rhs)]
-                //        | (false)
-                //  [shortcircuit_block: dest = false]
-                //
-                // Or:
-                //
-                // [block: If(lhs)] -false-> [else_block: dest = (rhs)]
-                //        | (true)
-                //  [shortcircuit_block: dest = true]
-
-                let (shortcircuit_block, mut else_block, join_block) = (
-                    this.cfg.start_new_block(),
-                    this.cfg.start_new_block(),
-                    this.cfg.start_new_block(),
-                );
-
-                let lhs = unpack!(block = this.as_local_operand(block, &this.thir[lhs]));
-                let blocks = match op {
-                    LogicalOp::And => (else_block, shortcircuit_block),
-                    LogicalOp::Or => (shortcircuit_block, else_block),
+                let condition_scope = this.local_scope();
+                let source_info = this.source_info(expr.span);
+                // We first evaluate the left-hand side of the predicate ...
+                let (then_block, else_block) =
+                    this.in_if_then_scope(condition_scope, expr.span, |this| {
+                        this.then_else_break(
+                            block,
+                            &this.thir[lhs],
+                            Some(condition_scope),
+                            condition_scope,
+                            source_info,
+                        )
+                    });
+                let (short_circuit, continuation, constant) = match op {
+                    LogicalOp::And => (else_block, then_block, false),
+                    LogicalOp::Or => (then_block, else_block, true),
                 };
-                let term = TerminatorKind::if_(lhs, blocks.0, blocks.1);
-                this.cfg.terminate(block, source_info, term);
-
+                // At this point, the control flow splits into a short-circuiting path
+                // and a continuation path.
+                // - If the operator is `&&`, passing `lhs` leads to continuation of evaluation on `rhs`;
+                //   failing it leads to the short-circuting path which assigns `false` to the place.
+                // - If the operator is `||`, failing `lhs` leads to continuation of evaluation on `rhs`;
+                //   passing it leads to the short-circuting path which assigns `true` to the place.
                 this.cfg.push_assign_constant(
-                    shortcircuit_block,
+                    short_circuit,
                     source_info,
                     destination,
                     Constant {
-                        span: expr_span,
+                        span: expr.span,
                         user_ty: None,
-                        literal: match op {
-                            LogicalOp::And => ConstantKind::from_bool(this.tcx, false),
-                            LogicalOp::Or => ConstantKind::from_bool(this.tcx, true),
-                        },
+                        literal: ConstantKind::from_bool(this.tcx, constant),
                     },
                 );
-                this.cfg.goto(shortcircuit_block, source_info, join_block);
-
-                let rhs = unpack!(else_block = this.as_local_operand(else_block, &this.thir[rhs]));
-                this.cfg.push_assign(else_block, source_info, destination, Rvalue::Use(rhs));
-                this.cfg.goto(else_block, source_info, join_block);
-
-                join_block.unit()
+                let rhs = unpack!(this.expr_into_dest(destination, continuation, &this.thir[rhs]));
+                let target = this.cfg.start_new_block();
+                this.cfg.goto(rhs, source_info, target);
+                this.cfg.goto(short_circuit, source_info, target);
+                target.unit()
             }
             ExprKind::Loop { body } => {
                 // [block]

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -64,6 +64,43 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
                 rhs_then_block.unit()
             }
+            ExprKind::LogicalOp { op: LogicalOp::Or, lhs, rhs } => {
+                let local_scope = this.local_scope();
+                let (lhs_success_block, failure_block) =
+                    this.in_if_then_scope(local_scope, expr_span, |this| {
+                        this.then_else_break(
+                            block,
+                            &this.thir[lhs],
+                            temp_scope_override,
+                            local_scope,
+                            variable_source_info,
+                        )
+                    });
+                let rhs_success_block = unpack!(this.then_else_break(
+                    failure_block,
+                    &this.thir[rhs],
+                    temp_scope_override,
+                    break_scope,
+                    variable_source_info,
+                ));
+                this.cfg.goto(lhs_success_block, variable_source_info, rhs_success_block);
+                rhs_success_block.unit()
+            }
+            ExprKind::Unary { op: UnOp::Not, arg } => {
+                let local_scope = this.local_scope();
+                let (success_block, failure_block) =
+                    this.in_if_then_scope(local_scope, expr_span, |this| {
+                        this.then_else_break(
+                            block,
+                            &this.thir[arg],
+                            temp_scope_override,
+                            local_scope,
+                            variable_source_info,
+                        )
+                    });
+                this.break_for_else(success_block, break_scope, variable_source_info);
+                failure_block.unit()
+            }
             ExprKind::Scope { region_scope, lint_level, value } => {
                 let region_scope = (region_scope, this.source_info(expr_span));
                 this.in_scope(region_scope, lint_level, |this| {
@@ -76,6 +113,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     )
                 })
             }
+            ExprKind::Use { source } => this.then_else_break(
+                block,
+                &this.thir[source],
+                temp_scope_override,
+                break_scope,
+                variable_source_info,
+            ),
             ExprKind::Let { expr, ref pat } => this.lower_let_expr(
                 block,
                 &this.thir[expr],

tests/codegen/slice-as_chunks.rs

@@ -22,9 +22,9 @@ pub fn chunks4(x: &[u8]) -> &[[u8; 4]] {
 // CHECK-LABEL: @chunks4_with_remainder
 #[no_mangle]
 pub fn chunks4_with_remainder(x: &[u8]) -> (&[[u8; 4]], &[u8]) {
-    // CHECK: and i64 %x.1, -4
-    // CHECK: and i64 %x.1, 3
-    // CHECK: lshr exact
+    // CHECK-DAG: and i64 %x.1, -4
+    // CHECK-DAG: and i64 %x.1, 3
+    // CHECK-DAG: lshr
     // CHECK-NOT: mul
     // CHECK-NOT: udiv
     // CHECK-NOT: urem

tests/codegen/slice-iter-nonnull.rs

@@ -100,13 +100,13 @@ pub fn slice_iter_is_empty(it: &std::slice::Iter<'_, u32>) -> bool {
 // CHECK-LABEL: @slice_iter_len
 #[no_mangle]
 pub fn slice_iter_len(it: &std::slice::Iter<'_, u32>) -> usize {
-    // CHECK: %[[START:.+]] = load ptr, ptr %it,
-    // CHECK-SAME: !nonnull
-    // CHECK-SAME: !noundef
     // CHECK: %[[ENDP:.+]] = getelementptr{{.+}}ptr %it,{{.+}} 1
     // CHECK: %[[END:.+]] = load ptr, ptr %[[ENDP]]
     // CHECK-SAME: !nonnull
     // CHECK-SAME: !noundef
+    // CHECK: %[[START:.+]] = load ptr, ptr %it,
+    // CHECK-SAME: !nonnull
+    // CHECK-SAME: !noundef
 
     // CHECK: ptrtoint
     // CHECK: ptrtoint

tests/mir-opt/bool_compare.opt1.InstSimplify.diff

@@ -13,16 +13,17 @@
           _3 = _1;
 -         _2 = Ne(move _3, const true);
 +         _2 = Not(move _3);
-          StorageDead(_3);
           switchInt(move _2) -> [0: bb2, otherwise: bb1];
       }
 
       bb1: {
+          StorageDead(_3);
           _0 = const 0_u32;
           goto -> bb3;
       }
 
       bb2: {
+          StorageDead(_3);
           _0 = const 1_u32;
           goto -> bb3;
       }

tests/mir-opt/bool_compare.opt2.InstSimplify.diff

@@ -13,16 +13,17 @@
           _3 = _1;
 -         _2 = Ne(const true, move _3);
 +         _2 = Not(move _3);
-          StorageDead(_3);
           switchInt(move _2) -> [0: bb2, otherwise: bb1];
       }
 
       bb1: {
+          StorageDead(_3);
           _0 = const 0_u32;
           goto -> bb3;
       }
 
       bb2: {
+          StorageDead(_3);
           _0 = const 1_u32;
           goto -> bb3;
       }

tests/mir-opt/bool_compare.opt3.InstSimplify.diff

@@ -13,16 +13,17 @@
           _3 = _1;
 -         _2 = Eq(move _3, const false);
 +         _2 = Not(move _3);
-          StorageDead(_3);
           switchInt(move _2) -> [0: bb2, otherwise: bb1];
       }
 
       bb1: {
+          StorageDead(_3);
           _0 = const 0_u32;
           goto -> bb3;
       }
 
       bb2: {
+          StorageDead(_3);
           _0 = const 1_u32;
           goto -> bb3;
       }

tests/mir-opt/bool_compare.opt4.InstSimplify.diff

@@ -13,16 +13,17 @@
           _3 = _1;
 -         _2 = Eq(const false, move _3);
 +         _2 = Not(move _3);
-          StorageDead(_3);
           switchInt(move _2) -> [0: bb2, otherwise: bb1];
       }
 
       bb1: {
+          StorageDead(_3);
           _0 = const 0_u32;
           goto -> bb3;
       }
 
       bb2: {
+          StorageDead(_3);
           _0 = const 1_u32;
           goto -> bb3;
       }

tests/mir-opt/building/logical_or_in_conditional.rs

@@ -0,0 +1,39 @@
+// compile-flags: -Z validate-mir
+#![feature(let_chains)]
+struct Droppy(u8);
+impl Drop for Droppy {
+    fn drop(&mut self) {
+        println!("drop {}", self.0);
+    }
+}
+
+enum E {
+    A(u8),
+    B,
+}
+
+impl E {
+    fn f() -> Self {
+        Self::A(1)
+    }
+}
+
+fn always_true() -> bool {
+    true
+}
+
+// EMIT_MIR logical_or_in_conditional.test_or.built.after.mir
+fn test_or() {
+    if Droppy(0).0 > 0 || Droppy(1).0 > 1 {}
+}
+
+// EMIT_MIR logical_or_in_conditional.test_complex.built.after.mir
+fn test_complex() {
+    if let E::A(_) = E::f() && ((always_true() && Droppy(0).0 > 0) || Droppy(1).0 > 1) {}
+
+    if !always_true() && let E::B = E::f() {}
+}
+
+fn main() {
+    test_or();
+}