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.

Sign up for GitHub

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

[WIP] Lower core::ops::drop directly to MIR drop #62864

Closed
wants to merge 1 commit into from
Closed

[WIP] Lower core::ops::drop directly to MIR drop #62864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libcore/mem/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,7 @@ pub fn replace<T>(dest: &mut T, mut src: T) -> T {
/// [`Copy`]: ../../std/marker/trait.Copy.html
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg_attr(not(bootstrap), lang = "drop")]
pub fn drop<T>(_x: T) { }

/// Interprets `src` as having type `&U`, and then reads `src` without moving
Expand Down
3 changes: 2 additions & 1 deletion src/libcore/ops/drop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@
/// let _second = PrintOnDrop("Declared second!");
/// }
/// ```
#[lang = "drop"]
#[cfg_attr(not(bootstrap), lang = "drop_trait")]
#[cfg_attr(bootstrap, lang = "drop")]
#[stable(feature = "rust1", since = "1.0.0")]
pub trait Drop {
/// Executes the destructor for this type.
Expand Down
3 changes: 2 additions & 1 deletion src/librustc/middle/lang_items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ language_item_table! {
SyncTraitLangItem, "sync", sync_trait, Target::Trait;
FreezeTraitLangItem, "freeze", freeze_trait, Target::Trait;

DropTraitLangItem, "drop", drop_trait, Target::Trait;
DropTraitLangItem, "drop_trait", drop_trait, Target::Trait;

CoerceUnsizedTraitLangItem, "coerce_unsized", coerce_unsized_trait, Target::Trait;
DispatchFromDynTraitLangItem,"dispatch_from_dyn", dispatch_from_dyn_trait, Target::Trait;
Expand Down Expand Up @@ -349,6 +349,7 @@ language_item_table! {

ExchangeMallocFnLangItem, "exchange_malloc", exchange_malloc_fn, Target::Fn;
BoxFreeFnLangItem, "box_free", box_free_fn, Target::Fn;
DropFnLangItem, "drop", drop_fn, Target::Fn;
DropInPlaceFnLangItem, "drop_in_place", drop_in_place_fn, Target::Fn;
OomLangItem, "oom", oom, Target::Fn;
AllocLayoutLangItem, "alloc_layout", alloc_layout, Target::Struct;
Expand Down
67 changes: 46 additions & 21 deletions src/librustc_mir/build/expr/into.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,16 +191,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
exit_block.unit()
}
ExprKind::Call { ty, fun, args, from_hir_call } => {
let intrinsic = match ty.sty {
let (fn_def_id, intrinsic) = match ty.sty {
ty::FnDef(def_id, _) => {
let f = ty.fn_sig(this.hir.tcx());
if f.abi() == Abi::RustIntrinsic || f.abi() == Abi::PlatformIntrinsic {
Some(this.hir.tcx().item_name(def_id).as_str())
(Some(def_id), Some(this.hir.tcx().item_name(def_id).as_str()))
} else {
None
(Some(def_id), None)
}
}
_ => None,
_ => (None, None),
};
let intrinsic = intrinsic.as_ref().map(|s| &s[..]);
let fun = unpack!(block = this.as_local_operand(block, fun));
Expand Down Expand Up @@ -237,26 +237,51 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
.map(|arg| unpack!(block = this.as_local_operand(block, arg)))
.collect();

let drop_location = if fn_def_id.is_some()
&& this.hir.tcx().lang_items().drop_fn() == fn_def_id
{
assert_eq!(args.len(), 1, "drop() must have exactly one argument");
match &args[0] {
Operand::Move(place) => Some(place.clone()),
_ => None,
}
} else {
None
};

let success = this.cfg.start_new_block();
let cleanup = this.diverge_cleanup();
this.cfg.terminate(
block,
source_info,
TerminatorKind::Call {
func: fun,
args,
cleanup: Some(cleanup),
// FIXME(varkor): replace this with an uninhabitedness-based check.
// This requires getting access to the current module to call
// `tcx.is_ty_uninhabited_from`, which is currently tricky to do.
destination: if expr.ty.is_never() {
None
} else {
Some((destination.clone(), success))

if let Some(location) = drop_location {
this.cfg.terminate(
block,
source_info,
TerminatorKind::Drop {
location,
target: success,
unwind: Some(cleanup)
},
from_hir_call,
},
);
);
} else {
this.cfg.terminate(
block,
source_info,
TerminatorKind::Call {
func: fun,
args,
cleanup: Some(cleanup),
// FIXME(varkor): replace this with an uninhabitedness-based check.
// This requires getting access to the current module to call
// `tcx.is_ty_uninhabited_from`, which is currently tricky to do.
destination: if expr.ty.is_never() {
None
} else {
Some((destination.clone(), success))
},
from_hir_call,
},
);
}
success.unit()
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/box_expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl Drop for S {
// StorageLive(_3);
// StorageLive(_4);
// _4 = move _1;
// _3 = const std::mem::drop::<std::boxed::Box<S>>(move _4) -> [return: bb5, unwind: bb7];
// drop(_4) -> [return: bb5, unwind: bb7];
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change actually result in generators being smaller? I was under the impression that the move on the line above was the problem.

Copy link
Member

Choose a reason for hiding this comment

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

No, this change does not change the size of generators, but it allows removing the let binding inside the .await expansion, which would reduce the size of generators.

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to solve that problem either, there's still the move on the line above that needs to be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch, that move will need to be eliminated as well.

// }
//
// bb5: {
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/issue-49232.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ fn main() {
// StorageLive(_5);
// StorageLive(_6);
// _6 = &_2;
// _5 = const std::mem::drop::<&i32>(move _6) -> [return: bb13, unwind: bb4];
// drop(_6) -> [return: bb13, unwind: bb4];
// }
// bb13: {
// StorageDead(_6);
Expand Down
4 changes: 3 additions & 1 deletion src/test/ui/type_length_limit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ link! { F, G }

pub struct G;

fn take<T>(x: T) {}

fn main() {
drop::<Option<A>>(None);
take::<Option<A>>(None);
}
8 changes: 4 additions & 4 deletions src/test/ui/type_length_limit.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: reached the type-length limit while instantiating `std::mem::drop::<std::option::Op... G), (G, G, G), (G, G, G))))))>>`
--> $SRC_DIR/libcore/mem/mod.rs:LL:COL
error: reached the type-length limit while instantiating `take::<std::option::Option<(((((... G), (G, G, G), (G, G, G))))))>>`
--> $DIR/type_length_limit.rs:25:1
|
LL | pub fn drop<T>(_x: T) { }
| ^^^^^^^^^^^^^^^^^^^^^^^^^
LL | fn take<T>(x: T) {}
| ^^^^^^^^^^^^^^^^^^^
|
= note: consider adding a `#![type_length_limit="1094"]` attribute to your crate

Expand Down