From b658050846f5ebb81f862934e130a1fa17927fe0 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Wed, 19 Apr 2023 20:42:18 -0400 Subject: [PATCH 1/2] Demonstrate the bug --- ...nstcombine_duplicate_switch_targets_e2e.rs | 16 +++++++++++ ...h_targets_e2e.ub_if_b.PreCodegen.after.mir | 27 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs create mode 100644 tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir diff --git a/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs b/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs new file mode 100644 index 0000000000000..09779d789e535 --- /dev/null +++ b/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs @@ -0,0 +1,16 @@ +// compile-flags: -Zmir-opt-level=2 -Zinline-mir +// ignore-debug: standard library debug assertions add a panic that breaks this optimization +#![crate_type = "lib"] + +pub enum Thing { + A, + B, +} + +// EMIT_MIR instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir +pub unsafe fn ub_if_b(t: Thing) -> Thing { + match t { + Thing::A => t, + Thing::B => std::hint::unreachable_unchecked(), + } +} diff --git a/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir b/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir new file mode 100644 index 0000000000000..ac60c0381c26a --- /dev/null +++ b/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir @@ -0,0 +1,27 @@ +// MIR for `ub_if_b` after PreCodegen + +fn ub_if_b(_1: Thing) -> Thing { + debug t => _1; // in scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+0:23: +0:24 + let mut _0: Thing; // return place in scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+0:36: +0:41 + let mut _2: isize; // in scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+2:9: +2:17 + scope 1 (inlined unreachable_unchecked) { // at $DIR/instcombine_duplicate_switch_targets_e2e.rs:14:21: 14:55 + scope 2 { + scope 3 (inlined unreachable_unchecked::runtime) { // at $SRC_DIR/core/src/intrinsics.rs:LL:COL + } + } + } + + bb0: { + _2 = discriminant(_1); // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:11: +1:12 + switchInt(move _2) -> [0: bb2, 1: bb1, otherwise: bb1]; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:5: +1:12 + } + + bb1: { + unreachable; // scope 2 at $SRC_DIR/core/src/intrinsics.rs:LL:COL + } + + bb2: { + _0 = move _1; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+2:21: +2:22 + return; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+5:2: +5:2 + } +} From 8ec49ad19a0c77c438c9d3092a6ecf52d6f2cab6 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Wed, 19 Apr 2023 20:52:49 -0400 Subject: [PATCH 2/2] Run combine_duplicate_switch_targets after the simplification that produces them --- .../rustc_mir_transform/src/instcombine.rs | 21 +++---------------- compiler/rustc_mir_transform/src/simplify.rs | 14 +++++++++++++ ...rap_unchecked.unwrap_unchecked.Inline.diff | 2 +- ...h_targets_e2e.ub_if_b.PreCodegen.after.mir | 2 +- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_mir_transform/src/instcombine.rs b/compiler/rustc_mir_transform/src/instcombine.rs index 3d06a0a495f79..432852a1fddc6 100644 --- a/compiler/rustc_mir_transform/src/instcombine.rs +++ b/compiler/rustc_mir_transform/src/instcombine.rs @@ -1,11 +1,9 @@ //! Performs various peephole optimizations. +use crate::simplify::combine_duplicate_switch_targets; use crate::MirPass; use rustc_hir::Mutability; -use rustc_middle::mir::{ - BinOp, Body, CastKind, Constant, ConstantKind, LocalDecls, Operand, Place, ProjectionElem, - Rvalue, SourceInfo, Statement, StatementKind, SwitchTargets, Terminator, TerminatorKind, UnOp, -}; +use rustc_middle::mir::*; use rustc_middle::ty::layout::ValidityRequirement; use rustc_middle::ty::util::IntTypeExt; use rustc_middle::ty::{self, ParamEnv, SubstsRef, Ty, TyCtxt}; @@ -46,7 +44,7 @@ impl<'tcx> MirPass<'tcx> for InstCombine { &mut block.terminator.as_mut().unwrap(), &mut block.statements, ); - ctx.combine_duplicate_switch_targets(&mut block.terminator.as_mut().unwrap()); + combine_duplicate_switch_targets(block.terminator.as_mut().unwrap()); } } } @@ -264,19 +262,6 @@ impl<'tcx> InstCombineContext<'tcx, '_> { terminator.kind = TerminatorKind::Goto { target: destination_block }; } - fn combine_duplicate_switch_targets(&self, terminator: &mut Terminator<'tcx>) { - let TerminatorKind::SwitchInt { targets, .. } = &mut terminator.kind - else { return }; - - let otherwise = targets.otherwise(); - if targets.iter().any(|t| t.1 == otherwise) { - *targets = SwitchTargets::new( - targets.iter().filter(|t| t.1 != otherwise), - targets.otherwise(), - ); - } - } - fn combine_intrinsic_assert( &self, terminator: &mut Terminator<'tcx>, diff --git a/compiler/rustc_mir_transform/src/simplify.rs b/compiler/rustc_mir_transform/src/simplify.rs index 88574addaa091..e1ca7c107b9d2 100644 --- a/compiler/rustc_mir_transform/src/simplify.rs +++ b/compiler/rustc_mir_transform/src/simplify.rs @@ -278,6 +278,18 @@ impl<'a, 'tcx> CfgSimplifier<'a, 'tcx> { } } +pub fn combine_duplicate_switch_targets(terminator: &mut Terminator<'_>) { + if let TerminatorKind::SwitchInt { targets, .. } = &mut terminator.kind { + let otherwise = targets.otherwise(); + if targets.iter().any(|t| t.1 == otherwise) { + *targets = SwitchTargets::new( + targets.iter().filter(|t| t.1 != otherwise), + targets.otherwise(), + ); + } + } +} + pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { struct OptApplier<'tcx> { tcx: TyCtxt<'tcx>, @@ -298,6 +310,8 @@ pub fn remove_duplicate_unreachable_blocks<'tcx>(tcx: TyCtxt<'tcx>, body: &mut B } } + combine_duplicate_switch_targets(terminator); + self.super_terminator(terminator, location); } } diff --git a/tests/mir-opt/inline/unwrap_unchecked.unwrap_unchecked.Inline.diff b/tests/mir-opt/inline/unwrap_unchecked.unwrap_unchecked.Inline.diff index 543ddcfc44c1d..8a8cd896e85db 100644 --- a/tests/mir-opt/inline/unwrap_unchecked.unwrap_unchecked.Inline.diff +++ b/tests/mir-opt/inline/unwrap_unchecked.unwrap_unchecked.Inline.diff @@ -34,7 +34,7 @@ - // + literal: Const { ty: unsafe fn(Option) -> T {Option::::unwrap_unchecked}, val: Value() } + StorageLive(_3); // scope 0 at $DIR/unwrap_unchecked.rs:+1:9: +1:27 + _4 = discriminant(_2); // scope 1 at $SRC_DIR/core/src/option.rs:LL:COL -+ switchInt(move _4) -> [0: bb1, 1: bb2, otherwise: bb1]; // scope 1 at $SRC_DIR/core/src/option.rs:LL:COL ++ switchInt(move _4) -> [1: bb2, otherwise: bb1]; // scope 1 at $SRC_DIR/core/src/option.rs:LL:COL } bb1: { diff --git a/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir b/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir index ac60c0381c26a..acb7297310fb8 100644 --- a/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir +++ b/tests/mir-opt/instcombine_duplicate_switch_targets_e2e.ub_if_b.PreCodegen.after.mir @@ -13,7 +13,7 @@ fn ub_if_b(_1: Thing) -> Thing { bb0: { _2 = discriminant(_1); // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:11: +1:12 - switchInt(move _2) -> [0: bb2, 1: bb1, otherwise: bb1]; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:5: +1:12 + switchInt(move _2) -> [0: bb2, otherwise: bb1]; // scope 0 at $DIR/instcombine_duplicate_switch_targets_e2e.rs:+1:5: +1:12 } bb1: {