[GHC] #15696: Derived Ord instance for enumerations with more than 8 elements seems to be incorrect

[GHC]

#15696: Derived Ord instance for enumerations with more than 8 elements seems to be
incorrect
-------------------------------------+-------------------------------------
        Reporter:  mrkkrp            |                Owner:  osa1
            Type:  bug               |               Status:  patch
        Priority:  highest           |            Milestone:  8.6.2
       Component:  Compiler          |              Version:  8.6.1
      Resolution:                    |             Keywords:
Operating System:  Unknown/Multiple  |         Architecture:
 Type of failure:  Incorrect result  |  Unknown/Multiple
  at runtime                         |            Test Case:
      Blocked By:                    |             Blocking:
 Related Tickets:  #14677, #15155    |  Differential Rev(s):  Phab:D5196,
       Wiki Page:                    |  Phab:D5201
-------------------------------------+-------------------------------------

Comment (by osa1):

 I did (4) and tried to validate. There are 4 regressions. 2 of them are
 perf,
 the other 2 are tests with checks on -ddump-simpl output. They're all
 basically
 the same problem: worker/wrapper results change slightly after the patch,
 and
 worker functions take more boxed params. Here's an example (T10482).
 Original
 program is this:

 {{{
 {-# LANGUAGE BangPatterns #-}
 {-# LANGUAGE TypeFamilies #-}
 module T10482 where

 data family Foo a
 data instance Foo (a, b) = FooPair !(Foo a) !(Foo b)
 newtype instance Foo Int = Foo Int

 foo :: Foo ((Int, Int), Int) -> Int -> Int
 foo !f k =
   if k == 0 then 0
   else if even k then foo f (k-1)
   else case f of
     FooPair (FooPair (Foo n) _) _ -> n
 }}}

 `$wfoo` originally:

 {{{
 Rec {
 -- RHS size: {terms: 19, types: 4, coercions: 0, joins: 0/0}
 T10482.$wfoo [InlPrag=NOUSERINLINE[2], Occ=LoopBreaker]
   :: GHC.Prim.Int# -> GHC.Prim.Int# -> GHC.Prim.Int#
 [GblId,
  Arity=2,
  Caf=NoCafRefs,
  Str=<L,1*U><S,1*U>,
  Unf=OtherCon []]
 T10482.$wfoo
   = \ (ww_s2Qy :: GHC.Prim.Int#) (ww1_s2QG :: GHC.Prim.Int#) ->
       case ww1_s2QG of wild_X1v {
         __DEFAULT ->
           case GHC.Prim.remInt# wild_X1v 2# of {
             __DEFAULT -> ww_s2Qy;
             0# -> T10482.$wfoo ww_s2Qy (GHC.Prim.-# wild_X1v 1#)
           };
         0# -> 0#
       }
 end Rec }
 }}}

 after the patch (`wip/T15696`):

 {{{
 Rec {
 -- RHS size: {terms: 22, types: 7, coercions: 3, joins: 0/0}
 T10482.$wfoo [InlPrag=NOUSERINLINE[2], Occ=LoopBreaker]
   :: Foo Int -> GHC.Prim.Int# -> GHC.Prim.Int#
 [GblId,
  Arity=2,
  Caf=NoCafRefs,
  Str=<L,1*U(1*U)><S,1*U>,
  Unf=OtherCon []]
 T10482.$wfoo
   = \ (ww_s2Qs
          :: Foo Int
          Unf=OtherCon [])
       (ww1_s2Qz :: GHC.Prim.Int#) ->
       case ww1_s2Qz of wild_X1l {
         __DEFAULT ->
           case GHC.Prim.remInt# wild_X1l 2# of {
             __DEFAULT ->
               case ww_s2Qs
                    `cast` (T10482.D:R:FooInt0[0] ; T10482.N:R:FooInt[0]
                            :: Foo Int ~R# Int)
               of
               { GHC.Types.I# ww3_s2QD ->
               ww3_s2QD
               };
             0# -> T10482.$wfoo ww_s2Qs (GHC.Prim.-# wild_X1l 1#)
           };
         0# -> 0#
       }
 end Rec }
 }}}

 The difference is that the first argument is now boxed `Int` instead of
 `Int#`
 as before.

 I think the reason for this change is this: originally evaluation of
 `ww_s2Qs`
 happens after `remInt# wild_X1l 2#`, but if we pass `ww_s2Qs` unboxed
 that'd
 mean evaluating it before `remInt#`. This is possible if at least one of
 those
 expressions are OK-for-speculation. `remInt#` is not OK-for-speculation
 because
 it can fail. `ww_s2Qs` was previously (without `wip/T15696`) OK-for-
 speculation,
 but we just made `app_ok` more strict by removing a disjunct which was
 actually
 holding for this expression:

 - `n_val_args == 0` holds because it has no args
 - `isEvaldUnfolding (idUnfolding fun)` apparently also somehow holds. I
 don't
   know how exactly, but I'm guessing that because of the strictness
 annotation
   on the data con field we give this id `evaldUnfolding`.

 Other 3 regressions are also of similar nature (worker function with less
 unboxing).

 What to do? I think it's correct to consider the data con field as strict
 here
 and pass an unboxed value to the worker. While we may not actually have a
 tagged
 value there (or even an evaluated one! e.g. #14677, #15155), the
 programmer
 indicated that it's fine to evaluate the field eagerly. As long as we
 don't
 expect (in the generated code) evaluated or tagged value it's fine.

 So perhaps we should not do the change in `ok_app`.

-- 
Ticket URL: <http://ghc.haskell.org/trac/ghc/ticket/15696#comment:76>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler