-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Simplification of BigNum::bit_length #92747
Simplification of BigNum::bit_length #92747
Conversation
As indicated in the comment, the BigNum::bit_length function could be optimized by using CLZ, which is often a single instruction instead a loop. I think the code is also simpler now without the loop. I added some additional tests for Big8x3 and Big32x40 to ensure that there were no regressions.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @scottmcm (or someone else) soon. Please see the contribution instructions for more information. |
library/core/src/num/bignum.rs
Outdated
@@ -162,20 +162,13 @@ macro_rules! define_bignum { | |||
let digits = self.digits(); | |||
let zeros = digits.iter().rev().take_while(|&&x| x == 0).count(); | |||
let end = digits.len() - zeros; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm new to this code, so let me know if I'm misunderstanding things. It took me a bit to figure out what was going on.)
I think this could be simplified further?
If I'm reading it right, I think this is something like
let digits = self.digits();
// Find the most significant non-zero digit
let msd = digits.iter().rposition(|&x| x != 0);
if let Some(msd) = msd {
let digitbits = <$ty>::BITS as usize;
msd * digitbits + digits[msd].log2() as usize
} else {
// There are no non-zero digits, i.e., the number is zero.
return 0;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the log2()
would be slower (more complex instruction that does a lot we don't need) than a CLZ, but the rest of the changes I can certainly incorporate. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log2
on integers is just BITS - CLZ
, though:
rust/library/core/src/num/uint_macros.rs
Lines 841 to 849 in 23ce5fc
pub const fn checked_log2(self) -> Option<u32> { | |
if self <= 0 { | |
None | |
} else { | |
// SAFETY: We just checked that this number is positive | |
let log = (Self::BITS - 1) - unsafe { intrinsics::ctlz_nonzero(self) as u32 }; | |
Some(log) | |
} | |
} |
(Oh, I might be off-by-one in the code. Up to you if you like ... + log2() + 1
or (msd + 1) * digitbits - leading_zeros()
or whatever best.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, my bad, I thought log2()
would return the actual logarithm base 2, not just the integer part. I'll totally use that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log2
on integers is justBITS - CLZ
, though:rust/library/core/src/num/uint_macros.rs
Lines 841 to 849 in 23ce5fc
pub const fn checked_log2(self) -> Option<u32> { if self <= 0 { None } else { // SAFETY: We just checked that this number is positive let log = (Self::BITS - 1) - unsafe { intrinsics::ctlz_nonzero(self) as u32 }; Some(log) } } (Oh, I might be off-by-one in the code. Up to you if you like
... + log2() + 1
or(msd + 1) * digitbits - leading_zeros()
or whatever best.)
Yep! We're always off-by-one the first time or two. :) I worked it out.
I went with a match
statement instead of if let
since it is one line shorter.
Thank you to @scottmcm for suggesting the handy `log2()` function.
Thanks for adding those tests! Make me less scared about my off-by-one errors when there are pinning tests :) @bors r+ |
📌 Commit 0589cac has been approved by |
Thanks @scottmcm for the timely review! |
…askrgr Rollup of 8 pull requests Successful merges: - rust-lang#92747 (Simplification of BigNum::bit_length) - rust-lang#92767 (Use the new language identifier for Rust in the PDB debug format) - rust-lang#92775 (Inline std::os::unix::ffi::OsStringExt methods) - rust-lang#92863 (Remove `&mut` from `io::read_to_string` signature) - rust-lang#92865 (Ignore static lifetimes for GATs outlives lint) - rust-lang#92873 (Generate more precise generator names) - rust-lang#92879 (Add Sync bound to allocator parameter in vec::IntoIter) - rust-lang#92892 (Do not fail evaluation in const blocks) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
As indicated in the comment, the BigNum::bit_length function could be
optimized by using CLZ, which is often a single instruction instead a
loop.
I think the code is also simpler now without the loop.
I added some additional tests for Big8x3 and Big32x40 to ensure that
there were no regressions.