Opened 3 years ago

Closed 2 years ago

#9238 closed bug (fixed)

Negative zero broken

Reported by: augustss Owned by:
Priority: normal Milestone: 7.10.3
Component: Compiler Version: 7.8.2
Keywords: Cc: gale@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect result at runtime Test Case:
Blocked By: Blocking:
Related Tickets: #7858, #9451, #10215 Differential Rev(s): Phab:D1061
Wiki Page:

Description (last modified by thomie)

Try the following program

compareDouble :: Double -> Double -> Ordering
compareDouble x y =
       case (isNaN x, isNaN y) of
       (True, True)   -> EQ
       (True, False)  -> LT
       (False, True)  -> GT
       (False, False) ->
          -- Make -0 less than 0
          case (x == 0, y == 0, isNegativeZero x, isNegativeZero y) of
          (True, True, True, False) -> LT
          (True, True, False, True) -> GT
          _                         -> x `compare` y

main = do
    let l = [-0, 0]
    print [ (x, y, compareDouble x y) | x <- l, y <- l ]

Compile and run with -O0

$ ghc -O0 -fforce-recomp D.hs
[1 of 1] Compiling Main             ( D.hs, D.o )
Linking D.exe ...
$ ./D
[(-0.0,-0.0,EQ),(-0.0,0.0,LT),(0.0,-0.0,GT),(0.0,0.0,EQ)]

This is the correct output.

Compile and run with -O1

$ ghc -O1 -fforce-recomp D.hs
[1 of 1] Compiling Main             ( D.hs, D.o )
Linking D.exe ...
$ ./D
[(-0.0,-0.0,LT),(-0.0,0.0,LT),(0.0,-0.0,EQ),(0.0,0.0,EQ)]

This is wrong.

Put a NOINLINE pragma on compareDouble:

$ ghc -O1 -fforce-recomp D.hs
[1 of 1] Compiling Main             ( D.hs, D.o )
Linking D.exe ...
$ ./D
[(-0.0,-0.0,EQ),(-0.0,0.0,EQ),(0.0,-0.0,EQ),(0.0,0.0,EQ)]

This is wrong in a different way.

Change History (20)

comment:1 Changed 3 years ago by augustss

My guess is that this very function (which gives a total order to most doubles) is needed instead of ordinary comparison somewhere in the ghc optimizer.

comment:2 Changed 3 years ago by ezyang

Appears to be a regression from 7.6.

comment:3 Changed 3 years ago by ezyang

Operating System: WindowsUnknown/Multiple

comment:4 Changed 3 years ago by carter

I can reproduce the behavior on ghc 7.8.2 on OS X 64 bit

i've also started taking a look at the core generated for the 3 alternatives lennart laid out, using variations of the following ghc invocation

ghc -O1 -ddump-simpl -dsuppress-idinfo -dsuppress-coercions -dsuppress-type-applications -dsuppress-uniques -dsuppress-module-prefixes bugMain.hs  > bugO1CoreNOINLINE.simple

though i'm still working through which differences are real and which arent

https://gist.github.com/cartazio/351f15ae74257a7ab64d

comment:5 Changed 3 years ago by rwbarton

When we do

    case ds1 of wild_XT {   -- ds1 :: Double#   (y = D# ds1)
      __DEFAULT -> ...
      0.0 -> ...
    }

inside the 0.0 case we seem to create a local unfolding ds1 = 0.0. You can see this happening with -dverbose-core2core -ddump-inlinings: search for "Inlining done: x" or "ds1".

This is wrong because the 0.0 case matches both positive and negative zero. That is the correct semantics that makes the Core match the original program, and it is implemented correctly by the code generation. But it means that we can't create this local unfolding when the pattern is specifically 0.0.

I don't know where this unfolding gets created, or I'd try to fix it myself.

comment:6 Changed 3 years ago by simonpj

I can tell you exactly where it's created: line 2107 of Simplify.lhs (in today's HEAD), in function simplAlt.

However I'm suspicious of adding a special case for float/double here. Rather, I think we should prohibit using Core-language case expressions to scrutinise float/double, so that case (in Core) behaves in a simple, predictable way.

Rather I think we should probably generate

   case eqDouble# ds1 0.0## of 
     True -> ...
     False -> ...

