Fold arithmetic identities in GVN by cjgillot · Pull Request #119670 · rust-lang/rust

cjgillot · 2024-01-06T17:51:21Z

Extracted from #111344

This PR implements a few arithmetic folds for unary and binary operations.
This should take care of the missed optimizations introduced by #116012.

rustbot · 2024-01-06T17:51:28Z

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2024-01-06T17:51:30Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

oli-obk · 2024-01-08T10:01:28Z

compiler/rustc_mir_transform/src/gvn.rs

+                let lhs = lhs?;
+                let rhs = rhs?;


you could do the ? op directly on the method calls above

Added a comment explaining why it is done this way.

oli-obk · 2024-01-08T10:01:34Z

compiler/rustc_mir_transform/src/gvn.rs

+                let lhs = lhs?;
+                let rhs = rhs?;


oli-obk · 2024-01-08T10:02:43Z

compiler/rustc_mir_transform/src/gvn.rs

+        // Represent the values as `Ok(bits)` or `Err(VnIndex)`.
+        let a = as_bits(lhs).ok_or(lhs);
+        let b = as_bits(rhs).ok_or(rhs);


Use Either instead of Result?

oli-obk · 2024-01-08T10:06:45Z

compiler/rustc_mir_transform/src/gvn.rs

+            (BinOp::Eq, a, b) if (a.is_ok() && b.is_ok()) || a == b => self.insert_bool(a == b),
+            (BinOp::Ne, a, b) if (a.is_ok() && b.is_ok()) || a == b => self.insert_bool(a != b),


why does it need to be ok for equality? wouldn't it also hold if they are the same VnIndex?

bors · 2024-01-16T14:32:20Z

☔ The latest upstream changes (presumably #119672) made this pull request unmergeable. Please resolve the merge conflicts.

oli-obk · 2024-01-16T22:28:50Z

@bors r+

bors · 2024-01-16T22:28:52Z

📌 Commit 0623a95 has been approved by oli-obk

It is now in the queue for this repository.

cjgillot · 2024-01-16T23:29:10Z

Blessed tests after rebasing.
@bors r=oli-obk

bors · 2024-01-16T23:29:13Z

📌 Commit 0cc2102 has been approved by oli-obk

It is now in the queue for this repository.

cjgillot · 2024-01-16T23:47:17Z

Perf effect is likely.
@bors rollup=never

Fold arithmetic identities in GVN Extracted from rust-lang#111344 This PR implements a few arithmetic folds for unary and binary operations. This should take care of the missed optimizations introduced by rust-lang#116012.

bors · 2024-01-17T02:01:48Z

⌛ Testing commit 0cc2102 with merge 232aeaf...

rust-log-analyzer · 2024-01-17T02:33:29Z

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

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

bors · 2024-01-17T02:34:00Z

💔 Test failed - checks-actions

cjgillot · 2024-01-17T10:33:17Z

Failure looks spurious.
@bors retry

bors · 2024-01-17T11:46:52Z

⌛ Testing commit 0cc2102 with merge 52790a9...

bors · 2024-01-17T13:59:57Z

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

rust-timer · 2024-01-17T15:17:44Z

Finished benchmarking commit (52790a9): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	1

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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-7.1%	[-11.0%, -0.1%]	3
Improvements ✅ (secondary)	-1.2%	[-1.5%, -0.7%]	3
All ❌✅ (primary)	-4.5%	[-11.0%, 3.2%]	4

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
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.3%	[-2.4%, -2.2%]	3
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 664.038s -> 665.924s (0.28%)
Artifact size: 308.36 MiB -> 308.39 MiB (0.01%)

ChrisDenton · 2024-01-31T18:15:38Z

Though this is a perf only fix, would T-compiler consider backporting?

cc @mati865

apiraino · 2024-02-01T20:06:11Z

Beta backport declined as per compiler team on Zulip, also comment from PR author.

backporting this it's a bit too risky

@rustbot label -beta-nominated

rustbot assigned wesleywiser Jan 6, 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 Jan 6, 2024

cjgillot added the A-mir-opt Area: MIR optimizations label Jan 6, 2024

oli-obk requested changes Jan 8, 2024

View reviewed changes

oli-obk assigned oli-obk and unassigned wesleywiser Jan 8, 2024

cjgillot force-pushed the gvn-arithmetic branch from 88f2fd8 to 61f3c06 Compare January 8, 2024 22:54

cjgillot added 3 commits January 16, 2024 22:20

Simplify binary ops.

666030c

Simplify unary operations.

5fc23ad

Simplify Len.

3c48243

cjgillot force-pushed the gvn-arithmetic branch from 61f3c06 to 0623a95 Compare January 16, 2024 22:21

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 16, 2024

cjgillot added 3 commits January 16, 2024 22:32

Do not read a scalar on a non-scalar layout.

22ed51e

Explain side-effects from simplify_operand.

166fe54

Expand match over binops.

0cc2102

cjgillot force-pushed the gvn-arithmetic branch from 0623a95 to 0cc2102 Compare January 16, 2024 22:34

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 17, 2024

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 17, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2024

bors merged commit 52790a9 into rust-lang:master Jan 17, 2024

rustbot added this to the 1.77.0 milestone Jan 17, 2024

cjgillot deleted the gvn-arithmetic branch January 17, 2024 16:26

deltragon mentioned this pull request Jan 17, 2024

RangeInclusive performance regression in beta + nightly #119643

Closed

ChrisDenton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 31, 2024

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

		(BinOp::Eq, a, b) if (a.is_ok() && b.is_ok()) \|\| a == b => self.insert_bool(a == b),
		(BinOp::Ne, a, b) if (a.is_ok() && b.is_ok()) \|\| a == b => self.insert_bool(a != b),

Uh oh!

Conversation

cjgillot commented Jan 6, 2024

Uh oh!

rustbot commented Jan 6, 2024

Uh oh!

rustbot commented Jan 6, 2024

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jan 16, 2024

Uh oh!

oli-obk commented Jan 16, 2024

Uh oh!

bors commented Jan 16, 2024

Uh oh!

cjgillot commented Jan 16, 2024

Uh oh!

bors commented Jan 16, 2024

Uh oh!

cjgillot commented Jan 16, 2024

Uh oh!

bors commented Jan 17, 2024

Uh oh!

rust-log-analyzer commented Jan 17, 2024

Uh oh!

bors commented Jan 17, 2024

Uh oh!

cjgillot commented Jan 17, 2024

Uh oh!

bors commented Jan 17, 2024

Uh oh!

bors commented Jan 17, 2024

Uh oh!

rust-timer commented Jan 17, 2024

Overall result: ✅ improvements - no action needed

Uh oh!

ChrisDenton commented Jan 31, 2024

Uh oh!

apiraino commented Feb 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants