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

compiler-errors · 2025-02-02T19:12:24Z

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 ();
          /* ... */
          // 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 #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 #136361
Fixes #135997

rustbot · 2025-02-02T19:12:33Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

saethlin · 2025-02-02T20:28:58Z

@bors try @rust-timer queue

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

bors · 2025-02-02T20:30:08Z

⌛ Trying commit a607ae7 with merge eed322b...

bors · 2025-02-02T22:21:13Z

☀️ Try build successful - checks-actions
Build commit: eed322b (eed322b85b68295e2beb1c66cdc17c974034d271)

rust-timer · 2025-02-03T01:36:30Z

Finished benchmarking commit (eed322b): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 778.752s -> 778.669s (-0.01%)
Artifact size: 328.73 MiB -> 328.71 MiB (-0.01%)

compiler-errors · 2025-02-05T18:05:39Z

Anyone want to give this a review?

saethlin · 2025-02-05T19:32:51Z

I was deferring to others who know GVN better but I'll take care of this evening if nobody else

r? saethlin

saethlin · 2025-02-06T01:25:34Z

🧐 I see

@bors r+

I think this miscompile is on beta, right?

bors · 2025-02-06T01:25:37Z

📌 Commit a607ae7 has been approved by saethlin

It is now in the queue for this repository.

compiler-errors · 2025-02-06T01:27:26Z

