Skip to content

Commit

Permalink
Auto merge of #68943 - ecstatic-morse:no-useless-drop-on-enum-variant…
Browse files Browse the repository at this point in the history
…s, r=matthewjasper

Skip `Drop` terminators for enum variants without drop glue

Split out from #68528.

When doing drop elaboration for an `enum` that may or may not be moved out of (an open drop), we check the discriminant of the `enum` to see whether the live variant has any drop flags and then check the drop flags to see whether we need to drop each field. Sometimes, however, the live
variant has no move paths and thus no drop flags. In this case, we still emit a drop terminator
for the entire enum after checking the enum discriminant. This drop shim will check the discriminant of the enum *again* and then drop the fields of the active variant. If the active variant has no drop glue, nothing will be done.

This commit skips emitting the drop terminator during drop elaboration when the "otherwise" variants, those without move paths, have no drop glue. A common example of this scenario is when an `Option` is moved from, since `Option::None` never needs drop glue. Below is a fragment the pre-codegen CFG for `Option::unwrap_or` in which we check the drop flag (`_5`) for `self` (`_1`), before and after the change.

Before:

![image](https://user-images.githubusercontent.com/29463364/74078927-52942380-49e5-11ea-8e34-4b9d6d94ef25.png)

After:

![image](https://user-images.githubusercontent.com/29463364/74078945-78b9c380-49e5-11ea-8302-b043c4a7515a.png)

This change doesn't do much on its own, but it is a prerequisite to get the perf gains from #68528.

cc @arielb1
  • Loading branch information
bors committed Mar 1, 2020
2 parents 2917d99 + b23d910 commit d905134
Showing 1 changed file with 30 additions and 15 deletions.
45 changes: 30 additions & 15 deletions src/librustc_mir/util/elaborate_drops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,9 @@ where
// FIXME: I think we should just control the flags externally,
// and then we do not need this machinery.
pub fn elaborate_drop(&mut self, bb: BasicBlock) {
debug!("elaborate_drop({:?})", self);
debug!("elaborate_drop({:?}, {:?})", bb, self);
let style = self.elaborator.drop_style(self.path, DropFlagMode::Deep);
debug!("elaborate_drop({:?}): live - {:?}", self, style);
debug!("elaborate_drop({:?}, {:?}): live - {:?}", bb, self, style);
match style {
DropStyle::Dead => {
self.elaborator
Expand Down Expand Up @@ -426,25 +426,21 @@ where
let mut unwind_blocks =
if unwind.is_cleanup() { None } else { Some(Vec::with_capacity(adt.variants.len())) };

let mut have_otherwise_with_drop_glue = false;
let mut have_otherwise = false;
let tcx = self.tcx();

for (variant_index, discr) in adt.discriminants(tcx) {
let variant = &adt.variants[variant_index];
let subpath = self.elaborator.downcast_subpath(self.path, variant_index);

if let Some(variant_path) = subpath {
let base_place = tcx.mk_place_elem(
self.place.clone(),
ProjectionElem::Downcast(
Some(adt.variants[variant_index].ident.name),
variant_index,
),
);
let fields = self.move_paths_for_fields(
&base_place,
variant_path,
&adt.variants[variant_index],
substs,
ProjectionElem::Downcast(Some(variant.ident.name), variant_index),
);
let fields =
self.move_paths_for_fields(&base_place, variant_path, &variant, substs);
values.push(discr.val);
if let Unwind::To(unwind) = unwind {
// We can't use the half-ladder from the original
Expand Down Expand Up @@ -474,16 +470,30 @@ where
normal_blocks.push(normal);
} else {
have_otherwise = true;

let param_env = self.elaborator.param_env();
let have_field_with_drop_glue = variant
.fields
.iter()
.any(|field| field.ty(tcx, substs).needs_drop(tcx, param_env));
if have_field_with_drop_glue {
have_otherwise_with_drop_glue = true;
}
}
}

if have_otherwise {
if !have_otherwise {
values.pop();
} else if !have_otherwise_with_drop_glue {
normal_blocks.push(self.goto_block(succ, unwind));
if let Unwind::To(unwind) = unwind {
unwind_blocks.as_mut().unwrap().push(self.goto_block(unwind, Unwind::InCleanup));
}
} else {
normal_blocks.push(self.drop_block(succ, unwind));
if let Unwind::To(unwind) = unwind {
unwind_blocks.as_mut().unwrap().push(self.drop_block(unwind, Unwind::InCleanup));
}
} else {
values.pop();
}

(
Expand Down Expand Up @@ -929,6 +939,11 @@ where
self.new_block(unwind, block)
}

fn goto_block(&mut self, target: BasicBlock, unwind: Unwind) -> BasicBlock {
let block = TerminatorKind::Goto { target };
self.new_block(unwind, block)
}

fn drop_flag_test_block(
&mut self,
on_set: BasicBlock,
Expand Down

0 comments on commit d905134

Please sign in to comment.