More ptr metadata gvn #126541

scottmcm · 2024-06-16T09:06:51Z

There's basically 3 parts to this PR.

Allow references as arguments to UnOp::PtrMetadata

This is a MIR semantics addition, so
r? mir

Rather than just raw pointers, also allow references to be passed to PtrMetadata. That means the length of a slice can be just PtrMetadata(_1) instead of also needing a ref-to-pointer statement (_2 = &raw *_1 + PtrMetadata(_2)).

AFAIK there should be no provenance or tagging implications of looking at the metadata of a pointer, and the code in the backends actually already supported it (other than a debug assert, given that they don't care about ptr vs reference, really), so we might as well allow it.

Simplify the argument to PtrMetadata in GVN

Because the specific kind of pointer-like thing isn't that important, GVN can simplify all those details away. Things like *const-to-*mut casts and &mut-to-& reborrows are irrelevant, and skipping them lets it see more interesting things.

cc @cjgillot

Notably, unsizing casts for arrays. GVN supported that for Len, and now it sees it for PtrMetadata as well, allowing PtrMetadata(pointer) to become a constant if that pointer came from an array-to-slice unsizing, even through a bunch of other possible steps.

Replace NormalizeArrayLen with GVN

The NormalizeArrayLen pass hasn't been running even in optimized builds for well over a year, and it turns out that GVN -- which is on in optimized builds -- can do everything it was trying to do.

So the code for the pass is deleted, but the tests are kept, just changed to the different pass.

As part of this, LowerSliceLen was changed to emit PtrMetadata(_1) instead of Len(*_1), a small step on the road to eventually eliminating Rvalue::Len.

rustbot · 2024-06-16T09:06:59Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

scottmcm · 2024-06-16T09:24:24Z

...r-opt/pre-codegen/slice_index.slice_get_unchecked_mut_range.PreCodegen.after.panic-abort.mir

-        _7 = PtrMetadata(_6);
-        StorageDead(_6);
-        _8 = <std::ops::Range<usize> as SliceIndex<[T]>>::get_unchecked_mut::precondition_check(_3, _4, move _7) -> [return: bb1, unwind unreachable];
+        _6 = PtrMetadata(_1);


Note here how it can grab the length from the &mut [u32] directly, and thus the cast to *const [u32] can completely disappear, saving a statement and a local.

rustbot · 2024-06-16T09:31:34Z

Some changes occurred in coverage tests.

cc @Zalathar

scottmcm · 2024-06-16T10:09:43Z

Since this touches .len(), probably worth a
@bors try @rust-timer queue

More ptr metadata gvn There's basically 3 parts to this PR. 1. Allow references as arguments to `UnOp::PtrMetadata` This is a MIR semantics addition, so r? mir Rather than just raw pointers, also allow references to be passed to `PtrMetadata`. That means the length of a slice can be just `PtrMetadata(_1)` instead of also needing a ref-to-pointer statement (`_2 = &raw *_1` + `PtrMetadata(_2)`). AFAIK there should be no provenance or tagging implications of looking at the *metadata* of a pointer, and the code in the backends actually already supported it (other than a debug assert, given that they don't care about ptr vs reference, really), so we might as well allow it. 2. Simplify the argument to `PtrMetadata` in GVN Because the specific kind of pointer-like thing isn't that important, GVN can simplify all those details away. Things like `*const`-to-`*mut` casts and `&mut`-to-`&` reborrows are irrelevant, and skipping them lets it see more interesting things. cc `@cjgillot` Notably, unsizing casts for arrays. GVN supported that for `Len`, and now it sees it for `PtrMetadata` as well, allowing `PtrMetadata(pointer)` to become a constant if that pointer came from an array-to-slice unsizing, even through a bunch of other possible steps. 3. Replace `NormalizeArrayLen` with GVN The `NormalizeArrayLen` pass hasn't been running even in optimized builds for well over a year, and it turns out that GVN -- which *is* on in optimized builds -- can do everything it was trying to do. So the code for the pass is deleted, but the tests are kept, just changed to the different pass. As part of this, `LowerSliceLen` was changed to emit `PtrMetadata(_1)` instead of `Len(*_1)`, a small step on the road to eventually eliminating `Rvalue::Len`.

