GVN: Invalidate derefs at loop headers #147607

dianqk · 2025-10-12T12:18:58Z

Fix a miscompiled case I found when re-reviewing #132527.

r? cjgillot
r? oli-obk

rustbot · 2025-10-12T12:19:02Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

dianqk · 2025-10-12T12:20:13Z

I want to backport the first two commits.

@rustbot label beta-nominated

cjgillot

Thanks for fixing this. I had a more brutal solution in mind, yours preserves more opts.
r=me with some nits

View changes since this review

cjgillot · 2025-10-12T15:22:39Z

compiler/rustc_middle/src/mir/mod.rs

+#[path = "loop.rs"]
+pub mod loop_;


Suggested change

#[path = "loop.rs"]

pub mod loop_;

pub mod loops;

To avoid having to use a path attribute.

cjgillot · 2025-10-12T15:23:33Z

compiler/rustc_mir_transform/src/gvn.rs


        let reverse_postorder = body.basic_blocks.reverse_postorder().to_vec();
        for bb in reverse_postorder {
+            // N.B. With loops, reverse postorder cannot produce a valid topological order.


Do you mind expanding the comment a bit?

Sorry, but I was expecting something like:

// A statement or terminator from inside the loop, that is not processed yet, may have performed an indirect write.

jackh726 · 2025-10-12T16:04:34Z

Just for clarity re. backport: this has been broken since 1.88? What's the benefit of beta backporting?

cjgillot · 2025-10-13T22:18:31Z

compiler/rustc_mir_transform/src/gvn.rs


        let reverse_postorder = body.basic_blocks.reverse_postorder().to_vec();
        for bb in reverse_postorder {
+            // N.B. With loops, reverse postorder cannot produce a valid topological order.


Sorry, but I was expecting something like:

// A statement or terminator from inside the loop, that is not processed yet, may have performed an indirect write.

cjgillot · 2025-10-13T22:19:29Z

compiler/rustc_mir_transform/src/gvn.rs

+            //             | bb3  |  |
+            //             | *x=c | -+
+            //             +------+
+            if maybe_loop {


Would it be enough to check maybe_loop_headers.contains(bb)?

I think so, maybe I'm too conservative.

cjgillot · 2025-10-14T02:12:01Z

Thanks!
@bors r+

bors · 2025-10-14T02:12:03Z

📌 Commit 2048b9c has been approved by cjgillot

It is now in the queue for this repository.

GVN: Invalidate derefs at loop headers Fix a miscompiled case I found when re-reviewing rust-lang#132527. r? cjgillot r? oli-obk

Rollup of 11 pull requests Successful merges: - #146277 (Enable `u64` limbs in `core::num::bignum`) - #146976 (constify basic Clone impls) - #147249 (Do two passes of `handle_opaque_type_uses_next`) - #147266 (fix 2 search graph bugs) - #147468 (Implement fs api set_times and set_times_nofollow) - #147497 (`proc_macro` cleanups (3/N)) - #147594 (std: implement `pal::os::exit` for VEXos) - #147596 (Adjust the Arm targets in CI to reflect latest changes) - #147607 (GVN: Invalidate derefs at loop headers) - #147620 (Avoid redundant UB check in RangeFrom slice indexing) - #147647 (Hide vendoring and copyright in GHA group) r? `@ghost` `@rustbot` modify labels: rollup

GVN: Invalidate derefs at loop headers Fix a miscompiled case I found when re-reviewing rust-lang#132527. r? cjgillot r? oli-obk

Rollup of 12 pull requests Successful merges: - #146277 (Enable `u64` limbs in `core::num::bignum`) - #146976 (constify basic Clone impls) - #147249 (Do two passes of `handle_opaque_type_uses_next`) - #147266 (fix 2 search graph bugs) - #147497 (`proc_macro` cleanups (3/N)) - #147546 (Suppress unused_parens for labeled break) - #147548 (Fix ICE for never pattern as closure parameters) - #147594 (std: implement `pal::os::exit` for VEXos) - #147596 (Adjust the Arm targets in CI to reflect latest changes) - #147607 (GVN: Invalidate derefs at loop headers) - #147620 (Avoid redundant UB check in RangeFrom slice indexing) - #147647 (Hide vendoring and copyright in GHA group) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #147607 - dianqk:gvn-deref-loop, r=cjgillot GVN: Invalidate derefs at loop headers Fix a miscompiled case I found when re-reviewing #132527. r? cjgillot r? oli-obk

saethlin · 2025-10-14T21:48:40Z

@rust-timer build 173ae98

rust-timer · 2025-10-14T23:51:17Z

Finished benchmarking commit (173ae98): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

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

Max RSS (memory usage)

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

Cycles

Results (primary 2.7%, secondary -2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Binary size

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

Bootstrap: 475.635s -> 474.033s (-0.34%)
Artifact size: 388.48 MiB -> 388.48 MiB (0.00%)

bors · 2025-10-15T09:15:36Z

⌛ Testing commit 2048b9c with merge 5413f7d...

dianqk · 2025-10-15T09:18:26Z

@bors r-

matthiaskrgr · 2025-10-15T09:31:26Z

@bors retry r-

apiraino · 2025-10-16T14:21:42Z

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

@rustbot label +beta-accepted

[beta] backports - Change int-to-ptr transmute lowering back to inttoptr #147541 - rewrite outlives placeholder constraints to outlives static when handling opaque types #147566 - GVN: Invalidate derefs at loop headers #147607 r? cuviper

Add miscompiled test cases

64c023b

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 Oct 12, 2025

rustbot assigned cjgillot Oct 12, 2025

rustbot added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 12, 2025

dianqk force-pushed the gvn-deref-loop branch from 3b879e2 to 76422fa Compare October 12, 2025 12:42

cjgillot reviewed Oct 12, 2025

View reviewed changes

dianqk force-pushed the gvn-deref-loop branch from 76422fa to ef51345 Compare October 13, 2025 13:07

cjgillot reviewed Oct 13, 2025

View reviewed changes

GVN: Invalidate derefs at loop headers

2048b9c

dianqk force-pushed the gvn-deref-loop branch from ef51345 to 2048b9c Compare October 14, 2025 00:11

dianqk changed the title ~~GVN: Invalidate derefs when a loop exists~~ GVN: Invalidate derefs at loop headers Oct 14, 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 Oct 14, 2025

Zalathar added a commit to Zalathar/rust that referenced this pull request Oct 14, 2025

Rollup merge of rust-lang#147607 - dianqk:gvn-deref-loop, r=cjgillot

23c518d

GVN: Invalidate derefs at loop headers Fix a miscompiled case I found when re-reviewing rust-lang#132527. r? cjgillot r? oli-obk

Zalathar mentioned this pull request Oct 14, 2025

Rollup of 11 pull requests #147659

Closed

Zalathar mentioned this pull request Oct 14, 2025

Rollup of 12 pull requests #147662

Merged

This was referenced Oct 14, 2025

Rollup of 10 pull requests #147663

Closed

Rollup of 10 pull requests #147661

Closed

bors merged commit 4fc3a05 into rust-lang:master Oct 14, 2025
10 checks passed

rustbot added this to the 1.92.0 milestone Oct 14, 2025

rust-timer added a commit that referenced this pull request Oct 14, 2025

Unrolled build for #147607

173ae98

Rollup merge of #147607 - dianqk:gvn-deref-loop, r=cjgillot GVN: Invalidate derefs at loop headers Fix a miscompiled case I found when re-reviewing #132527. r? cjgillot r? oli-obk

matthiaskrgr mentioned this pull request Oct 14, 2025

Rollup of 11 pull requests #147690

Closed

This comment has been minimized.

Sign in to view

dianqk deleted the gvn-deref-loop branch October 15, 2025 00:13

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 15, 2025

dianqk removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 15, 2025

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 16, 2025

cuviper mentioned this pull request Oct 16, 2025

[beta] backports #147791

Merged

cuviper modified the milestones: 1.92.0, 1.91.0 Oct 16, 2025

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 16, 2025

GVN: Invalidate derefs at loop headers #147607

GVN: Invalidate derefs at loop headers #147607

Uh oh!

Conversation

dianqk commented Oct 12, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 12, 2025

Uh oh!

dianqk commented Oct 12, 2025

Uh oh!

cjgillot left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackh726 commented Oct 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjgillot commented Oct 14, 2025

Uh oh!

bors commented Oct 14, 2025

Uh oh!

Uh oh!

saethlin commented Oct 14, 2025

Uh oh!

This comment has been minimized.

rust-timer commented Oct 14, 2025

Overall result: ✅ improvements - no action needed

Uh oh!

bors commented Oct 15, 2025

Uh oh!

dianqk commented Oct 15, 2025

Uh oh!

matthiaskrgr commented Oct 15, 2025

Uh oh!

apiraino commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

dianqk commented Oct 12, 2025 •

edited by rustbot

Loading

cjgillot left a comment •

edited by rustbot

Loading