-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
bors
merged 8 commits into
rust-lang:master
from
ecstatic-morse:const-precise-live-drops-take-2
Dec 3, 2021
Merged
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
bb27b05
Separate `RemoveFalseEdges` from `SimplifyBranches`
ecstatic-morse d707707
Add "is" methods for projections to a given index
ecstatic-morse 4f7605b
Add `RemoveUninitDrops` MIR pass
ecstatic-morse ce2959d
Add rationale for `RemoveUnneededDrops`
ecstatic-morse 3e0e8be
Handle `DropAndReplace` in const-checking
ecstatic-morse 58c996c
Move post-elaboration const-checking earlier in the pipeline
ecstatic-morse 9aaca1d
Update MIR opt tests with new name
ecstatic-morse 37fa925
Add regression test for #90770
ecstatic-morse File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
171
compiler/rustc_mir_transform/src/remove_uninit_drops.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) | ||
}) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 2 additions & 2 deletions
4
...in.SimplifyBranches-after-const-prop.diff → ...plifyConstCondition-after-const-prop.diff
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
....before-SimplifyBranches-final.after.diff → ...e-SimplifyConstCondition-final.after.diff
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 2 additions & 2 deletions
4
...in.SimplifyBranches-after-const-prop.diff → ...plifyConstCondition-after-const-prop.diff
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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? 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
.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 theFalseEdge
/FalseUnwind
terminators, which the borrow checker requires .FalseEdge
s 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 forOption::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
FalseEdge
s while computingMaybeInitializedPlaces
, but I don't feel comfortable doing so unilaterally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was: if we can do drop elaboration early enough, maybe we don't have to redundantly also run remove_uninit_drops.
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.