bors · 2024-06-16T10:10:55Z

⌛ Trying commit 72cda61 with merge bcb40db...

bors · 2024-06-16T11:51:24Z

☀️ Try build successful - checks-actions
Build commit: bcb40db (bcb40db9974a93769468e3683e983c8a2da624cf)

rust-timer · 2024-06-16T13:06:18Z

Finished benchmarking commit (bcb40db): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	5
Improvements ✅ (primary)	-0.5%	[-1.1%, -0.3%]	8
Improvements ✅ (secondary)	-2.0%	[-2.0%, -2.0%]	1
All ❌✅ (primary)	-0.4%	[-1.1%, 0.7%]	9

Max RSS (memory usage)

Results (primary -1.2%)

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.2%	[4.5%, 5.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.5%	[-10.2%, -2.9%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.2%	[-10.2%, 5.9%]	5

Cycles

Results (primary 0.3%, secondary 7.0%)

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)	1.3%	[1.3%, 1.3%]	1
Regressions ❌ (secondary)	7.0%	[3.9%, 8.9%]	4
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-0.7%, 1.3%]	2

Binary size

Results (primary 0.0%)

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.1%	[0.0%, 0.7%]	13
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	19
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.4%, 0.7%]	32

Bootstrap: 672.776s -> 674.201s (0.21%)
Artifact size: 319.83 MiB -> 320.43 MiB (0.19%)

cjgillot · 2024-06-16T14:13:00Z

compiler/rustc_mir_transform/src/gvn.rs

+                        }
+                        break;
+                    }
+                }


Can all this go in simplify_unary?

Took some more rejiggering, because it couldn't with its existing signature, but done.

cjgillot · 2024-06-16T14:15:43Z

compiler/rustc_mir_transform/src/gvn.rs

+                            from,
+                            to,
+                        } = value
+                            && from.builtin_deref(true) == to.builtin_deref(true)


Could you add a comment for this check?
Should we handle the case of both being slices of different element types?

Apparently there's a (relatively) new helper that makes this plausible; yay! Done, and added new negative tests to make sure that metadata-changing casts don't get picked up by it.

compiler/rustc_mir_transform/src/gvn.rs

RalfJung · 2024-06-16T20:17:33Z

compiler/rustc_mir_transform/src/validate.rs

                            // so earlier things can just `bug!` on it.
                            self.fail(location, "PtrMetadata should be in runtime MIR only");
                        }

-                        check_kinds!(a, "Cannot PtrMetadata non-pointer type {:?}", ty::RawPtr(..));


The doc comment should also be updated.

Thanks; good callout. Done.

EDIT: Oh, and editing that file is what triggers the "hey someone changed MIR" pings :)

rustbot · 2024-06-17T01:32:45Z

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

bors · 2024-06-20T00:51:34Z

☔ The latest upstream changes (presumably #126308) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot

Some nits on the mir-opt changes. r=me on those when the mir semantics addition is approved.

cjgillot · 2024-06-20T10:17:53Z

tests/mir-opt/gvn.rs

+    std::intrinsics::ptr_metadata(x)
+}
+
+#[custom_mir(dialect = "runtime")]


This test is a bit tricky, could you add a small explanation why each case must work as shown?

cjgillot · 2024-06-20T10:25:22Z

compiler/rustc_mir_transform/src/gvn.rs

+                        if from.builtin_deref(true)?.ptr_metadata_ty_or_tail(self.tcx, |t| t)
+                            == to.builtin_deref(true)?.ptr_metadata_ty_or_tail(self.tcx, |t| t) =>


Should we pass a normalization routine with our param_env? Or is it just being pedantic?

