From e82ec2315e5adb1c291c3702cd2ac1f46ecd0fcf Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 3 Mar 2020 11:26:51 -0800 Subject: [PATCH 1/2] Use correct place for `enum_place` PR #69562, which fixed a bug that was causing clippy to ICE, passed the place for the *result* of `Rvalue::Discriminant` instead of the *operand* to `apply_discriminant_switch_effect`. As a result, no effect was applied at all, and we lost the perf benefits from marking inactive enum variants as uninitialized. --- src/librustc_mir/dataflow/generic/engine.rs | 48 ++++++++++++--------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/librustc_mir/dataflow/generic/engine.rs b/src/librustc_mir/dataflow/generic/engine.rs index 1487129f6c77c..8d800f2d0ba05 100644 --- a/src/librustc_mir/dataflow/generic/engine.rs +++ b/src/librustc_mir/dataflow/generic/engine.rs @@ -239,23 +239,26 @@ where } SwitchInt { ref targets, ref values, ref discr, .. } => { - // If this is a switch on an enum discriminant, a custom effect may be applied - // along each outgoing edge. - if let Some(place) = discr.place() { - let enum_def = switch_on_enum_discriminant(self.tcx, self.body, bb_data, place); - if let Some(enum_def) = enum_def { + let Engine { tcx, body, .. } = *self; + let enum_ = discr + .place() + .and_then(|discr| switch_on_enum_discriminant(tcx, body, bb_data, discr)); + match enum_ { + // If this is a switch on an enum discriminant, a custom effect may be applied + // along each outgoing edge. + Some((enum_place, enum_def)) => { self.propagate_bits_into_enum_discriminant_switch_successors( - in_out, bb, enum_def, place, dirty_list, &*values, &*targets, + in_out, bb, enum_def, enum_place, dirty_list, &*values, &*targets, ); - - return; } - } - // Otherwise, it's just a normal `SwitchInt`, and every successor sees the same - // exit state. - for target in targets.iter().copied() { - self.propagate_bits_into_entry_set_for(&in_out, target, dirty_list); + // Otherwise, it's just a normal `SwitchInt`, and every successor sees the same + // exit state. + None => { + for target in targets.iter().copied() { + self.propagate_bits_into_entry_set_for(&in_out, target, dirty_list); + } + } } } @@ -342,22 +345,27 @@ where } } -/// Look at the last statement of a block that ends with to see if it is an assignment of an enum -/// discriminant to the local that determines the target of a `SwitchInt` like so: -/// _42 = discriminant(..) +/// Inspect a `SwitchInt`-terminated basic block to see if the condition of that `SwitchInt` is +/// an enum discriminant. +/// +/// We expect such blocks to have a call to `discriminant` as their last statement like so: +/// _42 = discriminant(_1) /// SwitchInt(_42, ..) +/// +/// If the basic block matches this pattern, this function returns the place corresponding to the +/// enum (`_1` in the example above) as well as the `AdtDef` of that enum. fn switch_on_enum_discriminant( tcx: TyCtxt<'tcx>, - body: &mir::Body<'tcx>, - block: &mir::BasicBlockData<'tcx>, + body: &'mir mir::Body<'tcx>, + block: &'mir mir::BasicBlockData<'tcx>, switch_on: &mir::Place<'tcx>, -) -> Option<&'tcx ty::AdtDef> { +) -> Option<(&'mir mir::Place<'tcx>, &'tcx ty::AdtDef)> { match block.statements.last().map(|stmt| &stmt.kind) { Some(mir::StatementKind::Assign(box (lhs, mir::Rvalue::Discriminant(discriminated)))) if lhs == switch_on => { match &discriminated.ty(body, tcx).ty.kind { - ty::Adt(def, _) => Some(def), + ty::Adt(def, _) => Some((discriminated, def)), // `Rvalue::Discriminant` is also used to get the active yield point for a // generator, but we do not need edge-specific effects in that case. This may From 8d325e6339bf4de8152dc53554c5e458063cb222 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 3 Mar 2020 12:05:59 -0800 Subject: [PATCH 2/2] Add `mir-opt` test for better drop elaboration --- .../mir-opt/no-drop-for-inactive-variant.rs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 src/test/mir-opt/no-drop-for-inactive-variant.rs diff --git a/src/test/mir-opt/no-drop-for-inactive-variant.rs b/src/test/mir-opt/no-drop-for-inactive-variant.rs new file mode 100644 index 0000000000000..cccfa2e1d41af --- /dev/null +++ b/src/test/mir-opt/no-drop-for-inactive-variant.rs @@ -0,0 +1,41 @@ +// Ensure that there are no drop terminators in `unwrap` (except the one along the cleanup +// path). + +fn unwrap(opt: Option) -> T { + match opt { + Some(x) => x, + None => panic!(), + } +} + +fn main() { + let _ = unwrap(Some(1i32)); +} + +// END RUST SOURCE +// START rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir +// fn unwrap(_1: std::option::Option) -> T { +// ... +// bb0: { +// ... +// switchInt(move _2) -> [0isize: bb2, 1isize: bb4, otherwise: bb3]; +// } +// bb1 (cleanup): { +// resume; +// } +// bb2: { +// ... +// const std::rt::begin_panic::<&'static str>(const "explicit panic") -> bb5; +// } +// bb3: { +// unreachable; +// } +// bb4: { +// ... +// return; +// } +// bb5 (cleanup): { +// drop(_1) -> bb1; +// } +// } +// END rustc.unwrap.SimplifyCfg-elaborate-drops.after.mir