-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Fix overflow in pow5 #47511
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
Fix overflow in pow5 #47511
Conversation
|
Too lazy to try and follow that reasoning, but does that also work if |
|
Good catch! No, the story is much worse then because the |
|
Brava, @LilithHafner 👏🏻 |
|
Unless folks object, I'm going to merge this. The reasoning justifying that the new behavior is always correct is complex, but it is pretty clear that this is at least an improvement. |
|
go for it. |
Fixup for #46764. See the post-merge conversation for context. Reposting the comment that motivates this pr:
There are two ways that pow5 can be wrong,
m2is divisible bybig(5)^requiredFivesm2is not divisible bybig(5)^requiredFivesIn the first case, for pow5 to be wrong,
big(5)^requiredFivesmust be greater thantypemax(Int), butm2is always less than1<<53, and a small number is not divisible by a large number. Thusm2will never be divisible by the truebig(5)^requiredFiveswhen there is overflow and case 1 is okay.For the second case, we need to find a
requiredFivessuch thatpow5may wrongly return true. That is, find arequiredFivessuch that there existsm2wherem2 % 5^requiredFives == 0For this to be the case, eitherm2must be 0 (handled much earlier as a special case) orabs(5^requiredFives) <= m2 < 1<<53. Searching exhaustively from the cases that have overflow,findfirst(requiredFives -> abs(5^requiredFives) < 1<<53, 28:10^5)+27 == 55. Note that5^55 < 0. If, on the other hand, we usedunsinged(5)^requiredFives, then we'd havefindfirst(requiredFives -> unsigned(5)^requiredFives < 1<<53, 28:10^5)+27 == 1048and IIUCrequiredFivesis at mostlog10(floatmax(Float64)) == 308.25471555991675.So using
unsigned(5)should fix this and I benchmark it as comparable to signed exponentiation.