[mir-opt] GVN some more transmute cases #133324

scottmcm · 2024-11-22T03:24:53Z

We already did Transmute-then-PtrToPtr; this adds the nearly-identical PtrToPtr-then-Transmute.

It also adds transmute(Foo(x)) → transmute(x), when Foo is a single-field transparent type. That's useful for things like NonNull { pointer: p }.as_ptr(). It also detects when a Transmute is just an identity-for-the-value PtrCast between different raw pointer types, to help such things fold with other GVN passes.

Found these as I was looking at rust-lang/compiler-team#807-related changes. This also removes the questionably-useful "turn a transmute into a field projection" part of instsimplify (which I added ages ago without an obvious need for it) since that would just put back the field projections that MCP807 is trying to ban.

r? mir-opt

rustbot · 2024-11-22T03:25:03Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

saethlin · 2024-11-22T05:48:55Z

@bors try @rust-timer queue

…<try> [mir-opt] GVN some more transmute cases We already did `Transmute`-then-`PtrToPtr`; this adds the nearly-identical `PtrToPtr`-then-`Transmute`. It also adds `transmute(Foo(x))` → `transmute(x)`, when `Foo` is a single-field transparent type. That's useful for things like `NonNull { pointer: p }.as_ptr()`. Found these as I was looking at <https://github.com/rust-lang/compiler-team/issues/807>-related changes. r? mir-opt

bors · 2024-11-22T05:50:07Z

⌛ Trying commit fd92746 with merge c6b7165...

bors · 2024-11-22T07:34:30Z

☀️ Try build successful - checks-actions
Build commit: c6b7165 (c6b716561da31605db7d1abe0c4785449a0311b6)

rust-timer · 2024-11-22T08:53:20Z

Finished benchmarking commit (c6b7165): comparison URL.

Overall result: ❌✅ regressions and improvements - no 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.

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-1.9%, -1.9%]	1
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

Max RSS (memory usage)

