Skip to content

Commit

Permalink
Rollup merge of #91410 - ecstatic-morse:const-precise-live-drops-take…
Browse files Browse the repository at this point in the history
…-2, r=oli-obk

Move `#![feature(const_precise_live_drops)]` checks earlier in the pipeline

Should mitigate the issues found during MCP on #73255.

Once this is done, we should clean up the queries a bit, since I think `mir_drops_elaborated_and_const_checked` can be merged back into `mir_promoted`.

Fixes #90770.

cc ``@rust-lang/wg-const-eval``
r? ``@nikomatsakis`` (since they reviewed #71824)
  • Loading branch information
matthiaskrgr committed Dec 2, 2021
2 parents b269213 + 37fa925 commit 3964131
Show file tree
Hide file tree
Showing 15 changed files with 275 additions and 32 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ impl Visitor<'tcx> for CheckLiveDrops<'mir, 'tcx> {
trace!("visit_terminator: terminator={:?} location={:?}", terminator, location);

match &terminator.kind {
mir::TerminatorKind::Drop { place: dropped_place, .. } => {
mir::TerminatorKind::Drop { place: dropped_place, .. }
| mir::TerminatorKind::DropAndReplace { place: dropped_place, .. } => {
let dropped_ty = dropped_place.ty(self.body, self.tcx).ty;
if !NeedsNonConstDrop::in_any_value_of_ty(self.ccx, dropped_ty) {
// Instead of throwing a bug, we just return here. This is because we have to
Expand All @@ -104,11 +105,6 @@ impl Visitor<'tcx> for CheckLiveDrops<'mir, 'tcx> {
}
}

mir::TerminatorKind::DropAndReplace { .. } => span_bug!(
terminator.source_info.span,
"`DropAndReplace` should be removed by drop elaboration",
),

mir::TerminatorKind::Abort
| mir::TerminatorKind::Call { .. }
| mir::TerminatorKind::Assert { .. }
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1803,6 +1803,16 @@ impl<V, T> ProjectionElem<V, T> {
| Self::Downcast(_, _) => false,
}
}

/// Returns `true` if this is a `Downcast` projection with the given `VariantIdx`.
pub fn is_downcast_to(&self, v: VariantIdx) -> bool {
matches!(*self, Self::Downcast(_, x) if x == v)
}

/// Returns `true` if this is a `Field` projection with the given index.
pub fn is_field_to(&self, f: Field) -> bool {
matches!(*self, Self::Field(x, _) if x == f)
}
}

/// Alias for projections as they appear in places, where the base is a place
Expand Down
24 changes: 19 additions & 5 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ mod match_branches;
mod multiple_return_terminators;
mod normalize_array_len;
mod nrvo;
mod remove_false_edges;
mod remove_noop_landing_pads;
mod remove_storage_markers;
mod remove_uninit_drops;
mod remove_unneeded_drops;
mod remove_zsts;
mod required_consts;
Expand All @@ -75,7 +77,7 @@ mod simplify_try;
mod uninhabited_enum_branching;
mod unreachable_prop;

use rustc_const_eval::transform::check_consts;
use rustc_const_eval::transform::check_consts::{self, ConstCx};
use rustc_const_eval::transform::promote_consts;
use rustc_const_eval::transform::validate;
use rustc_mir_dataflow::rustc_peek;
Expand Down Expand Up @@ -444,8 +446,20 @@ fn mir_drops_elaborated_and_const_checked<'tcx>(
let (body, _) = tcx.mir_promoted(def);
let mut body = body.steal();

// IMPORTANT
remove_false_edges::RemoveFalseEdges.run_pass(tcx, &mut body);

// Do a little drop elaboration before const-checking if `const_precise_live_drops` is enabled.
//
// FIXME: Can't use `run_passes` for these, since `run_passes` SILENTLY DOES NOTHING IF THE MIR
// PHASE DOESN'T CHANGE.
if check_consts::post_drop_elaboration::checking_enabled(&ConstCx::new(tcx, &body)) {
simplify::SimplifyCfg::new("remove-false-edges").run_pass(tcx, &mut body);
remove_uninit_drops::RemoveUninitDrops.run_pass(tcx, &mut body);
check_consts::post_drop_elaboration::check_live_drops(tcx, &body);
}

run_post_borrowck_cleanup_passes(tcx, &mut body);
check_consts::post_drop_elaboration::check_live_drops(tcx, &body);
tcx.alloc_steal_mir(body)
}

Expand All @@ -455,7 +469,7 @@ fn run_post_borrowck_cleanup_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tc

let post_borrowck_cleanup: &[&dyn MirPass<'tcx>] = &[
// Remove all things only needed by analysis
&simplify_branches::SimplifyBranches::new("initial"),
&simplify_branches::SimplifyConstCondition::new("initial"),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&cleanup_post_borrowck::CleanupNonCodegenStatements,
&simplify::SimplifyCfg::new("early-opt"),
Expand Down Expand Up @@ -514,13 +528,13 @@ fn run_optimization_passes<'tcx>(tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
&instcombine::InstCombine,
&separate_const_switch::SeparateConstSwitch,
&const_prop::ConstProp,
&simplify_branches::SimplifyBranches::new("after-const-prop"),
&simplify_branches::SimplifyConstCondition::new("after-const-prop"),
&early_otherwise_branch::EarlyOtherwiseBranch,
&simplify_comparison_integral::SimplifyComparisonIntegral,
&simplify_try::SimplifyArmIdentity,
&simplify_try::SimplifyBranchSame,
&dest_prop::DestinationPropagation,
&simplify_branches::SimplifyBranches::new("final"),
&simplify_branches::SimplifyConstCondition::new("final"),
&remove_noop_landing_pads::RemoveNoopLandingPads,
&simplify::SimplifyCfg::new("final"),
&nrvo::RenameReturnPlace,
Expand Down
29 changes: 29 additions & 0 deletions compiler/rustc_mir_transform/src/remove_false_edges.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
use rustc_middle::mir::{Body, TerminatorKind};
use rustc_middle::ty::TyCtxt;

use crate::MirPass;

/// Removes `FalseEdge` and `FalseUnwind` terminators from the MIR.
///
/// These are only needed for borrow checking, and can be removed afterwards.
///
/// FIXME: This should probably have its own MIR phase.
pub struct RemoveFalseEdges;

impl<'tcx> MirPass<'tcx> for RemoveFalseEdges {
fn run_pass(&self, _: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
for block in body.basic_blocks_mut() {
let terminator = block.terminator_mut();
terminator.kind = match terminator.kind {
TerminatorKind::FalseEdge { real_target, .. } => {
TerminatorKind::Goto { target: real_target }
}
TerminatorKind::FalseUnwind { real_target, .. } => {
TerminatorKind::Goto { target: real_target }
}

_ => continue,
}
}
}
}
171 changes: 171 additions & 0 deletions compiler/rustc_mir_transform/src/remove_uninit_drops.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
use rustc_index::bit_set::BitSet;
use rustc_middle::mir::{Body, Field, Rvalue, Statement, StatementKind, TerminatorKind};
use rustc_middle::ty::subst::SubstsRef;
use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt, VariantDef};
use rustc_mir_dataflow::impls::MaybeInitializedPlaces;
use rustc_mir_dataflow::move_paths::{LookupResult, MoveData, MovePathIndex};
use rustc_mir_dataflow::{self, move_path_children_matching, Analysis, MoveDataParamEnv};

use crate::MirPass;

/// Removes `Drop` and `DropAndReplace` terminators whose target is known to be uninitialized at
/// that point.
///
/// This is redundant with drop elaboration, but we need to do it prior to const-checking, and
/// running const-checking after drop elaboration makes it opimization dependent, causing issues
/// like [#90770].
///
/// [#90770]: https://github.com/rust-lang/rust/issues/90770
pub struct RemoveUninitDrops;

