[GHC] #9238: Negative zero broken
#9238: Negative zero broken --------------------------+------------------------------------------------ Reporter: | Owner: augustss | Status: new Type: bug | Milestone: Priority: normal | Version: 7.8.2 Component: | Operating System: Windows Compiler | Type of failure: Incorrect result at runtime Keywords: | Test Case: Architecture: | Blocking: Unknown/Multiple | Difficulty: | Unknown | Blocked By: | Related Tickets: | --------------------------+------------------------------------------------ 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 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 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 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. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken ------------------------------------------------+-------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Windows | Architecture: Type of failure: Incorrect result at runtime | Unknown/Multiple Test Case: | Difficulty: Blocking: | Unknown | Blocked By: | Related Tickets: ------------------------------------------------+-------------------------- Comment (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. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:1> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken ------------------------------------------------+-------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Windows | Architecture: Type of failure: Incorrect result at runtime | Unknown/Multiple Test Case: | Difficulty: Blocking: | Unknown | Blocked By: | Related Tickets: ------------------------------------------------+-------------------------- Comment (by ezyang): Appears to be a regression from 7.6. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:2> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken ------------------------------------------------+-------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result at runtime | Unknown/Multiple Test Case: | Difficulty: Blocking: | Unknown | Blocked By: | Related Tickets: ------------------------------------------------+-------------------------- Changes (by ezyang): * os: Windows => Unknown/Multiple -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:3> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken ------------------------------------------------+-------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result at runtime | Unknown/Multiple Test Case: | Difficulty: Blocking: | Unknown | Blocked By: | Related Tickets: ------------------------------------------------+-------------------------- Comment (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 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:4> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Unknown Type of failure: Incorrect | Blocked By: result at runtime | Related Tickets: Test Case: | Blocking: | Differential Revisions: | -------------------------------------+------------------------------------- Comment (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. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:5> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Unknown Type of failure: Incorrect | Blocked By: result at runtime | Related Tickets: Test Case: | Blocking: | Differential Revisions: | -------------------------------------+------------------------------------- Comment (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 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:6> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Unknown Type of failure: Incorrect | Blocked By: result at runtime | Related Tickets: #7858 Test Case: | Blocking: | Differential Revisions: | -------------------------------------+------------------------------------- Changes (by rwbarton): * related: => #7858 Comment: This arose also in #7858. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:7> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.8.4 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Unknown Type of failure: Incorrect | Blocked By: result at runtime | Related Tickets: #7858, #9451 Test Case: | Blocking: | Differential Revisions: | -------------------------------------+------------------------------------- Changes (by rwbarton): * related: #7858 => #7858, #9451 * milestone: => 7.8.4 Comment: And #9451. Ideally we would fix this for 7.8.4 since it is a regression from 7.6.3. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:8> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.1 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: | Architecture: Unknown/Multiple Unknown/Multiple | Difficulty: Unknown Type of failure: Incorrect | Blocked By: result at runtime | Related Tickets: #7858, #9451 Test Case: | Blocking: | Differential Revisions: | -------------------------------------+------------------------------------- Description changed by thomie: Old description:
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 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 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 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.
New description: 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. -- -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:10> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451 | Differential Revisions: -------------------------------------+------------------------------------- Comment (by lerkok): Similar issue, and perhaps exactly the same bug: https://ghc.haskell.org/trac/ghc/ticket/10215#ticket -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:12> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451 | Differential Revisions: -------------------------------------+------------------------------------- Comment (by bgamari): Replying to [comment:6 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.
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? -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:13> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451 | Differential Revisions: -------------------------------------+------------------------------------- Comment (by simonpj): Replying to [comment:13 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 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:14> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.12.1 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451 | Differential Revisions: Phab:D1061 -------------------------------------+------------------------------------- Changes (by bgamari): * differential: => Phab:D1061 -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:15> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.3 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451, | Differential Revisions: Phab:D1061 #10215 | -------------------------------------+------------------------------------- Changes (by YitzGale): * cc: gale@… (added) * related: #7858, #9451 => #7858, #9451, #10215 * milestone: 8.0.1 => 7.10.3 Comment: 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. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:17> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: new Priority: normal | Milestone: 7.10.3 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451, | Differential Rev(s): Phab:D1061 #10215 | -------------------------------------+------------------------------------- Comment (by Ben Gamari <ben@…>): In [changeset:"eb975d2eec349429e735c272d46a7becccf393c6/ghc" eb975d2/ghc]: {{{ #!CommitTicketReference repository="ghc" revision="eb975d2eec349429e735c272d46a7becccf393c6" 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 }}} -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:18> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: merge Priority: normal | Milestone: 7.10.3 Component: Compiler | Version: 7.8.2 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451, | Differential Rev(s): Phab:D1061 #10215 | -------------------------------------+------------------------------------- Changes (by bgamari): * status: new => merge Comment: Fixed. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:19> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
#9238: Negative zero broken -------------------------------------+------------------------------------- Reporter: augustss | Owner: Type: bug | Status: closed Priority: normal | Milestone: 7.10.3 Component: Compiler | Version: 7.8.2 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: Type of failure: Incorrect result | Unknown/Multiple at runtime | Test Case: Blocked By: | Blocking: Related Tickets: #7858, #9451, | Differential Rev(s): Phab:D1061 #10215 | -------------------------------------+------------------------------------- Changes (by bgamari): * status: merge => closed * resolution: => fixed Comment: Merged to `ghc-7.10` as 293bf83f209ed6202a131456bf93fd472a790b13. -- Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/9238#comment:20> GHC <http://www.haskell.org/ghc/> The Glasgow Haskell Compiler
participants (1)
-
GHC