Results (primary 0.8%, secondary 3.7%)

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)	6.5%	[4.7%, 7.7%]	3
Regressions ❌ (secondary)	3.7%	[3.7%, 3.7%]	1
Improvements ✅ (primary)	-3.5%	[-6.0%, -1.4%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[-6.0%, 7.7%]	7

Cycles

Results (primary -2.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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

Binary size

Results (primary -0.0%, secondary 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.3%]	8
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	1
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-0.6%, 0.3%]	11

Bootstrap: 796.222s -> 796.621s (0.05%)
Artifact size: 336.06 MiB -> 336.04 MiB (-0.01%)

compiler/rustc_mir_transform/src/gvn.rs

…projection, r=oli-obk Update `NonZero` and `NonNull` to not field-project (per MCP#807) rust-lang/compiler-team#807 (comment) was accepted, so this is the first PR towards moving the library to not using field projections into `[rustc_layout_scalar_valid_range_*]` types. `NonZero` was already using `transmute` nearly everywhere, so there are very few changes to it. `NonNull` needed more changes, but they're mostly simple, changing `.pointer` to `.as_ptr()`. r? libs cc rust-lang#133324, which will tidy up some of the MIR from this a bit more, but isn't a blocker.

Rollup merge of rust-lang#133651 - scottmcm:nonnull-nonzero-no-field-projection, r=oli-obk Update `NonZero` and `NonNull` to not field-project (per MCP#807) rust-lang/compiler-team#807 (comment) was accepted, so this is the first PR towards moving the library to not using field projections into `[rustc_layout_scalar_valid_range_*]` types. `NonZero` was already using `transmute` nearly everywhere, so there are very few changes to it. `NonNull` needed more changes, but they're mostly simple, changing `.pointer` to `.as_ptr()`. r? libs cc rust-lang#133324, which will tidy up some of the MIR from this a bit more, but isn't a blocker.

scottmcm · 2024-12-05T07:11:59Z

@bors try @rust-timer queue

…<try> [mir-opt] GVN some more transmute cases We already did `Transmute`-then-`PtrToPtr`; this adds the nearly-identical `PtrToPtr`-then-`Transmute`. It also adds `transmute(Foo(x))` → `transmute(x)`, when `Foo` is a single-field transparent type. That's useful for things like `NonNull { pointer: p }.as_ptr()`. Found these as I was looking at <https://github.com/rust-lang/compiler-team/issues/807>-related changes. r? mir-opt

bors · 2024-12-05T07:13:11Z

⌛ Trying commit 331a2fc with merge 6165d3e...

bors · 2024-12-05T08:56:01Z

☀️ Try build successful - checks-actions
Build commit: 6165d3e (6165d3ede89a29f49c121a6015daed14efa3e9f3)

rust-timer · 2024-12-05T10:36:18Z

Finished benchmarking commit (6165d3e): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.7%, 1.3%]	2
Regressions ❌ (secondary)	0.2%	[0.2%, 0.3%]	8
Improvements ✅ (primary)	-0.6%	[-0.9%, -0.3%]	6
Improvements ✅ (secondary)	-0.4%	[-0.6%, -0.1%]	4
All ❌✅ (primary)	-0.2%	[-0.9%, 1.3%]	8

Max RSS (memory usage)

Results (primary 5.2%, secondary -3.7%)

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.2%, 12.9%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.7%	[-3.7%, -3.7%]	1
All ❌✅ (primary)	5.2%	[2.2%, 12.9%]	7

Cycles

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

Binary size

Results (primary -0.1%, secondary -0.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.1%	[0.0%, 0.6%]	30
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	3
Improvements ✅ (primary)	-0.4%	[-4.2%, -0.0%]	37
Improvements ✅ (secondary)	-0.7%	[-2.0%, -0.0%]	5
All ❌✅ (primary)	-0.1%	[-4.2%, 0.6%]	67

Bootstrap: 768.023s -> 765.446s (-0.34%)
Artifact size: 330.84 MiB -> 330.80 MiB (-0.01%)

cjgillot · 2025-01-07T13:40:06Z

compiler/rustc_mir_transform/src/gvn.rs

+            if let Transmute = kind
+                && let Value::Aggregate(_aggregate_ty, variant_idx, field_values) = self.get(value)
+                && let Some((field_idx, field_ty)) =
+                    self.value_is_all_in_one_field(from, *variant_idx)


Should we gate this on self.type_may_have_niche_of_interest_to_backend(from)? It seems we lose information when transmuting from a NonNull aggregate.

Interesting point!

You're right that this does conceptually lose information.

It turns out that we actually don't use this information in the backend today at all: https://rust.godbolt.org/z/1jWYeW4rv

I was going to blame that on my Avoid allocas in codegen for simple mir::Aggregate statements #123886, but even before that when we had the alloca there still wasn't any !nonnull metadata anywhere: https://rust.godbolt.org/z/xoj1G4WWj

I don't think we want to block this optimization just because the struct has a niche, because that niche would normally come from a field of the struct -- like blocking the optimization on #[repr(transparent)] struct MyBool(bool); isn't something we need to do despite that type having an interesting niche in its Scalar.

So I think what I'd propose here is to leave this optimization as-is, and say that if people want to actually take advantage of the niche for this, there should be another Ban field-projecting into [rustc_layout_scalar_valid_range_*] types compiler-team#807 -style proposal to also ban Aggregateing a rustc_layout_scalar_valid_range_* type, and require Transmuteing to it instead. That way the niche would be asserted on construction too, not just on read. (Of course, the library could also choose to use transmute without such an MCP requiring it.)

cjgillot · 2025-01-08T19:46:03Z

Thanks!
@bors r+

bors · 2025-01-08T19:46:06Z

📌 Commit 071ba52 has been approved by cjgillot

It is now in the queue for this repository.

…cjgillot [mir-opt] GVN some more transmute cases We already did `Transmute`-then-`PtrToPtr`; this adds the nearly-identical `PtrToPtr`-then-`Transmute`. It also adds `transmute(Foo(x))` → `transmute(x)`, when `Foo` is a single-field transparent type. That's useful for things like `NonNull { pointer: p }.as_ptr()`. It also detects when a `Transmute` is just an identity-for-the-value `PtrCast` between different raw pointer types, to help such things fold with other GVN passes. Found these as I was looking at <https://github.com/rust-lang/compiler-team/issues/807>-related changes. This also removes the questionably-useful "turn a transmute into a field projection" part of instsimplify (which I added ages ago without an obvious need for it) since that would just put back the field projections that MCP807 is trying to ban. r? mir-opt

bors · 2025-01-08T20:54:54Z

⌛ Testing commit 071ba52 with merge aa1c934...

bors · 2025-01-08T21:30:44Z

💔 Test failed - checks-actions

We already did `Transmute`-then-`PtrToPtr`; this adds the nearly-identical `PtrToPtr`-then-`Transmute`. It also adds `transmute(Foo(x))` → `transmute(x)`, when `Foo` is a single-field transparent type. That's useful for things like `NonNull { pointer: p }.as_ptr()`. Found these as I was looking at MCP807-related changes.

Co-authored-by: Oli Scherer <[email protected]>

…n-transmute

scottmcm · 2025-01-09T03:45:27Z

Rebased to pick up #135182
@bors r=cjgillot

bors · 2025-01-09T03:45:29Z

📌 Commit b421a56 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2025-01-09T08:04:48Z

⌛ Testing commit b421a56 with merge b6b8361...

bors · 2025-01-09T10:49:11Z

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

rust-timer · 2025-01-09T12:53:41Z

Finished benchmarking commit (b6b8361): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

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

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.1%, 1.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.6%	[-3.0%, -0.2%]	11
Improvements ✅ (secondary)	-0.3%	[-0.5%, -0.2%]	7
All ❌✅ (primary)	-0.3%	[-3.0%, 1.4%]	14

Max RSS (memory usage)

Results (primary 0.8%, secondary -0.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)	6.1%	[2.7%, 11.9%]	5
Regressions ❌ (secondary)	1.7%	[1.2%, 2.8%]	4
Improvements ✅ (primary)	-4.5%	[-5.7%, -3.1%]	5
Improvements ✅ (secondary)	-2.1%	[-3.1%, -1.3%]	5
All ❌✅ (primary)	0.8%	[-5.7%, 11.9%]	10

Cycles

Results (primary -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)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-3.0%, 2.3%]	2

Binary size

Results (primary -0.1%, secondary 0.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.2%	[0.0%, 1.0%]	17
Regressions ❌ (secondary)	1.6%	[0.0%, 6.5%]	4
Improvements ✅ (primary)	-0.2%	[-1.3%, -0.0%]	44
Improvements ✅ (secondary)	-0.3%	[-0.8%, -0.0%]	8
All ❌✅ (primary)	-0.1%	[-1.3%, 1.0%]	61

Bootstrap: 765.392s -> 762.547s (-0.37%)
Artifact size: 325.81 MiB -> 325.82 MiB (0.00%)

scottmcm · 2025-01-10T08:06:49Z

Oh, nice, there's a much clearer improvement (albeit still not purely green) than the pre-merge results in #133324 (comment)

rustbot assigned wesleywiser Nov 22, 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 Nov 22, 2024

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 Nov 22, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Nov 22, 2024

yotamofek reviewed Nov 23, 2024

View reviewed changes

compiler/rustc_mir_transform/src/gvn.rs Outdated Show resolved Hide resolved

scottmcm force-pushed the gvn-aggregate-transmute branch from fd92746 to 1de5951 Compare November 24, 2024 02:17

This was referenced Nov 29, 2024

Update NonZero and NonNull to not field-project (per MCP#807) #133651

Merged

Tracking issue for banning field projecting into [rustc_layout_scalar_valid_range_*] types (MCP807) #133652

Open

scottmcm force-pushed the gvn-aggregate-transmute branch 3 times, most recently from 3a9056e to 331a2fc Compare December 5, 2024 06:53

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 Dec 5, 2024

This comment has been minimized.

Sign in to view

cjgillot reviewed Jan 7, 2025

View reviewed changes

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 Jan 8, 2025

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 Jan 8, 2025

scottmcm and others added 5 commits January 8, 2025 18:46

Apply suggestions from code review

259c425

Co-authored-by: Oli Scherer <[email protected]>

Use layout information to detect transparent transmutes

03650dd

Refactor the cast-then-cast cases together, and support transmute-the…

293f8e8

…n-transmute

Make the aggregate-then-transmute handling more general

b421a56

scottmcm force-pushed the gvn-aggregate-transmute branch from 071ba52 to b421a56 Compare January 9, 2025 02:47

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 Jan 9, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 9, 2025

bors merged commit b6b8361 into rust-lang:master Jan 9, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 9, 2025

bors mentioned this pull request Jan 9, 2025

Add some track_caller info to precondition panics #129658

Draft

scottmcm deleted the gvn-aggregate-transmute branch January 9, 2025 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mir-opt] GVN some more transmute cases #133324

[mir-opt] GVN some more transmute cases #133324

scottmcm commented Nov 22, 2024 •

edited

Loading

rustbot commented Nov 22, 2024

saethlin commented Nov 22, 2024

This comment has been minimized.

bors commented Nov 22, 2024

bors commented Nov 22, 2024

This comment has been minimized.

rust-timer commented Nov 22, 2024

scottmcm commented Dec 5, 2024

This comment has been minimized.

bors commented Dec 5, 2024

bors commented Dec 5, 2024

This comment has been minimized.

rust-timer commented Dec 5, 2024

cjgillot Jan 7, 2025

scottmcm Jan 7, 2025 •

edited

Loading

cjgillot commented Jan 8, 2025

bors commented Jan 8, 2025

bors commented Jan 8, 2025

This comment has been minimized.

bors commented Jan 8, 2025

scottmcm commented Jan 9, 2025

bors commented Jan 9, 2025

bors commented Jan 9, 2025

bors commented Jan 9, 2025

rust-timer commented Jan 9, 2025

scottmcm commented Jan 10, 2025

[mir-opt] GVN some more transmute cases #133324

[mir-opt] GVN some more transmute cases #133324

Conversation

scottmcm commented Nov 22, 2024 • edited Loading

rustbot commented Nov 22, 2024

saethlin commented Nov 22, 2024

This comment has been minimized.

bors commented Nov 22, 2024

bors commented Nov 22, 2024

This comment has been minimized.

rust-timer commented Nov 22, 2024

Overall result: ❌✅ regressions and improvements - no action needed

scottmcm commented Dec 5, 2024

This comment has been minimized.

bors commented Dec 5, 2024

bors commented Dec 5, 2024

This comment has been minimized.

rust-timer commented Dec 5, 2024

Overall result: ❌✅ regressions and improvements - please read the text below

Choose a reason for hiding this comment

scottmcm Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

cjgillot commented Jan 8, 2025

bors commented Jan 8, 2025

bors commented Jan 8, 2025

This comment has been minimized.

bors commented Jan 8, 2025

scottmcm commented Jan 9, 2025

bors commented Jan 9, 2025

bors commented Jan 9, 2025

bors commented Jan 9, 2025

rust-timer commented Jan 9, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

scottmcm commented Jan 10, 2025

scottmcm commented Nov 22, 2024 •

edited

Loading

scottmcm Jan 7, 2025 •

edited

Loading