Remove more PtrToPtr casts in GVN

[scottmcm]

This addresses two things I noticed in MIR:

1. NonNull::<T>::eq does (a as *mut T) == (b as *mut T), but it could just compare the *const Ts, so this removes PtrToPtr casts that are on both sides of a pointer comparison, so long as they're not fat-to-thin casts.

2. NonNull::<T>::addr does transmute::<_, usize>(p as *const ()), but so long as T: Thin that cast doesn't do anything, and thus we can directly transmute the *const T instead.

r? mir-opt

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

[saethlin]

Thanks! This is as usual more code than I wish it took to implement MIR transforms. But also I never thought of adding more casting cleanups to GVN. I'll re-assess some of the MIR in Vec after this PR lands to see if we're still missing more cleanup opportunities.

I'm going to send this to the benchmark queue on the off-chance that this produces a huge amount of more work for GVN or some other mir-opt pass to do. But if I see anything other than "whoa that's a lot of time in inside MIR transforms" I'll approve this when the perf run comes back.

@bors try @rust-timer queue

[bors]

⌛ Trying commit dd545e1 with merge 71e9c33...

[tgross35]

This is as usual more code than I wish it took to implement MIR transforms.

In theory, could MIR adopt something like Cranelift's isle to describe transformations?

[saethlin]

I don't think a project of that scale is worth it for MIR. I'd much rather see someone design an IR that's suited to implementing optimizations first, and co-design it with a DSL for writing optimizations.

Not that I have the kind of time/patience/expertise to do any of those things.

[bors]

☀️ Try build successful - checks-actions
Build commit: 71e9c33 (71e9c33ae6485059aad40069b551cfebed30a93b)

[scottmcm]

@tgross35 I'd love to have something like that. TBH the Value+VnIndex in GVN is already very similar to the types that ISLE uses, since it needs Copy stuff that can be looked up and matched on.

I do agree with @saethlin that it'd be nicer to have as SSA-primary IR on which to do this, though, rather than the GVN approximations to that.

[rust-timer]

Finished benchmarking commit (71e9c33): 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)	1.5%	[0.6%, 2.9%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-1.1%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-1.1%, 2.9%]	5

Max RSS (memory usage)

Results (primary 1.3%, secondary 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)	7.3%	[4.3%, 12.8%]	4
Regressions ❌ (secondary)	6.0%	[6.0%, 6.0%]	1
Improvements ✅ (primary)	-4.7%	[-10.9%, -0.8%]	4
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	1.3%	[-10.9%, 12.8%]	8

Cycles

Results (primary 0.4%, secondary -2.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)	2.0%	[1.2%, 2.8%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.3%, -0.9%]	2
Improvements ✅ (secondary)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	0.4%	[-1.3%, 2.8%]	4

Binary size

Results (primary -0.0%, secondary 0.1%)

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%]	37
Regressions ❌ (secondary)	0.2%	[0.1%, 0.5%]	43
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.0%]	37
Improvements ✅ (secondary)	-0.1%	[-0.4%, -0.0%]	8
All ❌✅ (primary)	-0.0%	[-0.5%, 0.6%]	74

Bootstrap: 693.553s -> 695.303s (0.25%)
Artifact size: 326.78 MiB -> 326.66 MiB (-0.04%)

[saethlin]

@bors r+

[bors]

📌 Commit dd545e1 has been approved by saethlin

It is now in the queue for this repository.

[cjgillot]

I wonder if we should also simplify PtrToPtr casts in deref projections too.

[bors]

⌛ Testing commit dd545e1 with merge d7c5937...

[bors]

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing d7c5937 to master...

[scottmcm]

@cjgillot That makes sense to me, though I guess we need to be careful about things like &mut*(p as *mut T) since I assume MIR still doesn't let you get a &mut to a dereferenced *const?

I would love to have a better way to write helpers for all of this, but haven't found a way yet. Because what you're describing here sounds very similar to set of things that the PtrMetadata GVN does -- removing casts because they're not important -- but everything always been just ever so slightly different between then and I've not found the right parameters to be able to pull out a bunch of shared checks :/

[rust-timer]

Finished benchmarking commit (d7c5937): 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)	1.3%	[0.3%, 2.9%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.1%, -0.4%]	2
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	0.6%	[-1.1%, 2.9%]	6

Max RSS (memory usage)

Results (primary 1.9%, secondary -2.1%)

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%	[2.1%, 12.0%]	6
Regressions ❌ (secondary)	5.0%	[5.0%, 5.0%]	1
Improvements ✅ (primary)	-8.1%	[-11.4%, -4.8%]	2
Improvements ✅ (secondary)	-5.7%	[-7.6%, -3.7%]	2
All ❌✅ (primary)	1.9%	[-11.4%, 12.0%]	8

Cycles

Results (primary 1.1%, secondary -3.8%)

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.9%	[1.0%, 2.3%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-3.8%	[-4.9%, -2.7%]	2
All ❌✅ (primary)	1.1%	[-1.1%, 2.3%]	4

Binary size

Results (primary -0.0%, secondary 0.1%)

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%]	35
Regressions ❌ (secondary)	0.2%	[0.1%, 0.5%]	43
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.0%]	37
Improvements ✅ (secondary)	-0.1%	[-0.4%, -0.0%]	8
All ❌✅ (primary)	-0.0%	[-0.5%, 0.6%]	72

Bootstrap: 694.171s -> 695.122s (0.14%)
Artifact size: 324.67 MiB -> 326.61 MiB (0.60%)

[scottmcm]

Lots of churn here. Fun stuff like ripgrep-opt-full being +2.33% cycles but -5.03% wall time :/

I think it's probably time to look more at exactly what we're MIR inlining, now that a bunch of PRs like this have shrunk things enough that they've started inlining more in MIR.

Maybe we want a different strategy that's less purely threshold based but tees it up to take more advantage of LLVM's inlining after ours...

[saethlin]

Maybe we want a different strategy that's less purely threshold based but tees it up to take more advantage of LLVM's inlining after ours...

What we really want is less total MIR post-mono. I wonder if we could start by doing some analysis of the MIR inliner decisions on codebases like ripgrep. I suspect some inlinings reduce total monomorphic MIR and some increase it. Some examples would be good. If all it comes down to is how much the caller is instantiated, that would be a bummer but maybe we could do a PGO sort of thing to the standard library MIR based on instantiations in the perf suite.

[pnkfelix]

Visiting for weekly rustc-perf triage

• Main primary regressions to opt-full benchmarks ripgrep (2.89%), webrender (1.11%), html5ever (0.70%).
• Some interesting discussion on the PR about the cause; e.g. are PR's like this causing individual MIR reduction that leads to more inlining and then more bloat overall?
• but I do not think any of that would cause us to undo this particular change; there are higher level inlining and code-generation policies that need to be revisited.
• marking as triaged.

@rustbot label: +perf-regression-triaged