add exact bitshifts by Qelxiros · Pull Request #144342 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits1 Checks10 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

r? @tgross35

rustbot has assigned @tgross35.
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 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

Jul 23, 2025

Commenting here to avoid cluttering the closed ACP or the tracking issue.

Regarding the names, we preferred calling these "exact" shifts just like exact_div

impl iX/uX { // Shift succeeds if shift < Self::BITS and right shifting the result returns the original value. fn exact_shl(self, shift: u32) -> Option; unsafe fn exact_shl_unchecked(self, shift: u32) -> Self;

If we're naming the methods after exact_div shouldn't there be 3 functions:

**checked_**exact_shl for the version returning Option (ref 1)
exact_shl for panicking (ref 2)
unchecked_exact_shl for the unchecked unsafe version (ref 3)

Choose a reason for hiding this comment

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

Sorry for the delay here. A few requests:

"bits disagreeing with the resulting sign bit" isn't the most clear. Maybe something based on "if the sign bit would be flipped"?
The exact_sh[lr]_unchecked methods seem to be missing examples
The signed example should demonstrate both positive and negative
In the examples, use a value based on $Self::BITS rather than 129. It would also be good to assert that one value below this returns Some, for demonstrating the boundary
The edge cases are tricky here and should get some tests in library/coretests/tests/num/uint_macros.rs (and the int version).
Can these be made const? min/max will need to be replaced with rhs < ... && rhs < ..., but I feel like that's more clear anyway

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

Aug 15, 2025

If we're naming the methods after exact_div shouldn't there be 3 functions:

* `**checked_**exact_shl` for the version returning `Option` ([ref 1](https://doc.rust-lang.org/nightly/std/primitive.u8.html#method.checked_exact_div))

* `exact_shl` for panicking ([ref 2](https://doc.rust-lang.org/nightly/std/primitive.u8.html#method.exact_div))

* `unchecked_exact_shl` for the unchecked unsafe version ([ref 3](https://doc.rust-lang.org/nightly/std/primitive.u8.html#method.unchecked_exact_div))

@Qelxiros could you do the last one here? That one is pretty straightforward for naming consistency.

For the other two, they sound right to me but we do have the strict_* prefix as well (for slightly different purposes). Could you bring those up on the tracking issue? libs-api will need to weigh in.

This comment has been minimized.

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

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

Aug 21, 2025

📌 Commit f2e8807 has been approved by tgross35

It is now in the queue for this repository.

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 3, 2025

Approved since this looks good to me (thank you for the changes) and has been sitting. One request if you get to it before bors does: the tracking issue mentions

i.e. bitshifts that can be losslessly reversed.

which seems like a nice way to make this intuitive. I think it would be helpful to change the doc summary sentence to say something like that, then put the more specific detail (sign bit / shifting out / rhs >= Self::BITS) in a new paragraph.

(can also be done as a followup at some point)

Just realized that LLVM does have this behavior, and it was requested in the ACP rust-lang/libs-team#570 (comment).

Not going to r- since the implementation here is still valid, but we'll need a followup PR adding new intrinsics at some point (I'll make a note in the tracking issue)

Update looks good, thanks. Could you squash please? Didn't realize there were two commits here.

📌 Commit cefa74f has been approved by tgross35

It is now in the queue for this repository.

tgross35 added a commit to tgross35/rust that referenced this pull request

Sep 5, 2025

tgross35 added a commit to tgross35/rust that referenced this pull request

Sep 5, 2025

bors added a commit that referenced this pull request

Sep 5, 2025

Rollup of 6 pull requests

Successful merges:

#144342 (add exact bitshifts)
#145709 (Fix LoongArch C function ABI when passing/returning structs containing floats)
#146152 (Unify and deduplicate algebraic float tests)
#146186 (Update cc-rs to 1.2.33, and switch rustc_codegen_ssa to use find-msvc-tools)
#146207 (std: Implement WASIp2-specific stdio routines)
#146217 (fix ICE when suggesting ::new)

r? @ghost @rustbot modify labels: rollup

bors added a commit that referenced this pull request

Sep 5, 2025

Rollup of 5 pull requests

Successful merges:

#144342 (add exact bitshifts)
#145709 (Fix LoongArch C function ABI when passing/returning structs containing floats)
#146152 (Unify and deduplicate algebraic float tests)
#146207 (std: Implement WASIp2-specific stdio routines)
#146217 (fix ICE when suggesting ::new)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit that referenced this pull request

Sep 5, 2025

Rollup merge of #144342 - Qelxiros:exact-bitshifts, r=tgross35

add exact bitshifts

Tracking issue: #144336

cc @lolbinarycat

Hey bors this has already merged.

@bors r- retry

bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

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-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Sep 5, 2025

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request

Sep 9, 2025

Comment on lines +1462 to +1473

assert_unsafe_precondition!(
check_language_ub,
concat!(stringify!($SelfT), "::unchecked_exact_shl cannot shift out non-zero bits"),
(
zeros: u32 = self.leading_zeros(),
ones: u32 = self.leading_ones(),
rhs: u32 = rhs,
) => rhs < zeros \|
);

// SAFETY: this is guaranteed to be safe by the caller
unsafe { self.unchecked_shl(rhs) }

Choose a reason for hiding this comment

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

The use of check_language_ub here seems wrong. The following code has language UB if rhs >= BITS, but the condition you are checking is stronger than that. check_language_ub should only be used if a violation of the precondition will always lead to language UB, and that is not the case here.

Choose a reason for hiding this comment

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

@Qelxiros are you interested in putting up a PR adding the intrinsics mentioned at #144342 (comment)? The comment here could be fixed at the same time.

Choose a reason for hiding this comment

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

I am interested, but I'm not sure exactly how to do it. Do I need to add implementations to all the rustc_codegen_* crates, or just llvm? How exactly are intrinsics added? It looks like I need a combination of LLVMBuildShl/LLVMBuildShr and LLVMSetNUW/LLVMSetNSW/LLVMSetExact (although LLVMSetExact currently only exists in C++ code). However, I'm not sure where that goes or how to associate it with the appropriate symbol.

Choose a reason for hiding this comment

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

Ralf got these already in #146878, thanks for the interest though

Labels

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.