add exact bitshifts

[Qelxiros]

Tracking issue: #144336

cc @lolbinarycat

[rustbot]

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

[kennytm]

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<Self>;
    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)

[tgross35]

Sorry for the delay here. A few requests:

1. "bits disagreeing with the resulting sign bit" isn't the most clear. Maybe something based on "if the sign bit would be flipped"?
2. The exact_sh[lr]_unchecked methods seem to be missing examples
3. The signed example should demonstrate both positive and negative
4. 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
5. The edge cases are tricky here and should get some tests in library/coretests/tests/num/uint_macros.rs (and the int version).
6. Can these be made const? min/max will need to be replaced with rhs < ... && rhs < ..., but I feel like that's more clear anyway

[tgross35]

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.

[rustbot]

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.

[Qelxiros]

@rustbot ready

[tgross35]

@bors r+

[bors]

📌 Commit f2e8807 has been approved by tgross35

It is now in the queue for this repository.

[tgross35]

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)

[tgross35]

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)

[tgross35]

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

[tgross35]

@bors r+

[bors]

📌 Commit cefa74f has been approved by tgross35

It is now in the queue for this repository.

[bors]

⌛ Testing commit cefa74f with merge c559c4a...

[Zalathar]

Hey bors this has already merged.

@bors r- retry

[RalfJung]

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.

[tgross35]

@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.

[Qelxiros]

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.

[tgross35]

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