math/big: Float.Float32() does not correctly handle denormals that are rounded up to a power of 2 #14553
Comments
bradfitz
changed the title
|
I think have a working patch for it now. Not the most elegant though, so I would appreciate Robert's review. |
|
CL https://golang.org/cl/20012 mentions this issue. |
|
Robert, would you mind taking a look at the patch? I'm happy to rework it if you think there is a better way. On Monday I'm going out of town for a couple weeks, so it would be great to get in before then. |
|
Sure, will do, tomorrow.
On Wed, Mar 2, 2016 at 8:32 PM, tarm notifications@github.com wrote:
|
|
@tarm Apologies for the delay and thank you for your investigation and for providing test cases. That has been a good starting point for my own investigation. As an aside, independent of the git commit history, big.Float should handle denormals correctly, so this is clearly an issue that needs to be addressed. Your analysis is not quite accurate: The whole point of rounding is to round to a given precision - there is no such thing as "rounding to a bit representation with more precision" in this context. Specifically, rounding is correct in this code. What does happen though in these special denormal cases is that the exponent changes. This is what you call "bit representation with more precision", except that the precision doesn't change, but the exponent gets increased by one due to rounding. The bug is that this exponent change is dropped on the floor in these case: if rounding "rounded up", the exponent increased but it is not considered anymore later. The correct fix is to recompute the precision available for the final denormal by looking at the new exponent again, rather than using r.prec when computing the mantissa. Consequently, the actual fix is much simpler than suggested. I'll send out a CL in a little bit. See also my feedback on your CL (forthcoming). |
|
CL https://golang.org/cl/20198 mentions this issue. |
|
CL https://golang.org/cl/20199 mentions this issue. |
|
Yes, my explanation was imprecise. I agree with your description and you patch looks much cleaner. Another way of thinking about it is that the float32 denormals have a constant exponent, but the rounding for in big.Float only has fixed precision rounding out of the box. So the Float32() code has to convert from fixed precision to fixed exponent in the denormal case. |
The changes to internal/big are completely automatic by running vendor.bash in that directory. Also added respective test case. For #14553. Change-Id: I98b124bcc9ad9e9bd987943719be27864423cb5d Reviewed-on: https://go-review.googlesource.com/20199 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Context for this issue is https://groups.google.com/forum/#!topic/golang-dev/8xrGLS6mzzE
It appears to be a bug in the rounding of big.Float.Float32() and
Float64(), which the compiler uses to convert constants to float32 and
float64.
Specifically the bug happens when 1) the float32/64 constant is a
tiny, denormal value, 2) the arbitrary precision value would have to
round up to a bit representation with more precision. So for example
0x7.9p-149 should round up to 0x8.0p-149 when converted to a float32,
but the existing code actually gets stuck at 3 bits of precision while
still overflowing the mantissa so 0x4.0p-149 is actually what gets
assigned (which is worse than rounding down to 0x7.0p-149).
Here is a playground link that shows the problem:
http://play.golang.org/p/_aMEvOuOAs
To be fair, several places in the git commit history and in the
comments discuss how math.Float does not completely handle denormal
values. On the other hand, it seems like the compiler should be able
to parse and assign them properly.
go v1.6, linux, x64
The text was updated successfully, but these errors were encountered: