Implement a global value numbering MIR optimization #109597

cjgillot · 2023-03-25T15:30:21Z

The aim of this pass is to avoid repeated computations by reusing past assignments. It is based on an analysis of SSA locals, in order to perform a restricted form of common subexpression elimination.

By opportunity, this pass allows for some simplifications by combining assignments. For instance, this pass could be able to see through projections of aggregates to directly reuse the aggregate field (not in this PR).

We handle references by assigning a different "provenance" index to each Ref/AddressOf rvalue. This ensure that we do not spuriously merge borrows that should not be merged. Meanwhile, we consider all the derefs of an immutable reference to a freeze type to give the same value:

_a = *_b // _b is &Freeze
_c = *_b // replaced by _c = _a

cjgillot · 2023-03-25T16:49:22Z

@bors try @rust-timer queue

bors · 2023-03-25T16:49:31Z

⌛ Trying commit 992e09cb207eabcae16c8babd86e56dca624f0e9 with merge cc3413ee04d8f6c966a2ccc47d9705ce40e6bec9...

bors · 2023-03-25T18:44:50Z

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

rust-timer · 2023-03-25T22:19:04Z

Finished benchmarking commit (cc3413ee04d8f6c966a2ccc47d9705ce40e6bec9): 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)	0.6%	[0.3%, 0.9%]	15
Regressions ❌ (secondary)	0.7%	[0.3%, 1.5%]	18
Improvements ✅ (primary)	-0.9%	[-2.6%, -0.4%]	17
Improvements ✅ (secondary)	-0.5%	[-1.0%, -0.1%]	8
All ❌✅ (primary)	-0.2%	[-2.6%, 0.9%]	32

Max RSS (memory usage)

Results

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.1%	[2.1%, 9.2%]	5
Regressions ❌ (secondary)	0.9%	[0.4%, 1.1%]	5
Improvements ✅ (primary)	-2.4%	[-7.9%, -0.4%]	7
Improvements ✅ (secondary)	-1.0%	[-3.4%, -0.4%]	14
All ❌✅ (primary)	0.7%	[-7.9%, 9.2%]	12

Cycles

Results

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.6%	[0.4%, 0.9%]	4
Improvements ✅ (primary)	-1.9%	[-2.7%, -1.2%]	2
Improvements ✅ (secondary)	-2.4%	[-3.3%, -1.2%]	6
All ❌✅ (primary)	-1.9%	[-2.7%, -1.2%]	2

rustbot · 2023-03-25T23:21:15Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

cjgillot · 2023-03-26T14:29:18Z

r? wg-mir-opt

davidtwco

This looks good to me but better to get another pair of eyes on it.

davidtwco · 2023-04-05T13:21:42Z

r? wg-mir-opt

bors · 2023-09-27T18:00:06Z

⌛ Testing commit 901be42 with merge bcadaa2...

Implement a global value numbering MIR optimization The aim of this pass is to avoid repeated computations by reusing past assignments. It is based on an analysis of SSA locals, in order to perform a restricted form of common subexpression elimination. By opportunity, this pass allows for some simplifications by combining assignments. For instance, this pass could be able to see through projections of aggregates to directly reuse the aggregate field (not in this PR). We handle references by assigning a different "provenance" index to each `Ref`/`AddressOf` rvalue. This ensure that we do not spuriously merge borrows that should not be merged. Meanwhile, we consider all the derefs of an immutable reference to a freeze type to give the same value: ```rust _a = *_b // _b is &Freeze _c = *_b // replaced by _c = _a ```

rust-log-analyzer · 2023-09-27T19:15:20Z

A job failed! Check out the build log: (web) (plain)

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

bors · 2023-09-27T19:15:47Z

💔 Test failed - checks-actions

cjgillot · 2023-09-27T19:26:19Z

@bors retry

bors · 2023-09-27T21:06:34Z

⌛ Testing commit 901be42 with merge e7c502d...

bors · 2023-09-27T22:55:34Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing e7c502d to master...

rust-timer · 2023-09-28T02:47:35Z

