Add post-mono passes to make mono-reachable analysis more accurate

Author: saethlin

compiler/rustc_codegen_cranelift/src/base.rs

@@ -41,7 +41,7 @@ pub(crate) fn codegen_fn<'tcx>(
     let symbol_name = tcx.symbol_name(instance).name.to_string();
     let _timer = tcx.prof.generic_activity_with_arg("codegen fn", &*symbol_name);
 
-    let mir = tcx.instance_mir(instance.def);
+    let mir = tcx.codegen_mir(instance);
     let _mir_guard = crate::PrintOnPanic(|| {
         let mut buf = Vec::new();
         with_no_trimmed_paths!({
@@ -302,19 +302,10 @@ fn codegen_fn_body(fx: &mut FunctionCx<'_, '_, '_>, start_block: Block) {
         .generic_activity("codegen prelude")
         .run(|| crate::abi::codegen_fn_prelude(fx, start_block));
 
-    let reachable_blocks = traversal::mono_reachable_as_bitset(fx.mir, fx.tcx, fx.instance);
-
     for (bb, bb_data) in fx.mir.basic_blocks.iter_enumerated() {
         let block = fx.get_block(bb);
         fx.bcx.switch_to_block(block);
 
-        if !reachable_blocks.contains(bb) {
-            // We want to skip this block, because it's not reachable. But we still create
-            // the block so terminators in other blocks can reference it.
-            fx.bcx.ins().trap(TrapCode::user(1 /* unreachable */).unwrap());
-            continue;
-        }
-
         if bb_data.is_cleanup {
             // Unwinding after panicking is not supported
             continue;

compiler/rustc_codegen_ssa/src/base.rs

@@ -416,7 +416,7 @@ pub(crate) fn codegen_instance<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>>(
     // release builds.
     info!("codegen_instance({})", instance);
 
-    mir::codegen_mir::<Bx>(cx, instance);
+    mir::lower_mir::<Bx>(cx, instance);
 }
 
 /// Creates the `main` function which will initialize the rust runtime and call

compiler/rustc_codegen_ssa/src/mir/block.rs

@@ -1252,16 +1252,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         }
     }
 
-    pub(crate) fn codegen_block_as_unreachable(&mut self, bb: mir::BasicBlock) {
-        let llbb = match self.try_llbb(bb) {
-            Some(llbb) => llbb,
-            None => return,
-        };
-        let bx = &mut Bx::build(self.cx, llbb);
-        debug!("codegen_block_as_unreachable({:?})", bb);
-        bx.unreachable();
-    }
-
     fn codegen_terminator(
         &mut self,
         bx: &mut Bx,

compiler/rustc_codegen_ssa/src/mir/mod.rs

@@ -159,15 +159,15 @@ impl<'tcx, V: CodegenObject> LocalRef<'tcx, V> {
 ///////////////////////////////////////////////////////////////////////////
 
 #[instrument(level = "debug", skip(cx))]
-pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
+pub fn lower_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
     cx: &'a Bx::CodegenCx,
     instance: Instance<'tcx>,
 ) {
     assert!(!instance.args.has_infer());
 
     let llfn = cx.get_fn(instance);
 
-    let mir = cx.tcx().instance_mir(instance.def);
+    let mir = cx.tcx().codegen_mir(instance);
 
     let fn_abi = cx.fn_abi_of_instance(instance, ty::List::empty());
     debug!("fn_abi: {:?}", fn_abi);
@@ -222,7 +222,8 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         fx.compute_per_local_var_debug_info(&mut start_bx).unzip();
     fx.per_local_var_debug_info = per_local_var_debug_info;
 
-    let traversal_order = traversal::mono_reachable_reverse_postorder(mir, cx.tcx(), instance);
+    let traversal_order: Vec<_> =
+        traversal::reverse_postorder(mir).map(|(block, _data)| block).collect();
     let memory_locals = analyze::non_ssa_locals(&fx, &traversal_order);
 
     // Allocate variable and temp allocas
@@ -282,20 +283,9 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
     // So drop the builder of `start_llbb` to avoid having two at the same time.
     drop(start_bx);
 
-    let mut unreached_blocks = BitSet::new_filled(mir.basic_blocks.len());
     // Codegen the body of each reachable block using our reverse postorder list.
     for bb in traversal_order {
         fx.codegen_block(bb);
-        unreached_blocks.remove(bb);
-    }
-
-    // FIXME: These empty unreachable blocks are *mostly* a waste. They are occasionally
-    // targets for a SwitchInt terminator, but the reimplementation of the mono-reachable
-    // simplification in SwitchInt lowering sometimes misses cases that
-    // mono_reachable_reverse_postorder manages to figure out.
-    // The solution is to do something like post-mono GVN. But for now we have this hack.
-    for bb in unreached_blocks.iter() {
-        fx.codegen_block_as_unreachable(bb);
     }
 }
 

compiler/rustc_middle/src/mir/basic_blocks.rs

@@ -64,7 +64,7 @@ impl<'tcx> BasicBlocks<'tcx> {
     #[inline]
     pub fn reverse_postorder(&self) -> &[BasicBlock] {
         self.cache.reverse_postorder.get_or_init(|| {
-            let mut rpo: Vec<_> = Postorder::new(&self.basic_blocks, START_BLOCK, ()).collect();
+            let mut rpo: Vec<_> = Postorder::new(&self.basic_blocks, START_BLOCK).collect();
             rpo.reverse();
             rpo
         })

compiler/rustc_middle/src/mir/mod.rs

@@ -39,7 +39,7 @@ use crate::ty::fold::{FallibleTypeFolder, TypeFoldable};
 use crate::ty::print::{FmtPrinter, Printer, pretty_print_const, with_no_trimmed_paths};
 use crate::ty::visit::TypeVisitableExt;
 use crate::ty::{
-    self, AdtDef, GenericArg, GenericArgsRef, Instance, InstanceKind, List, Ty, TyCtxt, TypingMode,
+    self, AdtDef, GenericArg, GenericArgsRef, InstanceKind, List, Ty, TyCtxt, TypingMode,
     UserTypeAnnotationIndex,
 };
 
@@ -617,73 +617,6 @@ impl<'tcx> Body<'tcx> {
         self.injection_phase.is_some()
     }
 
-    /// If this basic block ends with a [`TerminatorKind::SwitchInt`] for which we can evaluate the
-    /// dimscriminant in monomorphization, we return the discriminant bits and the
-    /// [`SwitchTargets`], just so the caller doesn't also have to match on the terminator.
-    fn try_const_mono_switchint<'a>(
-        tcx: TyCtxt<'tcx>,
-        instance: Instance<'tcx>,
-        block: &'a BasicBlockData<'tcx>,
-    ) -> Option<(u128, &'a SwitchTargets)> {
-        // There are two places here we need to evaluate a constant.
-        let eval_mono_const = |constant: &ConstOperand<'tcx>| {
-            let env = ty::ParamEnv::reveal_all();
-            let mono_literal = instance.instantiate_mir_and_normalize_erasing_regions(
-                tcx,
-                env,
-                crate::ty::EarlyBinder::bind(constant.const_),
-            );
-            mono_literal.try_eval_bits(tcx, env)
-        };
-
-        let TerminatorKind::SwitchInt { discr, targets } = &block.terminator().kind else {
-            return None;
-        };
-
-        // If this is a SwitchInt(const _), then we can just evaluate the constant and return.
-        let discr = match discr {
-            Operand::Constant(constant) => {
-                let bits = eval_mono_const(constant)?;
-                return Some((bits, targets));
-            }
-            Operand::Move(place) | Operand::Copy(place) => place,
-        };
-
-        // MIR for `if false` actually looks like this:
-        // _1 = const _
-        // SwitchInt(_1)
-        //
-        // And MIR for if intrinsics::ub_checks() looks like this:
-        // _1 = UbChecks()
-        // SwitchInt(_1)
-        //
-        // So we're going to try to recognize this pattern.
-        //
-        // If we have a SwitchInt on a non-const place, we find the most recent statement that
-        // isn't a storage marker. If that statement is an assignment of a const to our
-        // discriminant place, we evaluate and return the const, as if we've const-propagated it
-        // into the SwitchInt.
-
-        let last_stmt = block.statements.iter().rev().find(|stmt| {
-            !matches!(stmt.kind, StatementKind::StorageDead(_) | StatementKind::StorageLive(_))
-        })?;
-
-        let (place, rvalue) = last_stmt.kind.as_assign()?;
-
-        if discr != place {
-            return None;
-        }
-
-        match rvalue {
-            Rvalue::NullaryOp(NullOp::UbChecks, _) => Some((tcx.sess.ub_checks() as u128, targets)),
-            Rvalue::Use(Operand::Constant(constant)) => {
-                let bits = eval_mono_const(constant)?;
-                Some((bits, targets))
-            }
-            _ => None,
-        }
-    }
-
     /// For a `Location` in this scope, determine what the "caller location" at that point is. This
     /// is interesting because of inlining: the `#[track_caller]` attribute of inlined functions
     /// must be honored. Falls back to the `tracked_caller` value for `#[track_caller]` functions,
@@ -1433,19 +1366,6 @@ impl<'tcx> BasicBlockData<'tcx> {
     pub fn is_empty_unreachable(&self) -> bool {
         self.statements.is_empty() && matches!(self.terminator().kind, TerminatorKind::Unreachable)
     }
-
-    /// Like [`Terminator::successors`] but tries to use information available from the [`Instance`]
-    /// to skip successors like the `false` side of an `if const {`.
-    ///
-    /// This is used to implement [`traversal::mono_reachable`] and
-    /// [`traversal::mono_reachable_reverse_postorder`].
-    pub fn mono_successors(&self, tcx: TyCtxt<'tcx>, instance: Instance<'tcx>) -> Successors<'_> {
-        if let Some((bits, targets)) = Body::try_const_mono_switchint(tcx, instance, self) {
-            targets.successors_for_value(bits)
-        } else {
-            self.terminator().successors()
-        }
-    }
 }
 
 ///////////////////////////////////////////////////////////////////////////

compiler/rustc_middle/src/mir/traversal.rs

@@ -104,29 +104,23 @@ impl<'a, 'tcx> Iterator for Preorder<'a, 'tcx> {
 /// ```
 ///
 /// A Postorder traversal of this graph is `D B C A` or `D C B A`
-pub struct Postorder<'a, 'tcx, C> {
+pub struct Postorder<'a, 'tcx> {
     basic_blocks: &'a IndexSlice<BasicBlock, BasicBlockData<'tcx>>,
     visited: BitSet<BasicBlock>,
     visit_stack: Vec<(BasicBlock, Successors<'a>)>,
     root_is_start_block: bool,
-    extra: C,
 }
 
-impl<'a, 'tcx, C> Postorder<'a, 'tcx, C>
-where
-    C: Customization<'tcx>,
-{
+impl<'a, 'tcx> Postorder<'a, 'tcx> {
     pub fn new(
         basic_blocks: &'a IndexSlice<BasicBlock, BasicBlockData<'tcx>>,
         root: BasicBlock,
-        extra: C,
-    ) -> Postorder<'a, 'tcx, C> {
+    ) -> Postorder<'a, 'tcx> {
         let mut po = Postorder {
             basic_blocks,
             visited: BitSet::new_empty(basic_blocks.len()),
             visit_stack: Vec::new(),
             root_is_start_block: root == START_BLOCK,
-            extra,
         };
 
         po.visit(root);
@@ -140,7 +134,7 @@ where
             return;
         }
         let data = &self.basic_blocks[bb];
-        let successors = C::successors(data, self.extra);
+        let successors = data.terminator().successors();
         self.visit_stack.push((bb, successors));
     }
 
@@ -198,10 +192,7 @@ where
     }
 }
 
-impl<'tcx, C> Iterator for Postorder<'_, 'tcx, C>
-where
-    C: Customization<'tcx>,
-{
+impl<'tcx> Iterator for Postorder<'_, 'tcx> {
     type Item = BasicBlock;
 
     fn next(&mut self) -> Option<BasicBlock> {
@@ -252,29 +243,6 @@ impl<'tcx> Customization<'tcx> for () {
     }
 }
 
-impl<'tcx> Customization<'tcx> for (TyCtxt<'tcx>, Instance<'tcx>) {
-    fn successors<'a>(
-        data: &'a BasicBlockData<'tcx>,
-        (tcx, instance): (TyCtxt<'tcx>, Instance<'tcx>),
-    ) -> Successors<'a> {
-        data.mono_successors(tcx, instance)
-    }
-}
-
-pub fn mono_reachable_reverse_postorder<'a, 'tcx>(
-    body: &'a Body<'tcx>,
-    tcx: TyCtxt<'tcx>,
-    instance: Instance<'tcx>,
-) -> Vec<BasicBlock> {
-    let mut iter = Postorder::new(&body.basic_blocks, START_BLOCK, (tcx, instance));
-    let mut items = Vec::with_capacity(body.basic_blocks.len());
-    while let Some(block) = iter.next() {
-        items.push(block);
-    }
-    items.reverse();
-    items
-}
-
 /// Returns an iterator over all basic blocks reachable from the `START_BLOCK` in no particular
 /// order.
 ///
@@ -322,91 +290,3 @@ pub fn reverse_postorder<'a, 'tcx>(
 {
     body.basic_blocks.reverse_postorder().iter().map(|&bb| (bb, &body.basic_blocks[bb]))
 }
-
-/// Traversal of a [`Body`] that tries to avoid unreachable blocks in a monomorphized [`Instance`].
-///
-/// This is allowed to have false positives; blocks may be visited even if they are not actually
-/// reachable.
-///
-/// Such a traversal is mostly useful because it lets us skip lowering the `false` side
-/// of `if <T as Trait>::CONST`, as well as [`NullOp::UbChecks`].
-///
-/// [`NullOp::UbChecks`]: rustc_middle::mir::NullOp::UbChecks
-pub fn mono_reachable<'a, 'tcx>(
-    body: &'a Body<'tcx>,
-    tcx: TyCtxt<'tcx>,
-    instance: Instance<'tcx>,
-) -> MonoReachable<'a, 'tcx> {
-    MonoReachable::new(body, tcx, instance)
-}
-
-/// [`MonoReachable`] internally accumulates a [`BitSet`] of visited blocks. This is just a
-/// convenience function to run that traversal then extract its set of reached blocks.
-pub fn mono_reachable_as_bitset<'a, 'tcx>(
-    body: &'a Body<'tcx>,
-    tcx: TyCtxt<'tcx>,
-    instance: Instance<'tcx>,
-) -> BitSet<BasicBlock> {
-    let mut iter = mono_reachable(body, tcx, instance);
-    while let Some(_) = iter.next() {}
-    iter.visited
-}
-
-pub struct MonoReachable<'a, 'tcx> {
-    body: &'a Body<'tcx>,
-    tcx: TyCtxt<'tcx>,
-    instance: Instance<'tcx>,
-    visited: BitSet<BasicBlock>,
-    // Other traversers track their worklist in a Vec. But we don't care about order, so we can
-    // store ours in a BitSet and thus save allocations because BitSet has a small size
-    // optimization.
-    worklist: BitSet<BasicBlock>,
-}
-
-impl<'a, 'tcx> MonoReachable<'a, 'tcx> {
-    pub fn new(
-        body: &'a Body<'tcx>,
-        tcx: TyCtxt<'tcx>,
-        instance: Instance<'tcx>,
-    ) -> MonoReachable<'a, 'tcx> {
-        let mut worklist = BitSet::new_empty(body.basic_blocks.len());
-        worklist.insert(START_BLOCK);
-        MonoReachable {
-            body,
-            tcx,
-            instance,
-            visited: BitSet::new_empty(body.basic_blocks.len()),
-            worklist,
-        }
-    }
-
-    fn add_work(&mut self, blocks: impl IntoIterator<Item = BasicBlock>) {
-        for block in blocks.into_iter() {
-            if !self.visited.contains(block) {
-                self.worklist.insert(block);
-            }
-        }
-    }
-}
-
-impl<'a, 'tcx> Iterator for MonoReachable<'a, 'tcx> {
-    type Item = (BasicBlock, &'a BasicBlockData<'tcx>);
-
-    fn next(&mut self) -> Option<(BasicBlock, &'a BasicBlockData<'tcx>)> {
-        while let Some(idx) = self.worklist.iter().next() {
-            self.worklist.remove(idx);
-            if !self.visited.insert(idx) {
-                continue;
-            }
-
-            let data = &self.body[idx];
-
-            let targets = data.mono_successors(self.tcx, self.instance);
-            self.add_work(targets);
-
-            return Some((idx, data));
-        }
-
-        None
-    }
-}