change the type of the argument of drop_in_place lang item to &mut _#154327
change the type of the argument of drop_in_place lang item to &mut _#154327WaffleLapkin wants to merge 8 commits intorust-lang:mainfrom
drop_in_place lang item to &mut _#154327Conversation
|
Oh wow I hadn't expected my wish to be fulfilled before I could even finish my PR that motivated me to express the wish in the first place. :-) ❤️ |
This comment has been minimized.
This comment has been minimized.
281786e to
4842194
Compare
This comment has been minimized.
This comment has been minimized.
794fcf5 to
ea0c913
Compare
This comment has been minimized.
This comment has been minimized.
ea0c913 to
3dda404
Compare
This comment has been minimized.
This comment has been minimized.
3dda404 to
32510c2
Compare
This comment has been minimized.
This comment has been minimized.
|
I like the interpreter changes and renames. Not sure what the status of the rest of the PR is, but if you submit just those as a separate PR I'll r+. |
32510c2 to
487fcc4
Compare
This comment has been minimized.
This comment has been minimized.
487fcc4 to
6e43096
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…s, r=RalfJung Slightly refactor mplace<->ptr conversions split off of rust-lang#154327 r? RalfJung
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
|
LGTM apart from the problem that CI already pointed out. :) |
(scottmcm left this while adding `Local::arg`...)
40e1bb2 to
8d5a266
Compare
| StatementKind::Assign(Box::new((Place::from(cor_ref_local), reborrow))), | ||
| StatementKind::Assign(Box::new(( | ||
| Place::from(cor_ref_local), | ||
| Rvalue::Use(Operand::Move(Place::from(cor_ref_tmp_local)), WithRetag::No), |
There was a problem hiding this comment.
| Rvalue::Use(Operand::Move(Place::from(cor_ref_tmp_local)), WithRetag::No), | |
| Rvalue::Use(Operand::Move(Place::from(cor_ref_tmp_local)), WithRetag::Yes), |
All assignments generated during MIR building should have retags.
There was a problem hiding this comment.
Ugh, got confused with other changes. Fixed.
Is WithRetags::No for cases where an assignment did not exist in the original MIR, but got introduced by an optimization, and thus doing a retag would introduce UB, which did not exist in the original MIR?
There was a problem hiding this comment.
Yes, that's one major usecase for WithRetags::No.
8d5a266 to
e0ecfbb
Compare
|
r=me with CI green |
|
@bors r=RalfJung,scottmcm,saethlin |
This comment has been minimized.
This comment has been minimized.
…ottmcm,saethlin change the type of the argument of `drop_in_place` lang item to `&mut _` We used to special case `core::ptr::drop_in_place` when computing LLVM argument attributes with this hack: https://github.com/rust-lang/rust/blob/db5e2dc248fe5bb26f70d7baec46a3bca9fa3e1d/compiler/rustc_ty_utils/src/abi.rs#L383-L392 This is because even though `drop_in_place` takes a `*mut T` it is semantically a `&mut T` (remember how `&mut Self` is passed to `Drop::drop`). This is apparently relevant for perf. This PR replaces this hack with a simpler solution -- it makes `drop_in_place` a thin wrapper around newly added `core::ptr::drop_glue`, which is the actual lang item and takes a `&mut T`: https://github.com/rust-lang/rust/blob/d2563d5003bbecff1efc40c1f5673ceec603825b/library/core/src/ptr/mod.rs#L810-L833 ------ The rest of the PR is blessing tests and cleaning up things which are not necessary after this change. One thing that is a bit awkward is that now that `drop_glue` is the actual lang item, a lot of the comments referring to `drop_in_place` are outdated. Should I try fixing that? I've also changed `async_drop_in_place` to take a `&mut T`, and it simplified the code handling it a bit. (since it's unstable we don't need to introduce a wrapper) ------- cc @RalfJung Closes #154274
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for 2e5d870 failed: CI. Failed job:
|
View all comments
We used to special case
core::ptr::drop_in_placewhen computing LLVM argument attributes with this hack:rust/compiler/rustc_ty_utils/src/abi.rs
Lines 383 to 392 in db5e2dc
This is because even though
drop_in_placetakes a*mut Tit is semantically a&mut T(remember how&mut Selfis passed toDrop::drop). This is apparently relevant for perf.This PR replaces this hack with a simpler solution -- it makes
drop_in_placea thin wrapper around newly addedcore::ptr::drop_glue, which is the actual lang item and takes a&mut T:rust/library/core/src/ptr/mod.rs
Lines 810 to 833 in d2563d5
The rest of the PR is blessing tests and cleaning up things which are not necessary after this change.
One thing that is a bit awkward is that now that
drop_glueis the actual lang item, a lot of the comments referring todrop_in_placeare outdated. Should I try fixing that?I've also changed
async_drop_in_placeto take a&mut T, and it simplified the code handling it a bit. (since it's unstable we don't need to introduce a wrapper)cc @RalfJung
Closes #154274