(or, rather, today's unboxed-boolean version). Now the magic is confined to how eqDouble# is implemented, which is the proper place for it.

Now Haskell source code does allow case expressions over floats, but that's just a question of fixing the desugarer.

Does that make sense? Does anyone feel like taking this on?

Simon

comment:7 Changed 3 years ago by rwbarton

This arose also in #7858.

comment:8 Changed 3 years ago by rwbarton

Milestone: 7.8.4

And #9451.

Ideally we would fix this for 7.8.4 since it is a regression from 7.6.3.

comment:9 Changed 3 years ago by thoughtpolice

Milestone: 7.8.47.10.1

Moving (in bulk) to 7.10.4

comment:10 Changed 3 years ago by thomie

Description: modified (diff)

comment:11 Changed 3 years ago by thoughtpolice

Milestone: 7.10.17.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

comment:12 Changed 3 years ago by lerkok

Similar issue, and perhaps exactly the same bug: https://ghc.haskell.org/trac/ghc/ticket/10215#ticket

comment:13 in reply to:  6 ; Changed 2 years ago by bgamari

Replying to simonpj:

However I'm suspicious of adding a special case for float/double here. Rather, I think we should prohibit using Core-language case expressions to scrutinise float/double, so that case (in Core) behaves in a simple, predictable way.

Rather I think we should probably generate

   case eqDouble# ds1 0.0## of 
     True -> ...
     False -> ...

(or, rather, today's unboxed-boolean version). Now the magic is confined to how eqDouble# is implemented, which is the proper place for it.

simonpj, how do you think your suggestion should interact with the litEq rule? As far as I can tell this PrelRule is responsible for most of the floating-point pattern matches that we are seeing. As you essentially suggesting we disable litEq for floating-point types?

Last edited 2 years ago by bgamari (previous) (diff)

comment:14 in reply to:  13 Changed 2 years ago by simonpj

Replying to bgamari:

simonpj, how do you think your suggestion should interact with the litEq rule? As far as I can tell this PrelRule is responsible for most of the floating-point pattern matches that we are seeing. As you essentially suggesting we disable litEq for floating-point types?

Well, IF we agree that Core case expressions should not scrutinise floating point values, THEN

  • We should document that invariant, with explanation of why
  • We should make Core Lint check for it
  • And yes litEq (which generates such case expressions) should not do so for floating point values

Simon

comment:15 Changed 2 years ago by bgamari

Differential Rev(s): Phab:D1061

comment:16 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:17 Changed 2 years ago by YitzGale

Cc: gale@… added
Milestone: 8.0.17.10.3

As per Austin's request on ghc-dev, changing milestone to 7.10.3 due to: https://github.com/LeventErkok/sbv/issues/138

It appears from the submission that Lennart also needs this. And anyway this looks like a regression that really should be fixed.

However, I do note that this issue doesn't have a patch yet. So if this is wrong, I apologize.

EDIT: Oh, sorry, it does have a patch on Phab.

Last edited 2 years ago by YitzGale (previous) (diff)

comment:18 Changed 2 years ago by Ben Gamari <ben@…>

In eb975d2/ghc:

Fix treatment of -0.0

Here we fix a few mis-optimizations that could occur in code with
floating point comparisons with -0.0. These issues arose from our
insistence on rewriting equalities into case analyses and the
simplifier's ignorance of floating-point semantics.

For instance, in Trac #10215 (and the similar issue Trac #9238) we
turned `ds == 0.0` into a case analysis,

```
case ds of
    __DEFAULT -> ...
    0.0 -> ...
```

Where the second alternative matches where `ds` is +0.0 and *also* -0.0.
However, the simplifier doesn't realize this and will introduce a local
inlining of `ds = -- +0.0` as it believes this is the only
value that matches this pattern.

Instead of teaching the simplifier about floating-point semantics
we simply prohibit case analysis on floating-point scrutinees and keep
this logic in the comparison primops, where it belongs.

We do several things here,

 - Add test cases from relevant tickets
 - Clean up a bit of documentation
 - Desugar literal matches against floats into applications of the
   appropriate equality primitive instead of case analysis
 - Add a CoreLint to ensure we don't pattern match on floats in Core

Test Plan: validate with included testcases

Reviewers: goldfire, simonpj, austin

Subscribers: thomie

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

GHC Trac Issues: #10215, #9238

comment:19 Changed 2 years ago by bgamari

Status: newmerge

Fixed.

comment:20 Changed 2 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Note: See TracTickets for help on using tickets.