This miscompilation is luckily not on beta, AFAICT. The current version is 1.86 which is also the milestone that the regressed PR (#133324) is tagged with.

@rustbot label: -beta-nominated

jieyouxu · 2025-02-06T08:11:11Z

@bors p=5 (threading between rollups)

…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

bors · 2025-02-06T09:36:53Z

⌛ Testing commit a607ae7 with merge 0aa0d6f...

compiler-errors · 2025-02-06T18:41:16Z

// EMIT_MIR_FOR_EACH_PANIC_STRATEGY strikes again

compiler-errors · 2025-02-06T23:01:37Z

@bors r=saethlin

bors · 2025-02-06T23:01:40Z

📌 Commit de7d4a8 has been approved by saethlin

It is now in the queue for this repository.

…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

bors · 2025-02-07T01:38:08Z

⌛ Testing commit de7d4a8 with merge 5d28b2e...

rust-log-analyzer · 2025-02-07T03:34:57Z

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2025-02-07T03:35:32Z

💔 Test failed - checks-actions

saethlin · 2025-02-07T03:55:12Z

This is #135867 and we have #136647 lined up to stabilize CI in the meantime

saethlin · 2025-02-07T03:55:42Z

@bors retry

bors · 2025-02-07T06:26:45Z

⌛ Testing commit de7d4a8 with merge 550e035...

bors · 2025-02-07T09:12:12Z

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

rust-timer · 2025-02-07T10:29:36Z

Finished benchmarking commit (550e035): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 1.1%, secondary 2.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)	3.4%	[1.3%, 5.4%]	2
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-1.2%	[-1.7%, -0.8%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[-1.7%, 5.4%]	4

Cycles

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 779.478s -> 778.095s (-0.18%)
Artifact size: 329.05 MiB -> 328.98 MiB (-0.02%)

scottmcm · 2025-02-12T17:04:24Z

compiler/rustc_mir_transform/src/gvn.rs

@@ -1489,10 +1490,11 @@ impl<'body, 'tcx> VnState<'body, 'tcx> {
        }

        if was_ever_updated && let Some(op) = self.try_as_operand(value, location) {
-            *operand = op;
+            *initial_operand = op;
+            *initial_kind = kind;


Ah, moving these here is smart. Nice job!

Thanks for picking this up, compiler-errors.

…compiler-errors Emit MIR for each bit with on `dont_reset_cast_kind_without_updating_operand` PR rust-lang#136450 introduced a diff that includes a pointer-sized alloc. This doesn't cause any problems on the compiler test suite but it affects the test suite that ferrocene has for `aarch64-unknown-none` as the snapshot of the diff only includes a 32-bit alloc even though this should be a 64-bit alloc on `aarch64-unknown-none`. r? `@compiler-errors`

…compiler-errors Emit MIR for each bit with on `dont_reset_cast_kind_without_updating_operand` PR rust-lang#136450 introduced a diff that includes a pointer-sized alloc. This doesn't cause any problems on the compiler test suite but it affects the test suite that ferrocene has for `aarch64-unknown-none` as the snapshot of the diff only includes a 32-bit alloc even though this should be a 64-bit alloc on `aarch64-unknown-none`. r? ``@compiler-errors``

Rollup merge of rust-lang#137007 - pvdrz:fix-aarch64-alloc-layout, r=compiler-errors Emit MIR for each bit with on `dont_reset_cast_kind_without_updating_operand` PR rust-lang#136450 introduced a diff that includes a pointer-sized alloc. This doesn't cause any problems on the compiler test suite but it affects the test suite that ferrocene has for `aarch64-unknown-none` as the snapshot of the diff only includes a 32-bit alloc even though this should be a 64-bit alloc on `aarch64-unknown-none`. r? ``@compiler-errors``

rustbot assigned scottmcm Feb 2, 2025

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 Feb 2, 2025

compiler-errors changed the title ~~Simplify cast~~ Don't reset cast kind without also updating the operand in simplify_cast in GVN Feb 2, 2025

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 Feb 2, 2025

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 Feb 3, 2025

rustbot assigned saethlin and unassigned scottmcm Feb 5, 2025

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 Feb 6, 2025

rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 6, 2025

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 6, 2025

compiler-errors added 2 commits February 6, 2025 18:36

Failing test

c215e80

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

de7d4a8

compiler-errors force-pushed the simplify-cast branch from 269340f to de7d4a8 Compare February 6, 2025 18:39

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2025

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 Feb 7, 2025

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 Feb 7, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 7, 2025

bors merged commit 550e035 into rust-lang:master Feb 7, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Feb 7, 2025

scottmcm reviewed Feb 12, 2025

View reviewed changes

pvdrz mentioned this pull request Feb 14, 2025

Emit MIR for each bit with on dont_reset_cast_kind_without_updating_operand #137007

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

compiler-errors commented Feb 2, 2025 •

edited

Loading

rustbot commented Feb 2, 2025

saethlin commented Feb 2, 2025

This comment has been minimized.

bors commented Feb 2, 2025

bors commented Feb 2, 2025

This comment has been minimized.

rust-timer commented Feb 3, 2025

compiler-errors commented Feb 5, 2025

saethlin commented Feb 5, 2025

saethlin commented Feb 6, 2025

bors commented Feb 6, 2025

compiler-errors commented Feb 6, 2025

jieyouxu commented Feb 6, 2025

bors commented Feb 6, 2025

compiler-errors commented Feb 6, 2025

compiler-errors commented Feb 6, 2025

bors commented Feb 6, 2025

bors commented Feb 7, 2025

rust-log-analyzer commented Feb 7, 2025

bors commented Feb 7, 2025

saethlin commented Feb 7, 2025

saethlin commented Feb 7, 2025

bors commented Feb 7, 2025

bors commented Feb 7, 2025

rust-timer commented Feb 7, 2025

scottmcm Feb 12, 2025

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

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

Conversation

compiler-errors commented Feb 2, 2025 • edited Loading

rustbot commented Feb 2, 2025

saethlin commented Feb 2, 2025

This comment has been minimized.

bors commented Feb 2, 2025

bors commented Feb 2, 2025

This comment has been minimized.

rust-timer commented Feb 3, 2025

Overall result: no relevant changes - no action needed

compiler-errors commented Feb 5, 2025

saethlin commented Feb 5, 2025

saethlin commented Feb 6, 2025

bors commented Feb 6, 2025

compiler-errors commented Feb 6, 2025

jieyouxu commented Feb 6, 2025

bors commented Feb 6, 2025

compiler-errors commented Feb 6, 2025

compiler-errors commented Feb 6, 2025

bors commented Feb 6, 2025

bors commented Feb 7, 2025

rust-log-analyzer commented Feb 7, 2025

bors commented Feb 7, 2025

saethlin commented Feb 7, 2025

saethlin commented Feb 7, 2025

bors commented Feb 7, 2025

bors commented Feb 7, 2025

rust-timer commented Feb 7, 2025

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

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

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

compiler-errors commented Feb 2, 2025 •

edited

Loading