[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes #111807

erikdesjardins · 2023-05-20T23:46:17Z

This resurrects PR #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.

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 #103957, Miri was changed to retag drop_in_place's argument as if it was &mut, matching this change.

…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.

The CHECK, -NOT, -SAME pattern ensures that the `CHECK-NOT: noalias` is limited to only one line, and won't match unrelated lines further down in the file. Explicit drop call added to preserve the `foo` argument name, since names of unused arguments are not preserved.

workingjubilee · 2023-05-21T00:44:29Z

@bors try @rust-timer queue

bors · 2023-05-21T00:44:38Z

⌛ Trying commit c4d69b7 with merge 68f76c31233347201320d356e0de9c9ace939293...

bors · 2023-05-21T02:53:58Z

☀️ Try build successful - checks-actions
Build commit: 68f76c31233347201320d356e0de9c9ace939293 (68f76c31233347201320d356e0de9c9ace939293)

RalfJung · 2023-05-21T15:09:13Z

library/core/src/ptr/mod.rs

+///
+/// * `to_drop` must be nonnull, even if `T` has size 0.
+///
+/// * The value `to_drop` points to must be valid for dropping, which may mean


This particular requirement is ill-placed in the list since it is not a consequence of the things said above the enumeration -- but the docs claim that all list items follow from that prior information.

I think it is better to just state the requirement, we don't need to document hiw they come about because from the internal implementation details.

Makes sense, undid the change to the introduction of this section

rust-timer · 2023-05-22T03:06:13Z

Finished benchmarking commit (68f76c31233347201320d356e0de9c9ace939293): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking 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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.3%, 0.6%]	9
Regressions ❌ (secondary)	1.2%	[0.4%, 2.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	0.4%	[0.3%, 0.6%]	9

Max RSS (memory usage)

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	4.4%	[4.4%, 4.4%]	1
Regressions ❌ (secondary)	0.9%	[0.9%, 0.9%]	1
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[-3.1%, 4.4%]	2

Cycles

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[1.7%, 2.7%]	2
Improvements ✅ (primary)	-1.6%	[-1.7%, -1.5%]	2
Improvements ✅ (secondary)	-4.6%	[-10.2%, -1.4%]	10
All ❌✅ (primary)	-1.6%	[-1.7%, -1.5%]	2

Binary size

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 0.6%]	27
Regressions ❌ (secondary)	0.4%	[0.1%, 0.7%]	13
Improvements ✅ (primary)	-1.7%	[-1.7%, -1.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-1.7%, 0.6%]	28

Bootstrap: 645.412s -> 646.792s (0.21%)

erikdesjardins · 2023-05-22T03:57:08Z

Perf regressions are few, small in primary benchmarks (and not huge in secondary benchmarks), and are generally attributed to LLVM, which I think is acceptable. The point of this is to enable more optimizations*, so it's expected that LLVM does more work.
_{* @pcwalton's original finding that this change reduced the number of stack-to-stack copies by 6% should still be relevant, since he ran that test against LLVM 16}

oli-obk · 2023-05-22T13:41:19Z

@bors r+

bors · 2023-05-22T13:41:21Z

📌 Commit 340827a has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-05-22T23:10:15Z

⌛ Testing commit 340827a with merge 4570822ab9e0203f50ec1ec449a66f4351270ad8...

bors · 2023-05-22T23:47:51Z

💔 Test failed - checks-actions

…n nopt builders

oli-obk · 2023-05-23T07:32:23Z

@bors r+

bors · 2023-05-23T07:32:24Z

📌 Commit fb7f1d2 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-05-23T10:12:49Z

⌛ Testing commit fb7f1d2 with merge f3d597b...

bors · 2023-05-23T12:47:44Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing f3d597b to master...

rust-timer · 2023-05-23T14:03:00Z

Finished benchmarking commit (f3d597b): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 0.7%]	17
Regressions ❌ (secondary)	1.3%	[0.2%, 2.9%]	6
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-0.2%, 0.7%]	18

Max RSS (memory usage)

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	5.4%	[5.4%, 5.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.8%, -1.5%]	2
All ❌✅ (primary)	5.4%	[5.4%, 5.4%]	1

Cycles

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.4%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

