[rustc_ty_utils] Add the LLVM noalias parameter attribute to drop_in_place in certain cases.
#103614
Conversation
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
r? @oli-obk (Feel free to reassign review to someone else) :) |
|
Please also add comments on drop_in_place in libstd to make sure anyone touching it is aware they should also touch this logic |
|
Per discussions on Zulip, I'm going to change this to a draft while we wait on Miri results to figure out what the impact of this change will be. |
…in_place` in certain cases. LLVM can make use of the `noalias` parameter attribute on the parameter to `drop_in_place` in areas like argument promotion. Because the Rust compiler fully controls the code for `drop_in_place`, it can soundly deduce parameter attributes on it. In the case of a value that has a programmer-defined Drop implementation, we know that the first thing `drop_in_place` will do is pass a pointer to the object to `Drop::drop`. `Drop::drop` takes `&mut`, so it must be guaranteed that there are no pointers to the object upon entering that function. Therefore, it should be safe to mark `noalias` there. With this patch, we mark `noalias` only when the type is a value with a programmer-defined Drop implementation. This is possibly overly conservative, but I thought that proceeding cautiously was best in this instance.
… unconditionally. We've done measurements with Miri and have determined that `noalias` shouldn't break code. The requirements that allow us to add dereferenceable and align have been long documented in the standard library documentation.
pcwalton
force-pushed
the
drop-in-place-noalias
branch
from
3d402b0
to
02cfabe
Compare
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
Could not assign reviewer from: |
| /// | ||
| /// * The value `to_drop` points to must be valid for dropping, which may mean it must uphold | ||
| /// additional invariants - this is type-dependent. | ||
| /// * While `drop_in_place` is executing, the only way to access parts of |
There was a problem hiding this comment.
@RalfJung Would it be better to say
| /// * While `drop_in_place` is executing, the only way to access parts of | |
| /// * As soon as `drop_in_place` begins executing, the only way to access parts of |
There was a problem hiding this comment.
What is the difference you are getting at here?
One drop_in_place returns, old references can be used again, at least as far as the aliasing model goes.
This comment has been minimized.
This comment has been minimized.
|
This patch reduces the number of stack-to-stack copies from 2.50% to 2.36%, a 6% decrease. |
|
The Miri version of this is #103957. I'd prefer if we could land that first.
|
|
Just out of idle curiosity1... @bors try @rust-timer queue Footnotes
|
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 53f21aa with merge 7250e7fb50ffb04be8243ed3f0d9baa1caa63d4a... |
As long as it doesn't take too long. That PR has been sitting idle for 5 days. I don't want this to bitrot. |
|
Bitrot risk for this PR seems low and 5 days is not very long. I don't think this is an urgent change either. |
| /// Immediately upon executing, `drop_in_place` takes out a mutable borrow on the | ||
| /// pointed-to-value. Effectively, this function is implemented like so: | ||
| /// | ||
| /// ``` | ||
| /// # struct Foo { x: i32 } | ||
| /// unsafe fn drop_in_place(to_drop: *mut Foo) { | ||
| /// let mut value = &mut *to_drop; | ||
| /// // ... drop the fields of `value` ... | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
What we actually do is stronger than that: we have an &mut function argument. That makes a difference because for noalias, scope matters.
So if you want to write this in code, I'd suggest something like
/// ```
/// # struct Foo { x: i32 }
/// unsafe fn drop_in_place(to_drop: *mut Foo) {
/// drop_in_place_inner(&mut *to_drop);
/// unsafe fn drop_in_place_inner(to_drop: &mut Foo) {
/// // ... drop the fields of `value` ...
/// }
/// }
/// ```
| /// * `to_drop` must be properly aligned, even if T has size 0. | ||
| /// | ||
| /// * `to_drop` must be nonnull, even if T has size 0. |
There was a problem hiding this comment.
| /// * `to_drop` must be properly aligned, even if T has size 0. | |
| /// | |
| /// * `to_drop` must be nonnull, even if T has size 0. | |
| /// * `to_drop` must be properly aligned, even if `T` has size 0. | |
| /// | |
| /// * `to_drop` must be nonnull, even if `T` has size 0. |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7250e7fb50ffb04be8243ed3f0d9baa1caa63d4a): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
rustbot
added
perf-regression
|
Some of that regression is probably caused by emitting more attributes.
|
|
Yeah, the classic problem of measuring performance changes by measuring the compiler itself. |
|
If folks are concerned about the performance regression, I can disable this in incremental compilation mode. It doesn't sit that well with me though—it's essentially just improving compilation performance by artificially making LLVM able to optimize less. |
|
Oh, also keep in mind that I'm testing against LLVM 16, while the benchmark presumably isn't. |
I can't speak to anybody else, but I'm not concerned. Or, my concern is just that the value of this kind of thing can't really be measured using our existing perf suite, but that's an existing issue that shouldn't impact this PR (IMO). |
| let is_drop_in_place = match (cx.tcx.lang_items().drop_in_place_fn(), fn_def_id) { | ||
| (Some(drop_in_place_fn), Some(fn_def_id)) => drop_in_place_fn == fn_def_id, | ||
| _ => false, | ||
| }; |
There was a problem hiding this comment.
| let is_drop_in_place = match (cx.tcx.lang_items().drop_in_place_fn(), fn_def_id) { | |
| (Some(drop_in_place_fn), Some(fn_def_id)) => drop_in_place_fn == fn_def_id, | |
| _ => false, | |
| }; | |
| let is_drop_in_place = cx.tcx.lang_items().drop_in_place_fn() == Some(fn_def_id); |
|
@rustbot author perf regression is entirely in LLVM, so since this causes actual runtime improvements, and the regressions are small in primary tests, this lgtm |
rustbot
added
S-waiting-on-author
|
#103957 finally landed, so from a Miri / MIR semantics perspective this is good to go now. There are a bunch of review comments above though. |
[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes This resurrects PR rust-lang#103614, which has sat idle for a while. This could probably use a new perf run, since we're on a new LLVM version now. r? `@oli-obk` cc `@RalfJung` --- LLVM can make use of the `noalias` parameter attribute on the parameter to `drop_in_place` in areas like argument promotion. Because the Rust compiler fully controls the code for `drop_in_place`, it can soundly deduce parameter attributes on it. In rust-lang#103957, Miri was changed to retag `drop_in_place`'s argument as if it was `&mut`, matching this change.
|
This ended up landing via PR #111807 if I understand correctly. |
LLVM can make use of the
noaliasparameter attribute on the parameter todrop_in_placein areas like argument promotion. Because the Rust compiler fully controls the code fordrop_in_place, it can soundly deduce parameter attributes on it. In the case of a value that has a programmer-defined Drop implementation, we know that the first thingdrop_in_placewill do is pass a pointer to the object toDrop::drop.Drop::droptakes&mut, so it must be guaranteed that there are no pointers to the object upon entering that function. Therefore, it should be safe to marknoaliasthere.With this patch, we mark
noaliasonly when the type is a value with a programmer-defined Drop implementation. This is possibly overly conservative, but I thought that proceeding cautiously was best in this instance.