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

Simplification of BigNum::bit_length #92747

Merged
merged 2 commits into from
Jan 15, 2022

Conversation

swenson
Copy link
Contributor

@swenson swenson commented Jan 10, 2022

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.

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.
@rust-highfive
Copy link
Collaborator

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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2022
@camelid camelid added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jan 10, 2022
@@ -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;
Copy link
Member

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;
}

Copy link
Contributor Author

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!

Copy link
Member

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:

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

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

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:

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.
@scottmcm
Copy link
Member

Thanks for adding those tests! Make me less scared about my off-by-one errors when there are pinning tests :)

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2022

📌 Commit 0589cac has been approved by scottmcm

@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 Jan 11, 2022
@swenson
Copy link
Contributor Author

swenson commented Jan 11, 2022

Thanks @scottmcm for the timely review!

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 15, 2022
…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
@bors bors merged commit f511360 into rust-lang:master Jan 15, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 15, 2022
@swenson swenson deleted the bignum-bit-length-optimization branch January 15, 2022 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants