[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%)

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)

Don't reset cast kind without also updating the operand in `simplify_cast` in GVN Consider this heavily elided segment of the pre-GVN example code that was committed as a test: ``` let _4: *const (); let _5: *const [()]; let mut _6: *const (); let _7: *mut (); let mut _8: *const [()]; let mut _9: std::boxed::Box<()>; let mut _10: *const (); /* ... */ _10 = copy ((_9.0: std::ptr::Unique<()>).0: std::ptr::NonNull<()>) as *const () (Transmute); _4 = copy _10; _6 = copy _4; _5 = *const [()] from (copy _6, copy _11); _8 = copy _5; _7 = copy _8 as *mut () (PtrToPtr); ``` A malformed optimization was changing `_7` to: ``` _7 = copy _5 as *mut () (Transmute); ``` (where `_8` was just replaced with `_5` bc of simple copy propagation, that part is not important... the CastKind changing to Transmute is the important part here). In rust-lang#133324, two new functionalities were implemented: * Peeking through unsized -> sized PtrToPtr casts whose operand is `AggregateKind::RawPtr`, to turn it into PtrToPtr casts of the base of the aggregate. In this case, this allows us to see that the value of `_7` is just a ptr-to-ptr cast of `_6`. * Folding a PtrToPtr cast of an operand which is a Transmute cast into just a single Transmute, which (theoretically) allows us to treat `_7` as a transmute into `*mut ()` of the base of the cast of `_10`, which is the place projection of `((_9.0: std::ptr::Unique<()>).0: std::ptr::NonNull<()>)`. However, when applying those two subsequent optimizations, we must *not* update the CastKind of the final cast *unless* we also update the operand of the cast, since the operand may no longer make sense with the updated CastKind. In this case, this is problematic because the type of `_8` is `*const [()]`, but that operand in assignment statement of `_7` does *not* get turned into something like `((_9.0: std::ptr::Unique<()>).0: std::ptr::NonNull<()>)` -- **in other words, `try_to_operand` fails** -- because GVN only turns value nodes into locals or consts, not projections of locals. So we fail to update the operand, but we still update the CastKind to Transmute, which means we now are transmuting types of different sizes (a wide pointer and a thin pointer). r? `@scottmcm` or `@cjgillot` Fixes rust-lang#136361

…thlin Don't reset cast kind without also updating the operand in `simplify_cast` in GVN Consider this heavily elided segment of the pre-GVN example code that was committed as a test: ```rust let _4: *const (); let _5: *const [()]; let mut _6: *const (); let _7: *mut (); let mut _8: *const [()]; let mut _9: std::boxed::Box<()>; let mut _10: *const (); /* ... */ // Deref a box _10 = copy ((_9.0: std::ptr::Unique<()>).0: std::ptr::NonNull<()>) as *const () (Transmute); _4 = copy _10; _6 = copy _4; // Inlined body of `slice::from_raw_parts`, to turn a unit pointer into a slice-of-unit pointer _5 = *const [()] from (copy _6, copy _11); _8 = copy _5; // Cast the raw slice-of-unit pointer back to a unit pointer _7 = copy _8 as *mut () (PtrToPtr); ``` A malformed optimization was changing `_7` (which casted the slice-of-unit ptr to a unit ptr) to: ``` _7 = copy _5 as *mut () (Transmute); ``` ...where `_8` was just replaced with `_5` bc of simple copy propagation, that part is not important... the CastKind changing to Transmute is the important part here. In rust-lang#133324, two new functionalities were implemented: * Peeking through unsized -> sized PtrToPtr casts whose operand is `AggregateKind::RawPtr`, to turn it into PtrToPtr casts of the base of the aggregate. In this case, this allows us to see that the value of `_7` is just a ptr-to-ptr cast of `_6`. * Folding a PtrToPtr cast of an operand which is a Transmute cast into just a single Transmute, which (theoretically) allows us to treat `_7` as a transmute into `*mut ()` of the base of the cast of `_10`, which is the place projection of `((_9.0: std::ptr::Unique<()>).0: std::ptr::NonNull<()>)`. However, when applying those two subsequent optimizations, we must *not* update the CastKind of the final cast *unless* we also update the operand of the cast, since the operand may no longer make sense with the updated CastKind. In this case, this is problematic because the type of `_8` is `*const [()]`, but that operand in assignment statement of `_7` does *not* get turned into something like `((_9.0: std::ptr::Unique<()>).0: std::ptr::NonNull<()>)` -- **in other words, `try_to_operand` fails** -- because GVN only turns value nodes into locals or consts, not projections of locals. So we fail to update the operand, but we still update the CastKind to Transmute, which means we now are transmuting types of different sizes (a wide pointer and a thin pointer). r? `@scottmcm` or `@cjgillot` Fixes rust-lang#136361 Fixes rust-lang#135997

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

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

durin42 mentioned this pull request Jan 31, 2025

Nightly regressed igvm crate and now emits SIGILL at opt-level higher than 1 #136361

Closed

compiler-errors mentioned this pull request Feb 2, 2025

Don't reset cast kind without also updating the operand in simplify_cast in GVN #136450

Merged

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

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

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