-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Enable u64
limbs in core::num::bignum
#146277
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
base: master
Are you sure you want to change the base?
Conversation
I’m pretty sure uncommenting this doesn’t actually do anything unless you also update the |
Thanks, I wanted to hear something like this, but I might need little bit more explanations about this
Do I have to replace this pub type Digit32 = u32;
define_bignum!(Big32x40: type=Digit32, n=40); And update this name in other places like imports and tests? |
I don’t know if I can provide much guidance here. I haven’t touched this code in about ten years and the parts I’ve worked on have almost entirely been replaced by now. The only thing I can tell you is that simply replacing the existing u32 limbs with u64 limbs will probably be a performance regression on 32 bit targets. A straight replacement is probably easier to try out (to see if it gives any performance gains) but a version that can happily be merged will probably have to grapple with using different bignum types based on some |
I like your idea with |
It used to be used in the slow paths of dec2flt and flt2dec. From a quick code search on GitHub, only flt2dec is still using it. I don’t know off-hand how to find a benchmark that takes that particular code path. |
I tried a full replacement locally and saw the extensive fallout in tests and constants. It's a much larger change than I initially thought. Given the scope and the risk of regression on 32-bit, I'll keep this PR minimal and just uncommenting the line to clean up the obsolete comment. The architectural shift to |
Since
u128
is stable now, I guess, we can safely add this, not sure if this requires tests or anythingCloses #137887