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.

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

Remove SpecOptionPartialEq #122024

Merged
merged 2 commits into from
Mar 22, 2024
Merged

Conversation

clubby789
Copy link
Contributor

With the recent LLVM bump, the specialization for Option::partial_eq on types with niches is no longer necessary. I kept the manual implementation as it still gives us better codegen than the derive (will look at this seperately).

Also implemented PartialOrd/Ord by hand as it somewhat improves codegen for #49892: https://godbolt.org/z/vx5Y6oW4Y

@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2024

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 5, 2024
@clubby789 clubby789 added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Mar 5, 2024
@compiler-errors
Copy link
Member

@bors try @rust-timer queue

(are there codegen tests for this?)

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 5, 2024
@bors
Copy link
Contributor

bors commented Mar 5, 2024

⌛ Trying commit 36e048a with merge ce32709...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
…try>

Remove SpecOptionPartialEq

With the recent LLVM bump, the specialization for Option::partial_eq on types with niches is no longer necessary. I kept the manual implementation as it still gives us better codegen than the derive (will look at this seperately).

Also implemented PartialOrd/Ord by hand as it _somewhat_ improves codegen for rust-lang#49892: https://godbolt.org/z/vx5Y6oW4Y
@clubby789
Copy link
Contributor Author

tests/codegen/option-niche-eq.rs tests this (or at least the motivating case for types with niches). I also moved the cmp::Ordering assembly test into codegen as the assumes are now eliminated and the IR comes out neatly

@bors
Copy link
Contributor

bors commented Mar 5, 2024

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (ce32709): comparison URL.

Overall result: ✅ improvements - 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 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.4% [-0.4%, -0.4%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 2

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.0% [0.8%, 6.0%] 3
Regressions ❌
(secondary)
3.8% [2.1%, 6.1%] 14
Improvements ✅
(primary)
-3.6% [-7.1%, -0.0%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.4% [-7.1%, 6.0%] 5

Cycles

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

Binary size

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.1% [0.1%, 0.3%] 7
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.3%, -0.0%] 40
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) -0.1% [-0.3%, 0.3%] 47

Bootstrap: 643.644s -> 644.067s (0.07%)
Artifact size: 175.01 MiB -> 175.01 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 5, 2024
@jhpratt
Copy link
Member

jhpratt commented Mar 6, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 6, 2024

📌 Commit 36e048a has been approved by jhpratt

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 Mar 6, 2024
@bors
Copy link
Contributor

bors commented Mar 8, 2024

⌛ Testing commit 36e048a with merge 69d58cd...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2024
…hpratt

Remove SpecOptionPartialEq

With the recent LLVM bump, the specialization for Option::partial_eq on types with niches is no longer necessary. I kept the manual implementation as it still gives us better codegen than the derive (will look at this seperately).

Also implemented PartialOrd/Ord by hand as it _somewhat_ improves codegen for rust-lang#49892: https://godbolt.org/z/vx5Y6oW4Y
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 8, 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 Mar 8, 2024
@clubby789
Copy link
Contributor Author

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2024
@rust-cloud-vms rust-cloud-vms bot force-pushed the remove-spec-option-pe branch from 36e048a to 8e43732 Compare March 18, 2024 19:20
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 18, 2024
@bors
Copy link
Contributor

bors commented Mar 18, 2024

⌛ Testing commit 8e43732 with merge bdd82a7...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 18, 2024
…hpratt

Remove SpecOptionPartialEq

With the recent LLVM bump, the specialization for Option::partial_eq on types with niches is no longer necessary. I kept the manual implementation as it still gives us better codegen than the derive (will look at this seperately).

Also implemented PartialOrd/Ord by hand as it _somewhat_ improves codegen for rust-lang#49892: https://godbolt.org/z/vx5Y6oW4Y
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 18, 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 Mar 18, 2024
@clubby789
Copy link
Contributor Author

Since I made the PR, this seems to have stopped working 😕 Rebasing onto master makes this fail locally - I think there's been a change in the pre-optimized IR emitted.
@bors r-

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2024
@rust-cloud-vms rust-cloud-vms bot force-pushed the remove-spec-option-pe branch from 8e43732 to de4562f Compare March 19, 2024 13:23
@rust-cloud-vms rust-cloud-vms bot force-pushed the remove-spec-option-pe branch from de4562f to f8fd23a Compare March 19, 2024 16:32
@clubby789
Copy link
Contributor Author

@rustbot ready
Spelling out the match directly without a wildcard makes the opt work again in LLVM 18

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 19, 2024
@jhpratt
Copy link
Member

jhpratt commented Mar 21, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2024

📌 Commit f8fd23a has been approved by jhpratt

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 Mar 21, 2024
@scottmcm
Copy link
Member

Yay! So nice to have codegen tests that let people try removing hacks like this once they're no longer needed 🙂

@bors
Copy link
Contributor

bors commented Mar 22, 2024

⌛ Testing commit f8fd23a with merge cdb683f...

@bors
Copy link
Contributor

bors commented Mar 22, 2024

☀️ Test successful - checks-actions
Approved by: jhpratt
Pushing cdb683f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 22, 2024
@bors bors merged commit cdb683f into rust-lang:master Mar 22, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Mar 22, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cdb683f): comparison URL.

Overall result: ❌✅ regressions and 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)
1.0% [1.0%, 1.0%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) 1.0% [1.0%, 1.0%] 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)
4.9% [4.0%, 5.8%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.6% [-5.6%, -5.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.4% [-5.6%, 5.8%] 3

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

Binary size

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.3% [0.1%, 0.4%] 3
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.1% [-0.6%, -0.0%] 45
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 1
All ❌✅ (primary) -0.1% [-0.6%, 0.4%] 48

Bootstrap: 671.782s -> 667.777s (-0.60%)
Artifact size: 315.01 MiB -> 315.07 MiB (0.02%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. 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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants