Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fold arithmetic identities in GVN #119670

Merged
merged 6 commits into from
Jan 17, 2024
Merged

Fold arithmetic identities in GVN #119670

merged 6 commits into from
Jan 17, 2024

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jan 6, 2024

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
Copy link
Collaborator

rustbot commented Jan 6, 2024

r? @wesleywiser

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

@rustbot 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
@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@cjgillot cjgillot added the A-mir-opt Area: MIR optimizations label Jan 6, 2024
oli-obk
oli-obk requested changes Jan 8, 2024
View reviewed changes
compiler/rustc_mir_transform/src/gvn.rs
Comment on lines +790 to +800
let lhs = lhs?;
let rhs = rhs?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment explaining why it is done this way.

compiler/rustc_mir_transform/src/gvn.rs
Comment on lines +801 to +813
let lhs = lhs?;
let rhs = rhs?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

compiler/rustc_mir_transform/src/gvn.rs Outdated
Comment on lines 933 to 935
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Either instead of Result?

compiler/rustc_mir_transform/src/gvn.rs Outdated
Comment on lines 982 to 983
(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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@oli-obk oli-obk assigned oli-obk and unassigned wesleywiser Jan 8, 2024
@bors
Copy link
Contributor

bors commented Jan 16, 2024

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

@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2024

📌 Commit 0623a95 has been approved by oli-obk

It is now in the queue for this repository.

@bors 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
Copy link
Contributor Author

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

@bors
Copy link
Contributor

bors commented Jan 16, 2024

📌 Commit 0cc2102 has been approved by oli-obk

It is now in the queue for this repository.

@cjgillot
Copy link
Contributor Author

Perf effect is likely.
@bors rollup=never

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2024
@bors
Auto merge of rust-lang#119670 - cjgillot:gvn-arithmetic, r=oli-obk
232aeaf 
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
Copy link
Contributor

bors commented Jan 17, 2024

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

@rust-log-analyzer
Copy link
Collaborator

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
Copy link
Contributor

bors commented Jan 17, 2024

💔 Test failed - checks-actions

@bors 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
@cjgillot
Copy link
Contributor Author

Failure looks spurious.
@bors retry

@bors 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
Copy link
Contributor

bors commented Jan 17, 2024

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

@bors
Copy link
Contributor

bors commented Jan 17, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2024
@bors bors merged commit 52790a9 into rust-lang:master Jan 17, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 17, 2024
@rust-timer
Copy link
Collaborator

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

@cjgillot cjgillot deleted the gvn-arithmetic branch January 17, 2024 16:26
@deltragon deltragon mentioned this pull request Jan 17, 2024
Closed
@ChrisDenton ChrisDenton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 31, 2024
@ChrisDenton
Copy link
Member

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

cc @mati865

@apiraino
Copy link
Contributor

apiraino commented Feb 1, 2024

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 rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk requested changes

Assignees

@oli-obk oli-obk

Labels
A-mir-opt Area: MIR optimizations merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.77.0
Development

Successfully merging this pull request may close these issues.
9 participants
@cjgillot @rustbot @bors @oli-obk @rust-log-analyzer @rust-timer @ChrisDenton @apiraino @wesleywiser