I'll add a FIXME to consider it. I'm confident we don't need it, as if something needs normalizing and it doesn't happen we'll just not do this optimization.

Maybe it'd trigger sometimes, though? But it might also be that the types we get from the Cast are already about as normalized as we can do in a MIR pass anyway, since we don't have a monomorphization context and thus might still end up with projections or whatever...

cjgillot · 2024-06-20T10:26:23Z

compiler/rustc_mir_transform/src/gvn.rs

+                    ..
+                },
+            ) if let ty::Slice(..) = to.builtin_deref(true).unwrap().kind()
+                && let ty::Array(_, len) = from.builtin_deref(true).unwrap().kind() =>


Nit: why do we unwrap() the result of builtin_deref here, while we ? earlier?

Should probably be an unwrap again. I'll change it; I'm just sad because rustfmt makes it really ugly.

RalfJung · 2024-06-20T10:28:55Z

Allow references as arguments to UnOp::PtrMetadata

This seems entirely uncontroversial to me from a MIR semantics / opsem perspective, so fine for me.

No behaviour changes.

`PtrMetadata` doesn't care about `*const`/`*mut`/`&`/`&mut`, so GVN away those casts in its argument. This includes updating MIR to allow calling PtrMetadata on references too, not just raw pointers. That means that `[T]::len` can be just `_0 = PtrMetadata(_1)`, for example. # Conflicts: # tests/mir-opt/pre-codegen/slice_index.slice_get_unchecked_mut_range.PreCodegen.after.panic-abort.mir # tests/mir-opt/pre-codegen/slice_index.slice_get_unchecked_mut_range.PreCodegen.after.panic-unwind.mir

GVN is actually on in release, and covers all the same things (or more), with `LowerSliceLen` changed to produce `PtrMetadata`.

scottmcm · 2024-06-21T05:40:55Z

@bors r=cjgillot

bors · 2024-06-21T05:40:58Z

📌 Commit 55d1337 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2024-06-21T07:12:53Z

⌛ Testing commit 55d1337 with merge e32ea48...

bors · 2024-06-21T09:21:15Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing e32ea48 to master...

rust-timer · 2024-06-21T11:03:23Z

Finished benchmarking commit (e32ea48): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.0%	[-1.4%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-1.4%, 0.2%]	3

Max RSS (memory usage)

Results (primary 0.8%, secondary 3.9%)

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)	7.7%	[7.7%, 7.7%]	2
Regressions ❌ (secondary)	3.9%	[2.0%, 5.6%]	4
Improvements ✅ (primary)	-3.8%	[-5.5%, -0.5%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[-5.5%, 7.7%]	5

Cycles

Results (primary -1.4%)

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)	-	-	0
Improvements ✅ (primary)	-1.4%	[-1.5%, -1.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.4%	[-1.5%, -1.4%]	2

Binary size

Results (primary 0.0%, secondary 0.3%)

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.1%	[0.0%, 0.4%]	13
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	3
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.0%]	15
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.2%, 0.4%]	28

Bootstrap: 698.008s -> 696.377s (-0.23%)
Artifact size: 326.58 MiB -> 326.82 MiB (0.07%)

nnethercote · 2024-06-21T11:14:17Z

Some small wins and losses in each category, which roughly balance out.

@rustbot label: +perf-regression-triaged

rustbot assigned davidtwco Jun 16, 2024

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. labels Jun 16, 2024

scottmcm commented Jun 16, 2024

View reviewed changes

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 Jun 16, 2024

cjgillot self-assigned this Jun 16, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 16, 2024

cjgillot reviewed Jun 16, 2024

View reviewed changes

RalfJung reviewed Jun 16, 2024

View reviewed changes

scottmcm force-pushed the more-ptr-metadata-gvn branch from 72cda61 to 7d15cf2 Compare June 17, 2024 01:32

scottmcm force-pushed the more-ptr-metadata-gvn branch from 7d15cf2 to eec1b4b Compare June 17, 2024 05:27

scottmcm force-pushed the more-ptr-metadata-gvn branch from eec1b4b to 525b5c3 Compare June 20, 2024 05:27

cjgillot reviewed Jun 20, 2024

View reviewed changes

Add a try_as_constant+try_as_local helper

31d8696

No behaviour changes.

scottmcm force-pushed the more-ptr-metadata-gvn branch from 525b5c3 to 511bd8b Compare June 21, 2024 05:13

scottmcm added 3 commits June 20, 2024 22:16

Replace NormalizeArrayLen with GVN

b611b6b

GVN is actually on in release, and covers all the same things (or more), with `LowerSliceLen` changed to produce `PtrMetadata`.

I'd never even heard of a coverage map

4341cb7

[GVN] Add tests for generic pointees with PtrMetadata

55d1337

scottmcm force-pushed the more-ptr-metadata-gvn branch from 511bd8b to 55d1337 Compare June 21, 2024 05:17

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 Jun 21, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 21, 2024

bors merged commit e32ea48 into rust-lang:master Jun 21, 2024
7 checks passed

rustbot added this to the 1.81.0 milestone Jun 21, 2024

scottmcm deleted the more-ptr-metadata-gvn branch June 21, 2024 09:22

bors mentioned this pull request Jun 21, 2024

Ignore allocation bytes in some mir-opt tests #126502

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 21, 2024

scottmcm mentioned this pull request Jul 30, 2024

Apply "polymorphization at home" to RawVec #126793

Merged

scottmcm mentioned this pull request Sep 5, 2024

explain why Rvalue::Len still exists #129939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More ptr metadata gvn #126541

More ptr metadata gvn #126541

scottmcm commented Jun 16, 2024

rustbot commented Jun 16, 2024

scottmcm Jun 16, 2024

This comment has been minimized.

rustbot commented Jun 16, 2024

scottmcm commented Jun 16, 2024

This comment has been minimized.

bors commented Jun 16, 2024

bors commented Jun 16, 2024

This comment has been minimized.

rust-timer commented Jun 16, 2024

cjgillot Jun 16, 2024

scottmcm Jun 17, 2024

cjgillot Jun 16, 2024

scottmcm Jun 17, 2024

RalfJung Jun 16, 2024

scottmcm Jun 17, 2024 •

edited

Loading

rustbot commented Jun 17, 2024

bors commented Jun 20, 2024

cjgillot left a comment

cjgillot Jun 20, 2024

cjgillot Jun 20, 2024

scottmcm Jun 21, 2024

cjgillot Jun 20, 2024

scottmcm Jun 21, 2024

RalfJung commented Jun 20, 2024

scottmcm commented Jun 21, 2024

bors commented Jun 21, 2024

bors commented Jun 21, 2024

bors commented Jun 21, 2024

rust-timer commented Jun 21, 2024

nnethercote commented Jun 21, 2024

		if from.builtin_deref(true)?.ptr_metadata_ty_or_tail(self.tcx, \|t\| t)
		== to.builtin_deref(true)?.ptr_metadata_ty_or_tail(self.tcx, \|t\| t) =>

More ptr metadata gvn #126541

More ptr metadata gvn #126541

Conversation

scottmcm commented Jun 16, 2024

rustbot commented Jun 16, 2024

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Jun 16, 2024

scottmcm commented Jun 16, 2024

This comment has been minimized.

bors commented Jun 16, 2024

bors commented Jun 16, 2024

This comment has been minimized.

rust-timer commented Jun 16, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

rustbot commented Jun 17, 2024

bors commented Jun 20, 2024

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Jun 20, 2024

scottmcm commented Jun 21, 2024

bors commented Jun 21, 2024

bors commented Jun 21, 2024

bors commented Jun 21, 2024

rust-timer commented Jun 21, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Jun 21, 2024

scottmcm Jun 17, 2024 •

edited

Loading