Finished benchmarking commit (e7c502d): 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

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)	-1.5%	[-1.5%, -1.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.5%	[-1.5%, -1.5%]	1

Cycles

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

Binary size

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

Bootstrap: 632.097s -> 631.31s (-0.12%)
Artifact size: 317.30 MiB -> 317.32 MiB (0.01%)

RalfJung · 2023-09-28T05:37:14Z

src/tools/miri/tests/pass/function_pointers.rs

+    assert!(return_fn_ptr(i) as unsafe fn() -> i32 == i as fn() -> i32 as unsafe fn() -> i32);
+    // We don't check anything for `f`. Miri gives it many different addresses
+    // but mir-opts can turn them into the same address.
+    let _val = return_fn_ptr(f) != f;


This is somewhat dubious. Certainly we want to test that we still successfully give them different addresses sometimes, e.g. by adding black_box.

But I am also not convinced that what GVN is doing here is sound. What is the argument for that?

I guess if this is something specific to fn-item to fn-ptr coercions, then what the opt does is fine -- that coercion is allowed to deduplicate.

But please fix the test by adding black_box and still asserting !=. Let's not reduce test coverage unnecessarily.

Implement constant propagation on top of MIR SSA analysis This implements the idea I proposed in rust-lang#110719 (comment) Based on rust-lang#109597 The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in rust-lang#109597. From this abstract representation, we can perform more involved simplifications, for example in rust-lang#111344. With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`. The most relevant commit is [Evaluated computed values to constants.](rust-lang@2767c49)" r? `@oli-obk`

Perform opportunistic simplifications during value numbering Based on rust-lang#109597 Opening mostly for discussion. In its current form, I think this pass does too much. I want to remove the const-propagation part to make it simpler.

Implement constant propagation on top of MIR SSA analysis This implements the idea I proposed in rust-lang/rust#110719 (comment) Based on rust-lang/rust#109597 The value numbering "GVN" pass formulates each rvalue that appears in MIR with an abstract form (the `Value` enum), and assigns an integer `VnIndex` to each. This abstract form can be used to deduplicate values, reusing an earlier local that holds the same value instead of recomputing. This part is proposed in #109597. From this abstract representation, we can perform more involved simplifications, for example in rust-lang/rust#111344. With the abstract representation `Value`, we can also attempt to evaluate each to a constant using the interpreter. This builds a `VnIndex -> OpTy` map. From this map, we can opportunistically replace an operand or a rvalue with a constant if their value has an associated `OpTy`. The most relevant commit is [Evaluated computed values to constants.](rust-lang/rust@2767c49)" r? `@oli-obk`

Perform opportunistic simplifications during value numbering ~Based on rust-lang#109597 ~Based on rust-lang#119439 Opening mostly for discussion.

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 Mar 25, 2023

cjgillot force-pushed the gvn branch 4 times, most recently from ffa6715 to 992e09c Compare March 25, 2023 16:33

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 Mar 25, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 25, 2023

cjgillot force-pushed the gvn branch from 992e09c to 0b0b1a2 Compare March 25, 2023 23:11

cjgillot changed the title ~~perf: Global value numbering MIR pass~~ Implement a global value numbering MIR optimization Mar 25, 2023

cjgillot marked this pull request as ready for review March 25, 2023 23:21

cjgillot force-pushed the gvn branch 3 times, most recently from 58d0ea1 to 8673f8a Compare March 26, 2023 09:43

rustbot assigned davidtwco Mar 26, 2023

cjgillot force-pushed the gvn branch from 8673f8a to 3db7c62 Compare April 1, 2023 19:00

davidtwco approved these changes Apr 5, 2023

View reviewed changes

rustbot assigned JakobDegen and unassigned davidtwco Apr 5, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 27, 2023

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 Sep 27, 2023

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 Sep 27, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 27, 2023

bors merged commit e7c502d into rust-lang:master Sep 27, 2023
11 checks passed

rustbot added this to the 1.74.0 milestone Sep 27, 2023

matthiaskrgr mentioned this pull request Sep 27, 2023

ICE: gvn: node .. is not reachable #116212

Closed

rustbot removed the perf-regression Performance regression. label Sep 28, 2023

RalfJung reviewed Sep 28, 2023

View reviewed changes

RalfJung mentioned this pull request Sep 28, 2023

Re-enable fn ptr inequality test rust-lang/miri#3090

Closed

cjgillot deleted the gvn branch September 28, 2023 11:14

cjgillot mentioned this pull request Dec 30, 2023

The panic branch of Some(x).unwrap() is not removed for some types #93011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a global value numbering MIR optimization #109597

Implement a global value numbering MIR optimization #109597

cjgillot commented Mar 25, 2023 •

edited

Loading

cjgillot commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

bors commented Mar 25, 2023

This comment has been minimized.

rust-timer commented Mar 25, 2023

rustbot commented Mar 25, 2023

cjgillot commented Mar 26, 2023

davidtwco left a comment

davidtwco commented Apr 5, 2023

bors commented Sep 27, 2023

rust-log-analyzer commented Sep 27, 2023

bors commented Sep 27, 2023

cjgillot commented Sep 27, 2023

bors commented Sep 27, 2023

bors commented Sep 27, 2023

rust-timer commented Sep 28, 2023

RalfJung Sep 28, 2023

RalfJung Sep 28, 2023

Implement a global value numbering MIR optimization #109597

Implement a global value numbering MIR optimization #109597

Conversation

cjgillot commented Mar 25, 2023 • edited Loading

cjgillot commented Mar 25, 2023

This comment has been minimized.

bors commented Mar 25, 2023

bors commented Mar 25, 2023

This comment has been minimized.

rust-timer commented Mar 25, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

rustbot commented Mar 25, 2023

cjgillot commented Mar 26, 2023

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented Apr 5, 2023

bors commented Sep 27, 2023

rust-log-analyzer commented Sep 27, 2023

bors commented Sep 27, 2023

cjgillot commented Sep 27, 2023

bors commented Sep 27, 2023

bors commented Sep 27, 2023

rust-timer commented Sep 28, 2023

Overall result: no relevant changes - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Mar 25, 2023 •

edited

Loading