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

[libs] Simplify unchecked_{shl,shr} #112724

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

scottmcm
Copy link
Member

There's no need for the const_eval_select dance here. And while I originally wrote the .try_into().unwrap_unchecked() implementation here, it's kinda a mess in MIR -- this new one is substantially simpler, as shown by the old one being above the inlining threshold but the new one being below it in the mir-opt/inline/unchecked_shifts tests.

We don't need u32::checked_shl doing a dance through both Result and Option 🙃

@rustbot
Copy link
Collaborator

rustbot commented Jun 16, 2023

r? @Mark-Simulacrum

(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-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 16, 2023
@rust-log-analyzer

This comment has been minimized.

There's no need for the `const_eval_select` dance here.  And while I originally wrote the `.try_into().unwrap_unchecked()` implementation here, it's kinda a mess in MIR -- this new one is substantially simpler, as shown by the old one being above the inlining threshold but the new one being below it.
@scottmcm scottmcm force-pushed the simpler-unchecked-shifts branch from ea585d1 to 3ec4eed Compare June 17, 2023 01:01
@Mark-Simulacrum
Copy link
Member

r=me unless we want to rerun perf, but probably not needed.

@scottmcm
Copy link
Member Author

The unchecked shift methods are unstable and look like they're never used in the compiler, so it seems highly unlikely that this would materially affect perf -- TBH I'm more interested in the library code cleanup (less const_eval_select) than the MIR changes.

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jun 17, 2023

📌 Commit 3ec4eed has been approved by Mark-Simulacrum

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 Jun 17, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 18, 2023
… r=Mark-Simulacrum

[libs] Simplify `unchecked_{shl,shr}`

There's no need for the `const_eval_select` dance here.  And while I originally wrote the `.try_into().unwrap_unchecked()` implementation here, it's kinda a mess in MIR -- this new one is substantially simpler, as shown by the old one being above the inlining threshold but the new one being below it in the `mir-opt/inline/unchecked_shifts` tests.

We don't need `u32::checked_shl` doing a dance through both `Result` *and* `Option` 🙃
@bors
Copy link
Contributor

bors commented Jun 19, 2023

⌛ Testing commit 3ec4eed with merge 8d1fa47...

@bors
Copy link
Contributor

bors commented Jun 19, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 8d1fa47 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 19, 2023
@bors bors merged commit 8d1fa47 into rust-lang:master Jun 19, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8d1fa47): 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.7% [-0.7%, -0.6%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.7% [-0.7%, -0.6%] 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)
- - 0
Regressions ❌
(secondary)
3.7% [3.7%, 3.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.7% [-2.7%, -2.7%] 1
All ❌✅ (primary) - - 0

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

Bootstrap: 656.978s -> 653.833s (-0.48%)

@scottmcm scottmcm deleted the simpler-unchecked-shifts branch June 19, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-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.

7 participants