Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
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;
pub use rustc_const_eval::transform::MirPass;
Expand Down Expand Up @@ -445,8 +447,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 @@ -456,7 +470,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 @@ -515,13 +529,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].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move drop elaboration earlier, so we can still do const-checking after drop elaboration?

Also, isn't this additionally redundant with things done inside borrowck, which also knows to ignore those Drop/DropAndReplace terminators? It doesn't actually remove them, presumably it has an analysis to figure out that they are not needed. Is that the same analysis that is also used here?

Copy link
Contributor Author

@ecstatic-morse ecstatic-morse Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to move drop elaboration earlier, so we can still do const-checking after drop elaboration?

How is that better than the status quo? To be clear, I think the status quo is perfectly valid, it just needed a stronger warning about what is allowed as part of post_borrowck_cleanup.

Also, isn't this additionally redundant with things done inside borrowck, which also knows to ignore those Drop/DropAndReplace terminators? It doesn't actually remove them, presumably it has an analysis to figure out that they are not needed. Is that the same analysis that is also used here?

My long term goal (as I stated in this comment) is to move remove_uninit_drops prior to borrow checking and remove the duplicate logic in the borrow checker. I'm a little nervous about this, since I don't fully understand the purpose of the FalseEdge/FalseUnwind terminators, which the borrow checker requires . FalseEdges seem to be used to connect distinct match arms to one another, thus losing information about what the active variant is in a match arm body. That last bit is what makes it possible for Option::unwrap to be const.

I requested some input about this idea from the MIR borrowck implementors as part of that comment. Perhaps you have some thoughts as well? It's trivial to (optionally) ignore FalseEdges while computing MaybeInitializedPlaces, but I don't feel comfortable doing so unilaterally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that better than the status quo?

My thinking was: if we can do drop elaboration early enough, maybe we don't have to redundantly also run remove_uninit_drops.

I'm a little nervous about this, since I don't fully understand the purpose of the FalseEdge/FalseUnwind terminators, which the borrow checker requires . FalseEdges seem to be used to connect distinct match arms to one another, thus losing information about what the active variant is in a match arm body.

My rough idea for their purpose is that they make the borrow checker accept code that we do not want to accept -- the extra edges make variables 'more live', so that we do not accept code which depends on details such as the exact order in which disjoint match arms are being generated in the MIR. However, I do not have a good understanding of the borrow checker, so I hope you can get better info the the borrowck people -- @nikomatsakis might be able to help or at least say who should know these details. :) @lqd and @matthewjasper might also know more.

///
/// [#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