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

Fix overflow in pow5 #47511

Merged
merged 5 commits into from
Nov 26, 2022
Merged

Fix overflow in pow5 #47511

merged 5 commits into from
Nov 26, 2022

Conversation

LilithHafner
Copy link
Member

Fixup for #46764. See the post-merge conversation for context. Reposting the comment that motivates this pr:

julia> using Printf

julia> @printf "%.8g" 4.645833859177319e63 # master
4.6458338e+63

julia> @printf "%.8g" 4.645833859177319e63 # 1.8
4.6458339e+63

There are two ways that pow5 can be wrong,

  1. returning false when m2 is divisible by big(5)^requiredFives
  2. returning true when m2 is not divisible by big(5)^requiredFives
    In the first case, for pow5 to be wrong, big(5)^requiredFives must be greater than typemax(Int), but m2 is always less than 1<<53, and a small number is not divisible by a large number. Thus m2 will never be divisible by the true big(5)^requiredFives when there is overflow and case 1 is okay.

For the second case, we need to find a requiredFives such that pow5 may wrongly return true. That is, find a requiredFives such that there exists m2 where m2 % 5^requiredFives == 0 For this to be the case, either m2 must be 0 (handled much earlier as a special case) or abs(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 that 5^55 < 0. If, on the other hand, we used unsinged(5)^requiredFives, then we'd have findfirst(requiredFives -> unsigned(5)^requiredFives < 1<<53, 28:10^5)+27 == 1048 and IIUC requiredFives is at most log10(floatmax(Float64)) == 308.25471555991675.

So using unsigned(5) should fix this and I benchmark it as comparable to signed exponentiation.

@martinholters
Copy link
Member

Too lazy to try and follow that reasoning, but does that also work if unsinged(5) isa UInt32?

@LilithHafner
Copy link
Member Author

Good catch! No, the story is much worse then because the x can still be up to almost 1<<53 and 5^p is bounded by typemax(UInt32) so false positives are much easier to construct. Easy fix though.

base/ryu/utils.jl Outdated Show resolved Hide resolved
test/ryu.jl Show resolved Hide resolved
@StefanKarpinski
Copy link
Sponsor Member

Brava, @LilithHafner 👏🏻

@ViralBShah ViralBShah added the maths Mathematical functions label Nov 10, 2022
@LilithHafner LilithHafner added the display and printing Aesthetics and correctness of printed representations of objects. label Nov 10, 2022
@LilithHafner
Copy link
Member Author

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.

@LilithHafner LilithHafner added backport 1.9 Change should be backported to release-1.9 bugfix This change fixes an existing bug labels Nov 25, 2022
@oscardssmith
Copy link
Member

go for it.

@LilithHafner LilithHafner merged commit 02aa0b0 into master Nov 26, 2022
@LilithHafner LilithHafner deleted the LilithHafner-patch-4 branch November 26, 2022 00:47
KristofferC pushed a commit that referenced this pull request Nov 28, 2022
Fixup for #46764

(cherry picked from commit 02aa0b0)
@KristofferC KristofferC mentioned this pull request Dec 14, 2022
26 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug display and printing Aesthetics and correctness of printed representations of objects. maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants