Auto merge of #96098 - JakobDegen:always-return-place, r=oli-obk

Refactor call terminator to always include destination place

In #71117 people seemed to agree that call terminators should always have a destination place, even if the call was guaranteed to diverge. This implements that. Unsurprisingly, the diff touches a lot of code, but thankfully I had to do almost nothing interesting. The only interesting thing came up in const prop, where the stack frame having no return place was also used to indicate that the layout could not be computed (or similar). I replaced this with a ZST allocation, which should continue to do the right things.

cc `@RalfJung` `@eddyb` who were involved in the original conversation

r? rust-lang/mir-opt

compiler/rustc_borrowck/src/constraint_generation.rs

@@ -140,9 +140,7 @@ impl<'cg, 'cx, 'tcx> Visitor<'tcx> for ConstraintGeneration<'cg, 'cx, 'tcx> {
         // A `Call` terminator's return value can be a local which has borrows,
         // so we need to record those as `killed` as well.
         if let TerminatorKind::Call { destination, .. } = terminator.kind {
-            if let Some((place, _)) = destination {
-                self.record_killed_borrows_for_place(place, location);
-            }
+            self.record_killed_borrows_for_place(destination, location);
         }
 
         self.super_terminator(terminator, location);

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -2198,10 +2198,10 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 "annotate_argument_and_return_for_borrow: target={:?} terminator={:?}",
                 target, terminator
             );
-            if let TerminatorKind::Call { destination: Some((place, _)), args, .. } =
+            if let TerminatorKind::Call { destination, target: Some(_), args, .. } =
                 &terminator.kind
             {
-                if let Some(assigned_to) = place.as_local() {
+                if let Some(assigned_to) = destination.as_local() {
                     debug!(
                         "annotate_argument_and_return_for_borrow: assigned_to={:?} args={:?}",
                         assigned_to, args

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

@@ -705,10 +705,10 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 let terminator = block.terminator();
                 debug!("was_captured_by_trait_object: terminator={:?}", terminator);
 
-                if let TerminatorKind::Call { destination: Some((place, block)), args, .. } =
+                if let TerminatorKind::Call { destination, target: Some(block), args, .. } =
                     &terminator.kind
                 {
-                    if let Some(dest) = place.as_local() {
+                    if let Some(dest) = destination.as_local() {
                         debug!(
                             "was_captured_by_trait_object: target={:?} dest={:?} args={:?}",
                             target, dest, args

compiler/rustc_borrowck/src/invalidation.rs

@@ -124,6 +124,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
                 ref func,
                 ref args,
                 destination,
+                target: _,
                 cleanup: _,
                 from_hir_call: _,
                 fn_span: _,
@@ -132,9 +133,7 @@ impl<'cx, 'tcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx> {
                 for arg in args {
                     self.consume_operand(location, arg);
                 }
-                if let Some((dest, _ /*bb*/)) = destination {
-                    self.mutate_place(location, *dest, Deep);
-                }
+                self.mutate_place(location, *destination, Deep);
             }
             TerminatorKind::Assert { ref cond, expected: _, ref msg, target: _, cleanup: _ } => {
                 self.consume_operand(location, cond);

compiler/rustc_borrowck/src/lib.rs

@@ -661,7 +661,8 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
             TerminatorKind::Call {
                 ref func,
                 ref args,
-                ref destination,
+                destination,
+                target: _,
                 cleanup: _,
                 from_hir_call: _,
                 fn_span: _,
@@ -670,9 +671,7 @@ impl<'cx, 'tcx> rustc_mir_dataflow::ResultsVisitor<'cx, 'tcx> for MirBorrowckCtx
                 for arg in args {
                     self.consume_operand(loc, (arg, span), flow_state);
                 }
-                if let Some((dest, _ /*bb*/)) = *destination {
-                    self.mutate_place(loc, (dest, span), Deep, flow_state);
-                }
+                self.mutate_place(loc, (destination, span), Deep, flow_state);
             }
             TerminatorKind::Assert { ref cond, expected: _, ref msg, target: _, cleanup: _ } => {
                 self.consume_operand(loc, (cond, span), flow_state);

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -1403,7 +1403,9 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
                 }
                 // FIXME: check the values
             }
-            TerminatorKind::Call { ref func, ref args, ref destination, from_hir_call, .. } => {
+            TerminatorKind::Call {
+                ref func, ref args, destination, target, from_hir_call, ..
+            } => {
                 self.check_operand(func, term_location);
                 for arg in args {
                     self.check_operand(arg, term_location);
@@ -1424,7 +1426,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
                     sig,
                 );
                 let sig = self.normalize(sig, term_location);
-                self.check_call_dest(body, term, &sig, destination, term_location);
+                self.check_call_dest(body, term, &sig, destination, target, term_location);
 
                 self.prove_predicates(
                     sig.inputs_and_output
@@ -1502,15 +1504,16 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
         body: &Body<'tcx>,
         term: &Terminator<'tcx>,
         sig: &ty::FnSig<'tcx>,
-        destination: &Option<(Place<'tcx>, BasicBlock)>,
+        destination: Place<'tcx>,
+        target: Option<BasicBlock>,
         term_location: Location,
     ) {
         let tcx = self.tcx();
-        match *destination {
-            Some((ref dest, _target_block)) => {
-                let dest_ty = dest.ty(body, tcx).ty;
+        match target {
+            Some(_) => {
+                let dest_ty = destination.ty(body, tcx).ty;
                 let dest_ty = self.normalize(dest_ty, term_location);
-                let category = match dest.as_local() {
+                let category = match destination.as_local() {
                     Some(RETURN_PLACE) => {
                         if let BorrowCheckContext {
                             universal_regions:
@@ -1659,8 +1662,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
                     self.assert_iscleanup(body, block_data, unwind, true);
                 }
             }
-            TerminatorKind::Call { ref destination, cleanup, .. } => {
-                if let &Some((_, target)) = destination {
+            TerminatorKind::Call { ref target, cleanup, .. } => {
+                if let &Some(target) = target {
                     self.assert_iscleanup(body, block_data, target, is_cleanup);
                 }
                 if let Some(cleanup) = cleanup {

compiler/rustc_borrowck/src/used_muts.rs

@@ -66,8 +66,8 @@ impl<'visit, 'cx, 'tcx> Visitor<'tcx> for GatherUsedMutsVisitor<'visit, 'cx, 'tc
     fn visit_terminator(&mut self, terminator: &Terminator<'tcx>, location: Location) {
         debug!("visit_terminator: terminator={:?}", terminator);
         match &terminator.kind {
-            TerminatorKind::Call { destination: Some((into, _)), .. } => {
-                self.remove_never_initialized_mut_locals(*into);
+            TerminatorKind::Call { destination, .. } => {
+                self.remove_never_initialized_mut_locals(*destination);
             }
             TerminatorKind::DropAndReplace { place, .. } => {
                 self.remove_never_initialized_mut_locals(*place);

compiler/rustc_codegen_cranelift/src/abi/mod.rs

@@ -312,13 +312,14 @@ pub(crate) fn codegen_terminator_call<'tcx>(
     source_info: mir::SourceInfo,
     func: &Operand<'tcx>,
     args: &[Operand<'tcx>],
-    mir_dest: Option<(Place<'tcx>, BasicBlock)>,
+    destination: Place<'tcx>,
+    target: Option<BasicBlock>,
 ) {
     let fn_ty = fx.monomorphize(func.ty(fx.mir, fx.tcx));
     let fn_sig =
         fx.tcx.normalize_erasing_late_bound_regions(ParamEnv::reveal_all(), fn_ty.fn_sig(fx.tcx));
 
-    let destination = mir_dest.map(|(place, bb)| (codegen_place(fx, place), bb));
+    let ret_place = codegen_place(fx, destination);
 
     // Handle special calls like instrinsics and empty drop glue.
     let instance = if let ty::FnDef(def_id, substs) = *fn_ty.kind() {
@@ -333,7 +334,8 @@ pub(crate) fn codegen_terminator_call<'tcx>(
                 &fx.tcx.symbol_name(instance).name,
                 substs,
                 args,
-                destination,
+                ret_place,
+                target,
             );
             return;
         }
@@ -344,14 +346,15 @@ pub(crate) fn codegen_terminator_call<'tcx>(
                     fx,
                     instance,
                     args,
-                    destination,
+                    ret_place,
+                    target,
                     source_info,
                 );
                 return;
             }
             InstanceDef::DropGlue(_, None) => {
                 // empty drop glue - a nop.
-                let (_, dest) = destination.expect("Non terminating drop_in_place_real???");
+                let dest = target.expect("Non terminating drop_in_place_real???");
                 let ret_block = fx.get_block(dest);
                 fx.bcx.ins().jump(ret_block, &[]);
                 return;
@@ -377,7 +380,7 @@ pub(crate) fn codegen_terminator_call<'tcx>(
         .unwrap_or(false);
     if is_cold {
         fx.bcx.set_cold_block(fx.bcx.current_block().unwrap());
-        if let Some((_place, destination_block)) = destination {
+        if let Some(destination_block) = target {
             fx.bcx.set_cold_block(fx.get_block(destination_block));
         }
     }
@@ -459,7 +462,6 @@ pub(crate) fn codegen_terminator_call<'tcx>(
         }
     };
 
-    let ret_place = destination.map(|(place, _)| place);
     self::returning::codegen_with_call_return_arg(fx, &fn_abi.ret, ret_place, |fx, return_ptr| {
         let call_args = return_ptr
             .into_iter()
@@ -511,7 +513,7 @@ pub(crate) fn codegen_terminator_call<'tcx>(
         call_inst
     });
 
-    if let Some((_, dest)) = destination {
+    if let Some(dest) = target {
         let ret_block = fx.get_block(dest);
         fx.bcx.ins().jump(ret_block, &[]);
     } else {

compiler/rustc_codegen_cranelift/src/abi/returning.rs

@@ -56,23 +56,22 @@ pub(super) fn codegen_return_param<'tcx>(
 pub(super) fn codegen_with_call_return_arg<'tcx>(
     fx: &mut FunctionCx<'_, '_, 'tcx>,
     ret_arg_abi: &ArgAbi<'tcx, Ty<'tcx>>,
-    ret_place: Option<CPlace<'tcx>>,
+    ret_place: CPlace<'tcx>,
     f: impl FnOnce(&mut FunctionCx<'_, '_, 'tcx>, Option<Value>) -> Inst,
 ) {
     let (ret_temp_place, return_ptr) = match ret_arg_abi.mode {
         PassMode::Ignore => (None, None),
-        PassMode::Indirect { attrs: _, extra_attrs: None, on_stack: _ } => match ret_place {
-            Some(ret_place) if matches!(ret_place.inner(), CPlaceInner::Addr(_, None)) => {
+        PassMode::Indirect { attrs: _, extra_attrs: None, on_stack: _ } => {
+            if matches!(ret_place.inner(), CPlaceInner::Addr(_, None)) {
                 // This is an optimization to prevent unnecessary copies of the return value when
                 // the return place is already a memory place as opposed to a register.
                 // This match arm can be safely removed.
                 (None, Some(ret_place.to_ptr().get_addr(fx)))
-            }
-            _ => {
+            } else {
                 let place = CPlace::new_stack_slot(fx, ret_arg_abi.layout);
                 (Some(place), Some(place.to_ptr().get_addr(fx)))
             }
-        },
+        }
         PassMode::Indirect { attrs: _, extra_attrs: Some(_), on_stack: _ } => {
             unreachable!("unsized return value")
         }
@@ -84,39 +83,25 @@ pub(super) fn codegen_with_call_return_arg<'tcx>(
     match ret_arg_abi.mode {
         PassMode::Ignore => {}
         PassMode::Direct(_) => {
-            if let Some(ret_place) = ret_place {
-                let ret_val = fx.bcx.inst_results(call_inst)[0];
-                ret_place.write_cvalue(fx, CValue::by_val(ret_val, ret_arg_abi.layout));
-            }
+            let ret_val = fx.bcx.inst_results(call_inst)[0];
+            ret_place.write_cvalue(fx, CValue::by_val(ret_val, ret_arg_abi.layout));
         }
         PassMode::Pair(_, _) => {
-            if let Some(ret_place) = ret_place {
-                let ret_val_a = fx.bcx.inst_results(call_inst)[0];
-                let ret_val_b = fx.bcx.inst_results(call_inst)[1];
-                ret_place.write_cvalue(
-                    fx,
-                    CValue::by_val_pair(ret_val_a, ret_val_b, ret_arg_abi.layout),
-                );
-            }
+            let ret_val_a = fx.bcx.inst_results(call_inst)[0];
+            let ret_val_b = fx.bcx.inst_results(call_inst)[1];
+            ret_place
+                .write_cvalue(fx, CValue::by_val_pair(ret_val_a, ret_val_b, ret_arg_abi.layout));
         }
         PassMode::Cast(cast) => {
-            if let Some(ret_place) = ret_place {
-                let results = fx
-                    .bcx
-                    .inst_results(call_inst)
-                    .iter()
-                    .copied()
-                    .collect::<SmallVec<[Value; 2]>>();
-                let result =
-                    super::pass_mode::from_casted_value(fx, &results, ret_place.layout(), cast);
-                ret_place.write_cvalue(fx, result);
-            }
+            let results =
+                fx.bcx.inst_results(call_inst).iter().copied().collect::<SmallVec<[Value; 2]>>();
+            let result =
+                super::pass_mode::from_casted_value(fx, &results, ret_place.layout(), cast);
+            ret_place.write_cvalue(fx, result);
         }
         PassMode::Indirect { attrs: _, extra_attrs: None, on_stack: _ } => {
-            if let (Some(ret_place), Some(ret_temp_place)) = (ret_place, ret_temp_place) {
-                // Both ret_place and ret_temp_place must be Some. If ret_place is None, this is
-                // a non-returning call. If ret_temp_place is None, it is not necessary to copy the
-                // return value.
+            if let Some(ret_temp_place) = ret_temp_place {
+                // If ret_temp_place is None, it is not necessary to copy the return value.
                 let ret_temp_value = ret_temp_place.to_cvalue(fx);
                 ret_place.write_cvalue(fx, ret_temp_value);
             }

compiler/rustc_codegen_cranelift/src/base.rs

@@ -393,6 +393,7 @@ fn codegen_fn_content(fx: &mut FunctionCx<'_, '_, '_>) {
                 func,
                 args,
                 destination,
+                target,
                 fn_span,
                 cleanup: _,
                 from_hir_call: _,
@@ -404,6 +405,7 @@ fn codegen_fn_content(fx: &mut FunctionCx<'_, '_, '_>) {
                         func,
                         args,
                         *destination,
+                        *target,
                     )
                 });
             }

compiler/rustc_codegen_cranelift/src/constant.rs

@@ -542,8 +542,8 @@ pub(crate) fn mir_operand_get_const_val<'tcx>(
                     | TerminatorKind::FalseEdge { .. }
                     | TerminatorKind::FalseUnwind { .. } => unreachable!(),
                     TerminatorKind::InlineAsm { .. } => return None,
-                    TerminatorKind::Call { destination: Some((call_place, _)), .. }
-                        if call_place == place =>
+                    TerminatorKind::Call { destination, target: Some(_), .. }
+                        if destination == place =>
                     {
                         return None;
                     }

compiler/rustc_codegen_cranelift/src/intrinsics/llvm.rs

@@ -10,10 +10,9 @@ pub(crate) fn codegen_llvm_intrinsic_call<'tcx>(
     intrinsic: &str,
     _substs: SubstsRef<'tcx>,
     args: &[mir::Operand<'tcx>],
-    destination: Option<(CPlace<'tcx>, BasicBlock)>,
+    ret: CPlace<'tcx>,
+    target: Option<BasicBlock>,
 ) {
-    let ret = destination.unwrap().0;
-
     intrinsic_match! {
         fx, intrinsic, args,
         _ => {
@@ -126,7 +125,7 @@ pub(crate) fn codegen_llvm_intrinsic_call<'tcx>(
         };
     }
 
-    let dest = destination.expect("all llvm intrinsics used by stdlib should return").1;
+    let dest = target.expect("all llvm intrinsics used by stdlib should return");
     let ret_block = fx.get_block(dest);
     fx.bcx.ins().jump(ret_block, &[]);
 }