Opened 5 years ago

Closed 3 years ago

Last modified 10 months ago

#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)

0001-Make-Prelude.abs-handle-0.0-correctly-7858.patch (1.9 KB) - added by bernalex 4 years ago.
0002-Make-Prelude.signum-handle-0.0-correctly-7858.patch (1.9 KB) - added by bernalex 4 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 4 years ago by lelf

Cc: anton.nik@… added

comment:2 Changed 4 years ago by ekmett

Cc: ekmett@… hvr added
difficulty: Unknown

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. ;)

comment:3 Changed 4 years ago by bernalex

Cc: ekmett added
Status: newpatch

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 thoughtpolice

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 rwbarton

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 rwbarton

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 rwbarton

Actually, how about this for signum?

signum :: Double -> Double
signum x 
  | x > 0     = 1
  | x < 0     = negate 1
  | otherwise = x

comment:8 Changed 3 years ago by 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)?

comment:9 in reply to:  8 Changed 3 years ago by rwbarton

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 bernalex

Resubmitted abs without isNegativeZero to <https://phabricator.haskell.org/D145>.

comment:11 Changed 3 years ago by thoughtpolice

Differential Rev(s): Phab:D145

comment:12 Changed 3 years ago by bernalex

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:13 Changed 3 years ago by Austin Seipp <austin@…>

In 6f6ee6eaa348b1a4815190c4d526d5c81c264fa7/ghc:

Make Prelude.abs handle -0.0 correctly (#7858)

Summary:
Make the `Float` and `Double` implementations of `abs` handle -0.0
correctly per IEEE-754.

abs (-0.0::Float) and abs (-0.0::Double) previously returned -0.0, when
they should return 0.0. This patch fixes this.

Signed-off-by: Alexander Berntsen <alexander@plaimi.net>

Test Plan: abs (-0.0::Double) should = 0.0 instead of (-0.0)

Reviewers: ekmett, hvr, austin, rwbarton

Reviewed By: austin, rwbarton

Subscribers: phaskell, trofi, simonmar, relrod, ezyang, carter

Differential Revision: https://phabricator.haskell.org/D145

GHC Trac Issues: #7858

comment:14 Changed 3 years ago by Austin Seipp <austin@…>

In d9a20573f473cc7389004470999b8a318aa6b3f2/ghc:

Make Prelude.signum handle -0.0 correctly (#7858)

Summary:
Make the `Float` and `Double` implementations of `signum` handle -0.0
correctly per IEEE-754.

This, together with "Make Prelude.abs handle -0.0 correctly (#7858)",
fixes Trac #7858.

Depends on D145

Signed-off-by: Alexander Berntsen <alexander@plaimi.net>

Test Plan:
signum of (-0.0) should be (-0.0) not 0.0.

Test program:

  main =
    putStrLn $ p ++ " " ++ n
    where
      f = show . signum
      p = f (-0.0 :: Double)
    n = f (0.0 :: Double)

Reviewers: ekmett, hvr, rwbarton, austin

Reviewed By: austin

Subscribers: phaskell, simonmar, relrod, ezyang, carter

Differential Revision: https://phabricator.haskell.org/D148

GHC Trac Issues: #7858

comment:15 Changed 3 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

Merged.

comment:16 Changed 10 months ago by rwbarton

Ticket for adding a faster abs implementation is #13212.

Note: See TracTickets for help on using tickets.