Results

This 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.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 0.6%]	27
Regressions ❌ (secondary)	0.4%	[0.1%, 0.7%]	13
Improvements ✅ (primary)	-1.8%	[-1.8%, -1.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-1.8%, 0.6%]	28

Bootstrap: 644.28s -> 646.644s (0.37%)

pnkfelix · 2023-05-23T15:10:26Z

This change emits more attributes; it is expected to slow down the compiler, and that is compensated by the fact that it enables better code-generation from what we emit out of the compiler

@rustbot label: +perf-regression-triaged

pcwalton and others added 9 commits May 20, 2023 18:12

Fix noalias box test

74dd3cb

Update documentation for drop_in_place()

2836e55

Add missing "unsafe" to fix doctest

b2ef9f7

improve drop_in_place docs

58c3999

improve code checking for drop_in_place lang item

47444d7

ensure !Unpin types do not get noalias

6448183

rustbot assigned oli-obk May 20, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 20, 2023

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 21, 2023

This comment has been minimized.

Sign in to view

RalfJung reviewed May 21, 2023

View reviewed changes

drop_in_place docs: remove pseudocode-ish implementation details

340827a

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 22, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 22, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 22, 2023

drop-in-place-noalias test: needs -O to ensure attributes are added o…

fb7f1d2

…n nopt builders

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label May 23, 2023

bors merged commit f3d597b into rust-lang:master May 23, 2023

rustbot added this to the 1.71.0 milestone May 23, 2023

bors mentioned this pull request May 23, 2023

Shrink error variants for layout and fn_abi #111035

Merged

erikdesjardins deleted the noalias branch May 23, 2023 14:07

pnkfelix mentioned this pull request May 23, 2023

[rustc_ty_utils] Add the LLVM noalias parameter attribute to drop_in_place in certain cases. #103614

Closed

rustbot added the perf-regression-triaged The performance regression has been triaged. label May 23, 2023

matthiaskrgr mentioned this pull request May 23, 2023

ICE: argument to drop_in_place is not a raw ptr: &dyn Trait2<Type1> #111883

Closed

pnkfelix mentioned this pull request May 30, 2023

triage oddity: why did same PR show up in two consecutive weeks rust-lang/rustc-perf#1599

Open

lukas-code mentioned this pull request Aug 13, 2023

ICE: Failed to extract DefId: opt_local_def_id_to_hir_id #114768

Open

Skepfyr mentioned this pull request Jan 6, 2024

regression: overflow evaluating requirement #113239

Open

lukas-code mentioned this pull request Oct 27, 2024

Rc/Arc: don't leak the allocation if drop panics #132231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes #111807

[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes #111807

erikdesjardins commented May 20, 2023

workingjubilee commented May 21, 2023

This comment has been minimized.

bors commented May 21, 2023

bors commented May 21, 2023

This comment has been minimized.

RalfJung May 21, 2023

erikdesjardins May 21, 2023

rust-timer commented May 22, 2023

erikdesjardins commented May 22, 2023

oli-obk commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

This comment has been minimized.

bors commented May 22, 2023

oli-obk commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

rust-timer commented May 23, 2023

pnkfelix commented May 23, 2023

[rustc_ty_utils] Treat drop_in_place's *mut argument like &mut when adding LLVM attributes #111807

[rustc_ty_utils] Treat drop_in_place's *mut argument like &mut when adding LLVM attributes #111807

Conversation

erikdesjardins commented May 20, 2023

workingjubilee commented May 21, 2023

This comment has been minimized.

bors commented May 21, 2023

bors commented May 21, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-timer commented May 22, 2023

Overall result: ❌ regressions - ACTION NEEDED

erikdesjardins commented May 22, 2023

oli-obk commented May 22, 2023

bors commented May 22, 2023

bors commented May 22, 2023

This comment has been minimized.

bors commented May 22, 2023

oli-obk commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

bors commented May 23, 2023

rust-timer commented May 23, 2023

Overall result: ❌ regressions - ACTION NEEDED

pnkfelix commented May 23, 2023

[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes #111807

[rustc_ty_utils] Treat `drop_in_place`'s *mut argument like &mut when adding LLVM attributes #111807