impl<'tcx> MirPass<'tcx> for RemoveUninitDrops {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
let param_env = tcx.param_env(body.source.def_id());
let Ok(move_data) = MoveData::gather_moves(body, tcx, param_env) else {
// We could continue if there are move errors, but there's not much point since our
// init data isn't complete.
return;
};

let mdpe = MoveDataParamEnv { move_data, param_env };
let mut maybe_inits = MaybeInitializedPlaces::new(tcx, body, &mdpe)
.into_engine(tcx, body)
.pass_name("remove_uninit_drops")
.iterate_to_fixpoint()
.into_results_cursor(body);

let mut to_remove = vec![];
for (bb, block) in body.basic_blocks().iter_enumerated() {
let terminator = block.terminator();
let (TerminatorKind::Drop { place, .. } | TerminatorKind::DropAndReplace { place, .. })
= &terminator.kind
else { continue };

maybe_inits.seek_before_primary_effect(body.terminator_loc(bb));

// If there's no move path for the dropped place, it's probably a `Deref`. Let it alone.
let LookupResult::Exact(mpi) = mdpe.move_data.rev_lookup.find(place.as_ref()) else {
continue;
};

let should_keep = is_needs_drop_and_init(
tcx,
param_env,
maybe_inits.get(),
&mdpe.move_data,
place.ty(body, tcx).ty,
mpi,
);
if !should_keep {
to_remove.push(bb)
}
}

for bb in to_remove {
let block = &mut body.basic_blocks_mut()[bb];

let (TerminatorKind::Drop { target, .. } | TerminatorKind::DropAndReplace { target, .. })
= &block.terminator().kind
else { unreachable!() };

// Replace block terminator with `Goto`.
let target = *target;
let old_terminator_kind = std::mem::replace(
&mut block.terminator_mut().kind,
TerminatorKind::Goto { target },
);

// If this is a `DropAndReplace`, we need to emulate the assignment to the return place.
if let TerminatorKind::DropAndReplace { place, value, .. } = old_terminator_kind {
block.statements.push(Statement {
source_info: block.terminator().source_info,
kind: StatementKind::Assign(Box::new((place, Rvalue::Use(value)))),
});
}
}
}
}

fn is_needs_drop_and_init(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
maybe_inits: &BitSet<MovePathIndex>,
move_data: &MoveData<'tcx>,
ty: Ty<'tcx>,
mpi: MovePathIndex,
) -> bool {
// No need to look deeper if the root is definitely uninit or if it has no `Drop` impl.
if !maybe_inits.contains(mpi) || !ty.needs_drop(tcx, param_env) {
return false;
}

let field_needs_drop_and_init = |(f, f_ty, mpi)| {
let child = move_path_children_matching(move_data, mpi, |x| x.is_field_to(f));
let Some(mpi) = child else {
return f_ty.needs_drop(tcx, param_env);
};

is_needs_drop_and_init(tcx, param_env, maybe_inits, move_data, f_ty, mpi)
};

// This pass is only needed for const-checking, so it doesn't handle as many cases as
// `DropCtxt::open_drop`, since they aren't relevant in a const-context.
match ty.kind() {
ty::Adt(adt, substs) => {
let dont_elaborate = adt.is_union() || adt.is_manually_drop() || adt.has_dtor(tcx);
if dont_elaborate {
return true;
}

// Look at all our fields, or if we are an enum all our variants and their fields.
//
// If a field's projection *is not* present in `MoveData`, it has the same
// initializedness as its parent (maybe init).
//
// If its projection *is* present in `MoveData`, then the field may have been moved
// from separate from its parent. Recurse.
adt.variants.iter_enumerated().any(|(vid, variant)| {
// Enums have multiple variants, which are discriminated with a `Downcast` projection.
// Structs have a single variant, and don't use a `Downcast` projection.
let mpi = if adt.is_enum() {
let downcast =
move_path_children_matching(move_data, mpi, |x| x.is_downcast_to(vid));
let Some(dc_mpi) = downcast else {
return variant_needs_drop(tcx, param_env, substs, variant);
};

dc_mpi
} else {
mpi
};

variant
.fields
.iter()
.enumerate()
.map(|(f, field)| (Field::from_usize(f), field.ty(tcx, substs), mpi))
.any(field_needs_drop_and_init)
})
}

ty::Tuple(_) => ty
.tuple_fields()
.enumerate()
.map(|(f, f_ty)| (Field::from_usize(f), f_ty, mpi))
.any(field_needs_drop_and_init),

_ => true,
}
}

