#7858 closed bug (fixed)
Fix definitions of abs/signum for Floats/Doubles
Reported by: | lerkok | Owned by: | bernalex |
---|---|---|---|
Priority: | normal | Milestone: | 7.10.1 |
Component: | libraries/base | Version: | 7.6.3 |
Keywords: | floating point | Cc: | anton.nik@…, ekmett@…, hvr, ekmett |
Operating System: | Unknown/Multiple | Architecture: | Unknown/Multiple |
Type of failure: | None/Unknown | Test Case: | |
Blocked By: | Blocking: | ||
Related Tickets: | #9238, #13212 | Differential Rev(s): | Phab:D145 |
Wiki Page: |
Description
The current definition of abs doesn't work correctly for -0.0 (negative zero); and the signum function doesn't return NaN on NaN. The issue is discussed in detail in the following thread:
http://www.haskell.org/pipermail/libraries/2013-April/019677.html
To summarize, the proposal is to change both the Float and Double instance definitions for signum/abs as follows:
signum x | x == 0 = x | isNaN x = x | x > 0 = 1 | otherwise = negate 1 abs x | isNegativeZero x = 0 | x >= 0 = x | otherwise = negate x
Simon PJ expressed interest in taking advantage of the underlying platforms floating-point instructions when available to speed things up, and the above referenced thread has some discussion regarding how this might be done. However, it might be best to start with a "correct" implementation first, and then later worry about speed.
On a related note, this "bug" is actually present in the Haskell'98 standard definition itself, which seems to have overlooked NaN's and negative-0's. It'd be great if the standard got a similar amendment as well.
Attachments (2)
Change History (18)
comment:1 Changed 4 years ago by
Cc: | anton.nik@… added |
---|
comment:2 Changed 4 years ago by
Cc: | ekmett@… hvr added |
---|---|
difficulty: | → Unknown |
Changed 4 years ago by
Attachment: | 0001-Make-Prelude.abs-handle-0.0-correctly-7858.patch added |
---|
Changed 4 years ago by
Attachment: | 0002-Make-Prelude.signum-handle-0.0-correctly-7858.patch added |
---|
comment:3 Changed 4 years ago by
Cc: | ekmett added |
---|---|
Status: | new → patch |
I added my patches for this here per SPJ's suggestion.
The signum patch does not work. I do not know why. It must be due to GHC internals, because the function itself is fine as may be verified by copying
signumFix x | x == 0.0 = x | isNaN x = x | x > 0.0 = 1 | otherwise = negate 1
into a file signumfix.hs and :l signumfix.hs into GHCi. signumFix (-0.0 :: Float) will now return -0.0, where Prelude.signum returns 0.0 with the same argument.
Here is a core dump of me invoking my Prelude.signum after building GHC/GHCi with my patches:
Prelude> abs (-0.0 :: Float) ==================== Simplified expression ==================== let { it_aqt :: GHC.Types.Float [LclId, Str=DmdType, Unf=Unf{Src=<vanilla>, TopLvl=False, Arity=0, Value=False, ConLike=False, WorkFree=False, Expandable=False, Guidance=IF_ARGS [] 70 0}] it_aqt = GHC.Num.abs @ GHC.Types.Float GHC.Float.$fNumFloat (GHC.Num.negate @ GHC.Types.Float GHC.Float.$fNumFloat (GHC.Types.F# (__float 0.0))) } in GHC.Base.thenIO @ () @ [()] (System.IO.print @ GHC.Types.Float GHC.Float.$fShowFloat it_aqt) (GHC.Base.returnIO @ [()] (GHC.Types.: @ () (it_aqt `cast` (UnivCo representational GHC.Types.Float () :: GHC.Types.Float ~# ())) (GHC.Types.[] @ ()))) 0.0 Prelude> signum (-0.0 :: Float) ==================== Simplified expression ==================== let { it_aKQ :: GHC.Types.Float [LclId, Str=DmdType, Unf=Unf{Src=<vanilla>, TopLvl=False, Arity=0, Value=False, ConLike=False, WorkFree=False, Expandable=False, Guidance=IF_ARGS [] 70 0}] it_aKQ = GHC.Num.signum @ GHC.Types.Float GHC.Float.$fNumFloat (GHC.Num.negate @ GHC.Types.Float GHC.Float.$fNumFloat (GHC.Types.F# (__float 0.0))) } in GHC.Base.thenIO @ () @ [()] (System.IO.print @ GHC.Types.Float GHC.Float.$fShowFloat it_aKQ) (GHC.Base.returnIO @ [()] (GHC.Types.: @ () (it_aKQ `cast` (UnivCo representational GHC.Types.Float () :: GHC.Types.Float ~# ())) (GHC.Types.[] @ ()))) 0.0
Please note that the abs patch works well and is likely ready to go.
comment:4 Changed 3 years ago by
I'm taking a look at these patches now to see if I can figure out the issue with the first one.
comment:5 Changed 3 years ago by
Related Tickets: | → #9238 |
---|
Oh I know why. See #9238. signum
looks something like
\(D# x) -> case x of { __DEFAULT__ -> ... 0.0 -> x }
and at -O1
and higher, x
is inlined to 0
in the right-hand side of the second case alternative.
comment:6 Changed 3 years ago by
Milestone: | → 7.10.1 |
---|
We can replace the expensive isNegativeZero x
test in abs
with x == 0
, since abs
of positive zero is also zero.
abs x | x == 0 = 0 | x >= 0 = x | otherwise = negate x
Of course this merits a comment on the first guard!
As for signum
, I don't see off-hand a simple way to work around the bug #9238, so I suggest we apply the abs
patch, then mark this ticket as blocked by #9238. If someone gets around to implementing efficient abs
and signum
with fancy bit tricks before 7.10 then it will become moot anyway.
comment:7 Changed 3 years ago by
Actually, how about this for signum?
signum :: Double -> Double signum x | x > 0 = 1 | x < 0 = negate 1 | otherwise = x
comment:8 follow-up: 9 Changed 3 years ago by
if we had a cheap Float<->Word32, Double <-> Word64, could we define these wrt the bit banging approach? In fact, can't we define those already using CMM (wrt the unlifted types)?
comment:9 Changed 3 years ago by
Replying to carter:
if we had a cheap Float<->Word32, Double <-> Word64, could we define these wrt the bit banging approach? In fact, can't we define those already using CMM (wrt the unlifted types)?
Well, on x86, ideally we would use andps
/andpd
for abs
, and stay within the SSE register set. I don't know what is the most efficient way to implement signum
.
comment:10 Changed 3 years ago by
Resubmitted abs without isNegativeZero to <https://phabricator.haskell.org/D145>.
comment:11 Changed 3 years ago by
Differential Rev(s): | → Phab:D145 |
---|
comment:12 Changed 3 years ago by
Owner: | set to bernalex |
---|
I'm going to write a patch using rwbarton's suggestion for signum, and test it with ghci and with various ghc optimisations tonight or tomorrow.
comment:16 Changed 10 months ago by
Related Tickets: | #9238 → #9238, #13212 |
---|
Ticket for adding a faster abs
implementation is #13212.
This makes sense to me.
I'm personally, in favor of fixing it to be correct first, then opening an issue that it could be faster, as what we are doing now is incorrect.
It doesn't matter how fast you do your job if you do the wrong job. ;)