Add functions for reversing the bit pattern in an integer #48573
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Amanieu
force-pushed
the
bitreverse2
branch
2 times, most recently
from
9467b30
to
616ea71
Compare
|
Should this also support |
src/libcore/num/mod.rs
Outdated
| #[cfg(not(stage0))] | ||
| #[inline] | ||
| pub fn reverse_bits(self) -> Self { | ||
| (self as $UnsignedT).swap_bytes() as Self |
There was a problem hiding this comment.
Should this be reverse_bits?
Yes these should all be supported. (I fixed the missing intrinsic for |
| /// # Examples | ||
| /// | ||
| /// Please note that this example is shared between integer types. | ||
| /// Which explains why `i16` is used here. |
There was a problem hiding this comment.
@GuillaumeGomez @QuietMisdreavus I feel like I've recently heard something about us being able to generate dedicated examples with the right types for primitives...
There was a problem hiding this comment.
I just copied the docs for swap_bytes and made minor adjustments.
There was a problem hiding this comment.
Well, in this case it is very complicated to be able to write a fully generic example.
| assert_eq!(bitreverse(0x0ABBCC0Eu32), 0x7033DD50); | ||
| assert_eq!(bitreverse(0x0ABBCC0Ei32), 0x7033DD50); | ||
| assert_eq!(bitreverse(0x0122334455667708u64), 0x10EE66AA22CC4480); | ||
| assert_eq!(bitreverse(0x0122334455667708i64), 0x10EE66AA22CC4480); |
There was a problem hiding this comment.
Could you add a test for u/i128? It would not surprise me at all if some LLVM backends have trouble with this intrinsic on 128 bit integers, so it would be good to have at least some test to demonstrate that it works at all. (And if it works at all, it's probably reasonably robust given how LLVM is structured.)
|
All comments have been addressed. I will create a tracking issue when this is approved. |
|
Ping from the release team @sfackler! This PR needs your review. |
|
There were some questions about use cases in the original PR - where would these functions be used? |
|
I need these because I am emulating an architecture that has a bit reverse instruction. I would like to be able to use native bit-reverse instructions if they are available. AFAIK LLVM won't optimize normal code into a native bit-reverse instruction, you need to use the intrinsic for that. |
|
LGTM on the API side, but I'm not super familiar with trans - @alexcrichton does that side of things look okay to you? |
|
Looks good! Want to open a tracking issue and this can be r+'d? |
|
Tracking issue created: #48763 |
|
@bors r+ |
|
📌 Commit 88aec91 has been approved by |
bors
removed
the
S-waiting-on-review
bors
added
the
S-waiting-on-bors
Add functions for reversing the bit pattern in an integer I'm reviving PR rust-lang#32798 now that the LLVM issues have been resolved. > This adds the bitreverse intrinsic and adds a reverse_bits function to all integer types.
|
@Amanieu aren't these exposed by stdsimd already? If not, could you fill an issue there mentioning which instructions you were missing for which architecture? |
|
I've opened an issue on |
|
They are only exposed as vendor-specific intrinsics in stdsimd. I wanted to add a generic solution that worked on all architectures. |
|
@Amanieu makes sense, just keep in mind that the performance of LLVM's generic bitreverse solution can vary greatly depending on the types involved, your target architecture, and what the rest of your code does around it: https://bugs.llvm.org/show_bug.cgi?id=31810 If your target has a bit reverse instruction (only ARMv8 as far as I know) then everything should be fine. |
I'm reviving PR #32798 now that the LLVM issues have been resolved.