fn variant_needs_drop(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
substs: SubstsRef<'tcx>,
variant: &VariantDef,
) -> bool {
variant.fields.iter().any(|field| {
let f_ty = field.ty(tcx, substs);
f_ty.needs_drop(tcx, param_env)
})
}
6 changes: 5 additions & 1 deletion compiler/rustc_mir_transform/src/remove_unneeded_drops.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
//! This pass replaces a drop of a type that does not need dropping, with a goto
//! This pass replaces a drop of a type that does not need dropping, with a goto.
//!
//! When the MIR is built, we check `needs_drop` before emitting a `Drop` for a place. This pass is
//! useful because (unlike MIR building) it runs after type checking, so it can make use of
//! `Reveal::All` to provide more precies type information.

use crate::MirPass;
use rustc_middle::mir::*;
Expand Down
17 changes: 5 additions & 12 deletions compiler/rustc_mir_transform/src/simplify_branches.rs
Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
//! A pass that simplifies branches when their condition is known.

use crate::MirPass;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;

use std::borrow::Cow;

pub struct SimplifyBranches {
/// A pass that replaces a branch with a goto when its condition is known.
pub struct SimplifyConstCondition {
label: String,
}

impl SimplifyBranches {
impl SimplifyConstCondition {
pub fn new(label: &str) -> Self {
SimplifyBranches { label: format!("SimplifyBranches-{}", label) }
SimplifyConstCondition { label: format!("SimplifyConstCondition-{}", label) }
}
}

impl<'tcx> MirPass<'tcx> for SimplifyBranches {
impl<'tcx> MirPass<'tcx> for SimplifyConstCondition {
fn name(&self) -> Cow<'_, str> {
Cow::Borrowed(&self.label)
}
Expand Down Expand Up @@ -53,12 +52,6 @@ impl<'tcx> MirPass<'tcx> for SimplifyBranches {
Some(v) if v == expected => TerminatorKind::Goto { target },
_ => continue,
},
TerminatorKind::FalseEdge { real_target, .. } => {
TerminatorKind::Goto { target: real_target }
}
TerminatorKind::FalseUnwind { real_target, .. } => {
TerminatorKind::Goto { target: real_target }
}
_ => continue,
};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- // MIR for `main` before SimplifyBranches-after-const-prop
+ // MIR for `main` after SimplifyBranches-after-const-prop
- // MIR for `main` before SimplifyConstCondition-after-const-prop
+ // MIR for `main` after SimplifyConstCondition-after-const-prop

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/switch_int.rs:6:11: 6:11
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/switch_int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
fn foo(_: i32) { }

// EMIT_MIR switch_int.main.ConstProp.diff
// EMIT_MIR switch_int.main.SimplifyBranches-after-const-prop.diff
// EMIT_MIR switch_int.main.SimplifyConstCondition-after-const-prop.diff
fn main() {
match 1 {
1 => foo(0),
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/early_otherwise_branch_68867.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub enum ViewportPercentageLength {
}

// EMIT_MIR early_otherwise_branch_68867.try_sum.EarlyOtherwiseBranch.diff
// EMIT_MIR early_otherwise_branch_68867.try_sum EarlyOtherwiseBranch.before SimplifyBranches-final.after
// EMIT_MIR early_otherwise_branch_68867.try_sum EarlyOtherwiseBranch.before SimplifyConstCondition-final.after
#[no_mangle]
pub extern "C" fn try_sum(
x: &ViewportPercentageLength,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- // MIR for `try_sum` before EarlyOtherwiseBranch
+ // MIR for `try_sum` after SimplifyBranches-final
+ // MIR for `try_sum` after SimplifyConstCondition-final

fn try_sum(_1: &ViewportPercentageLength, _2: &ViewportPercentageLength) -> Result<ViewportPercentageLength, ()> {
debug x => _1; // in scope 0 at $DIR/early_otherwise_branch_68867.rs:17:5: 17:6
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
- // MIR for `main` before SimplifyBranches-after-const-prop
+ // MIR for `main` after SimplifyBranches-after-const-prop
- // MIR for `main` before SimplifyConstCondition-after-const-prop
+ // MIR for `main` after SimplifyConstCondition-after-const-prop

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/simplify_if.rs:5:11: 5:11
Expand Down
Loading

0 comments on commit 3964131

Please sign in to comment.