Opened 7 years ago

Closed 5 years ago

#6056 closed bug (fixed)

INLINABLE pragma prevents worker-wrapper to happen.

Reported by: milan Owned by: simonpj
Priority: normal Milestone: 7.10.1
Component: Compiler Version: 7.4.1
Keywords: Cc: fox@…, tkn.akio@…, hackage.haskell.org@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case: simplCore/should_compile/T6056
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

When working on containers I found out that a method returning a pair marked as INLINABLE does not go through a worker/wrapper transformation to get a worker that would return unboxed pair.

For example:

module Test where

smallerAndRest :: Ord a => a -> [a] -> (Maybe a, [a])
smallerAndRest x [] = (Nothing, [])
smallerAndRest x (y:ys) | y < x = (Just y, ys)
                        | otherwise = smallerAndRest x ys

{-# INLINABLE smallerAndRest #-}

With the INLINABLE pragma, -ddump-prep prints

==================== CorePrep ====================
Result size = 42

lvl_rkg :: forall a_ajz. (Data.Maybe.Maybe a_ajz, [a_ajz])
[GblId, Caf=NoCafRefs, Str=DmdType m, Unf=OtherCon []]
lvl_rkg =
  \ (@ a_ajz) -> (Data.Maybe.Nothing @ a_ajz, GHC.Types.[] @ a_ajz)

Rec {
Test.smallerAndRest [InlPrag=INLINABLE[ALWAYS], Occ=LoopBreaker]
  :: forall a_a9I.
     GHC.Classes.Ord a_a9I =>
     a_a9I -> [a_a9I] -> (Data.Maybe.Maybe a_a9I, [a_a9I])
[GblId, Arity=3, Caf=NoCafRefs, Str=DmdType LLSm, Unf=OtherCon []]
Test.smallerAndRest =
  \ (@ a_ajz)
    ($dOrd_sko :: GHC.Classes.Ord a_ajz)
    (x_skq :: a_ajz)
    (ds_skk :: [a_ajz]) ->
    case ds_skk of _ {
      [] -> lvl_rkg @ a_ajz;
      : y_skp ys_sks ->
        case GHC.Classes.< @ a_ajz $dOrd_sko y_skp x_skq of _ {
          GHC.Types.False ->
            Test.smallerAndRest @ a_ajz $dOrd_sko x_skq ys_sks;
          GHC.Types.True ->
            let {
              sat_skw :: Data.Maybe.Maybe a_ajz
              [LclId]
              sat_skw = Data.Maybe.Just @ a_ajz y_skp } in
            (sat_skw, ys_sks)
        }
    }
end Rec }

but without the INLINABLE pragma, we get

==================== CorePrep ====================
Result size = 57

Rec {
Test.$wsmallerAndRest [Occ=LoopBreaker]
  :: forall a_a9I.
     GHC.Classes.Ord a_a9I =>
     a_a9I -> [a_a9I] -> (# Data.Maybe.Maybe a_a9I, [a_a9I] #)
[GblId, Arity=3, Caf=NoCafRefs, Str=DmdType LLS, Unf=OtherCon []]
Test.$wsmallerAndRest =
  \ (@ a_a9I)
    (w_skC :: GHC.Classes.Ord a_a9I)
    (w1_skE :: a_a9I)
    (w2_sky :: [a_a9I]) ->
    case w2_sky of _ {
      [] -> (# Data.Maybe.Nothing @ a_a9I, GHC.Types.[] @ a_a9I #);
      : y_skD ys_skG ->
        case GHC.Classes.< @ a_a9I w_skC y_skD w1_skE of _ {
          GHC.Types.False ->
            Test.$wsmallerAndRest @ a_a9I w_skC w1_skE ys_skG;
          GHC.Types.True ->
            let {
              sat_skV :: Data.Maybe.Maybe a_a9I
              [LclId]
              sat_skV = Data.Maybe.Just @ a_a9I y_skD } in
            (# sat_skV, ys_skG #)
        }
    }
end Rec }

Test.smallerAndRest [InlPrag=INLINE[0]]
  :: forall a_a9I.
     GHC.Classes.Ord a_a9I =>
     a_a9I -> [a_a9I] -> (Data.Maybe.Maybe a_a9I, [a_a9I])
[GblId, Arity=3, Caf=NoCafRefs, Str=DmdType LLSm, Unf=OtherCon []]
Test.smallerAndRest =
  \ (@ a_a9I)
    (w_skL :: GHC.Classes.Ord a_a9I)
    (w1_skM :: a_a9I)
    (w2_skN :: [a_a9I]) ->
    case Test.$wsmallerAndRest @ a_a9I w_skL w1_skM w2_skN
    of _ { (# ww1_skR, ww2_skS #) ->
    (ww1_skR, ww2_skS)
    }

Here the worker-wrapper creates a variant, which returns unboxed pair.

I assume that there is no simple solution to this, because the worker/wrapper changes the INLINABLE on smallerAndRest to INLINE. The INLINABLE could be added to $wsmallerAndRest, but then #5928 causes the specialization to fail.

Maybe at least both smallerAndRest and $wsmallerAndRest could be marked INLINABLE? Then it is not sure that smallerAndRest gets INLINED and $wsmallerAndRest exposed, but it is not worse than current situation.

Change History (10)

comment:1 Changed 7 years ago by igloo

difficulty: Unknown
Milestone: 7.8.1
Owner: set to simonpj

comment:2 Changed 6 years ago by akio

Cc: tkn.akio@… added

I was hit by this. Perhaps this behavior should at least be documented, because it is unexpected (at least to me) that adding an INLINABLE pragma generates a less efficient call.

comment:3 Changed 6 years ago by liyang

Cc: hackage.haskell.org@… added

comment:4 Changed 6 years ago by simonpj

I agree that it's surprising and undesirable. The reason it happens is this.

GHC keeps an "Unfolding inside each Id:

  • For INLINEABLE things it is the original user-written RHS
  • For w/w'd things it is the wrapper function

But there's only one Unfolding for each Id, so we have to choose. As you point out, that is a pity. Solution is probably to keep two unfoldings, in effect. Thanks for pointing this out so clearly.

Simon

comment:5 Changed 6 years ago by carter

I tested this just now on current HEAD (well, built 2 weeks ago)

here are the three versions

module Test where

smallerAndRest :: Ord a => a -> [a] -> (Maybe a, [a])
smallerAndRest x [] = (Nothing, [])
smallerAndRest x (y:ys) | y < x = (Just y, ys)
                        | otherwise = smallerAndRest x ys

{-# INLINABLE smallerAndRest #-}

smallerAndRestGood :: Ord a => a -> [a] -> (Maybe a, [a])
smallerAndRestGood x ys = go  x ys
    where 
        go  x [] = (Nothing, [])
        go x (y:ys)  | y < x = (Just y, ys)
                        | otherwise = go x ys

{-# INLINABLE smallerAndRestGood #-}


smallerAndRestGoodNotInlined :: Ord a => a -> [a] -> (Maybe a, [a])
smallerAndRestGoodNotInlined x ys = go  x ys
    where 
        go  x [] = (Nothing, [])
        go x (y:ys)  | y < x = (Just y, ys)
                        | otherwise = go x ys


i used

ghc  -O1 -ddump-prep test.hs -fforce-recomp

with the following results

==================== CorePrep ====================
Result size of CorePrep = {terms: 123, types: 192, coercions: 0}

lvl_r10X :: forall a_aTr. (Data.Maybe.Maybe a_aTr, [a_aTr])
[GblId, Caf=NoCafRefs, Str=DmdType m, Unf=OtherCon []]
lvl_r10X =
  \ (@ a_aTr) -> (Data.Maybe.Nothing @ a_aTr, GHC.Types.[] @ a_aTr)

Rec {
Test.smallerAndRest [InlPrag=INLINABLE[ALWAYS], Occ=LoopBreaker]
  :: forall a_aDK.
     GHC.Classes.Ord a_aDK =>
     a_aDK -> [a_aDK] -> (Data.Maybe.Maybe a_aDK, [a_aDK])
[GblId,
 Arity=3,
 Caf=NoCafRefs,
 Str=DmdType <L,U(A,A,C(C1(U)),A,A,A,A,A)><L,U><S,1*U>m,
 Unf=OtherCon []]
Test.smallerAndRest =
  \ (@ a_aTr)
    ($dOrd_s17b :: GHC.Classes.Ord a_aTr)
    (x_s17c :: a_aTr)
    (ds_s17d [Occ=Once!] :: [a_aTr]) ->
    case ds_s17d of _ [Occ=Dead] {
      [] -> lvl_r10X @ a_aTr;
      : y_s17f ys_s17g [Occ=Once*] ->
        case GHC.Classes.< @ a_aTr $dOrd_s17b y_s17f x_s17c
        of _ [Occ=Dead] {
          GHC.Types.False ->
            Test.smallerAndRest @ a_aTr $dOrd_s17b x_s17c ys_s17g;
          GHC.Types.True ->
            let {
              sat_s17i [Occ=Once] :: Data.Maybe.Maybe a_aTr
              [LclId, Str=DmdType]
              sat_s17i = Data.Maybe.Just @ a_aTr y_s17f } in
            (sat_s17i, ys_s17g)
        }
    }
end Rec }

Test.smallerAndRestGood [InlPrag=INLINABLE[ALWAYS]]
  :: forall a_aDJ.
     GHC.Classes.Ord a_aDJ =>
     a_aDJ -> [a_aDJ] -> (Data.Maybe.Maybe a_aDJ, [a_aDJ])
[GblId,
 Arity=3,
 Caf=NoCafRefs,
 Str=DmdType <L,U(A,A,C(C1(U)),A,A,A,A,A)><L,U><S,1*U>m,
 Unf=OtherCon []]
Test.smallerAndRestGood =
  \ (@ a_aTb)
    ($dOrd_s17j :: GHC.Classes.Ord a_aTb)
    (x_s17k [Occ=Once] :: a_aTb)
    (ys_s17l [Occ=Once] :: [a_aTb]) ->
    let {
      lvl1_s17m [Occ=OnceL!] :: a_aTb -> a_aTb -> GHC.Types.Bool
      [LclId, Str=DmdType]
      lvl1_s17m = GHC.Classes.< @ a_aTb $dOrd_s17j } in
    letrec {
      $wgo_s17n [Occ=LoopBreaker]
        :: a_aTb -> [a_aTb] -> (# Data.Maybe.Maybe a_aTb, [a_aTb] #)
      [LclId, Arity=2, Str=DmdType <L,U><S,1*U>, Unf=OtherCon []]
      $wgo_s17n =
        \ (w_s17o :: a_aTb) (w1_s17p [Occ=Once!] :: [a_aTb]) ->
          case w1_s17p of _ [Occ=Dead] {
            [] -> (# Data.Maybe.Nothing @ a_aTb, GHC.Types.[] @ a_aTb #);
            : y_s17r ys1_s17s [Occ=Once*] ->
              case lvl1_s17m y_s17r w_s17o of _ [Occ=Dead] {
                GHC.Types.False -> $wgo_s17n w_s17o ys1_s17s;
                GHC.Types.True ->
                  let {
                    sat_s17u [Occ=Once] :: Data.Maybe.Maybe a_aTb
                    [LclId, Str=DmdType]
                    sat_s17u = Data.Maybe.Just @ a_aTb y_s17r } in
                  (# sat_s17u, ys1_s17s #)
              }
          }; } in
    case $wgo_s17n x_s17k ys_s17l
    of _ [Occ=Dead] { (# ww1_s17w [Occ=Once], ww2_s17x [Occ=Once] #) ->
    (ww1_s17w, ww2_s17x)
    }

Test.$wsmallerAndRestGoodNotInlined
  :: forall a_aql.
     GHC.Classes.Ord a_aql =>
     a_aql -> [a_aql] -> (# Data.Maybe.Maybe a_aql, [a_aql] #)
[GblId,
 Arity=3,
 Caf=NoCafRefs,
 Str=DmdType <L,U(A,A,C(C1(U)),A,A,A,A,A)><L,U><S,1*U>,
 Unf=OtherCon []]
Test.$wsmallerAndRestGoodNotInlined =
  \ (@ a_aql)
    (w_s17y :: GHC.Classes.Ord a_aql)
    (w1_s17z [Occ=Once] :: a_aql)
    (w2_s17A [Occ=Once] :: [a_aql]) ->
    let {
      lvl1_s17B [Occ=OnceL!] :: a_aql -> a_aql -> GHC.Types.Bool
      [LclId, Str=DmdType]
      lvl1_s17B = GHC.Classes.< @ a_aql w_s17y } in
    letrec {
      $wgo_s17C [Occ=LoopBreaker]
        :: a_aql -> [a_aql] -> (# Data.Maybe.Maybe a_aql, [a_aql] #)
      [LclId, Arity=2, Str=DmdType <L,U><S,1*U>, Unf=OtherCon []]
      $wgo_s17C =
        \ (w3_s17D :: a_aql) (w4_s17E [Occ=Once!] :: [a_aql]) ->
          case w4_s17E of _ [Occ=Dead] {
            [] -> (# Data.Maybe.Nothing @ a_aql, GHC.Types.[] @ a_aql #);
            : y_s17G ys_s17H [Occ=Once*] ->
              case lvl1_s17B y_s17G w3_s17D of _ [Occ=Dead] {
                GHC.Types.False -> $wgo_s17C w3_s17D ys_s17H;
                GHC.Types.True ->
                  let {
                    sat_s17J [Occ=Once] :: Data.Maybe.Maybe a_aql
                    [LclId, Str=DmdType]
                    sat_s17J = Data.Maybe.Just @ a_aql y_s17G } in
                  (# sat_s17J, ys_s17H #)
              }
          }; } in
    $wgo_s17C w1_s17z w2_s17A

Test.smallerAndRestGoodNotInlined [InlPrag=INLINE[0]]
  :: forall a_aql.
     GHC.Classes.Ord a_aql =>
     a_aql -> [a_aql] -> (Data.Maybe.Maybe a_aql, [a_aql])
[GblId,
 Arity=3,
 Caf=NoCafRefs,
 Str=DmdType <L,U(A,A,C(C1(U)),A,A,A,A,A)><L,U><S,1*U>m,
 Unf=OtherCon []]
Test.smallerAndRestGoodNotInlined =
  \ (@ a_aql)
    (w_s17K [Occ=Once] :: GHC.Classes.Ord a_aql)
    (w1_s17L [Occ=Once] :: a_aql)
    (w2_s17M [Occ=Once] :: [a_aql]) ->
    case Test.$wsmallerAndRestGoodNotInlined
           @ a_aql w_s17K w1_s17L w2_s17M
    of _ [Occ=Dead] { (# ww1_s17O [Occ=Once], ww2_s17P [Occ=Once] #) ->
    (ww1_s17O, ww2_s17P)
    }


this seems to hint that doing worker wrapper by hand works, whether or not you marked it INLINEABLE.

(i don't know enough about this topic to judge if that means its resolved or still a problem mind you)

comment:6 Changed 5 years ago by thoughtpolice

Milestone: 7.8.37.10.1

Moving to 7.10.1.

comment:7 Changed 5 years ago by Simon Peyton Jones <simonpj@…>

In 9cf5906b692c31b7ec67856b0859cb0e33770651/ghc:

Make worker/wrapper work on INLINEABLE things

This fixes a long-standing bug: Trac #6056.  The trouble was that
INLINEABLE "used up" the unfolding for the Id, so it couldn't be
worker/wrapper'd by the strictness analyser.

This patch allows the w/w to go ahead, and makes the *worker* INLINEABLE
instead, so it can later be specialised.

However, that doesn't completely solve the problem, because the dictionary
argument (which the specialiser treats specially) may be strict and
hence unpacked by w/w, so now the worker won't be specilialised after all.

Solution: never unpack dictionary arguments, which is done by the isClassTyCon
          test in WwLib.deepSplitProductType_maybe

comment:8 Changed 5 years ago by Simon Peyton Jones <simonpj@…>

In e5f766c392c8c1cb329e1409102b5655c3c253c9/ghc:

Give the worker for an INLINABLE function a suitably-phased Activation

See Note [Activation for INLINABLE worker].  This was preventing
Trac #6056 from working.

comment:9 Changed 5 years ago by Simon Peyton Jones <simonpj@…>

In 393506269315bca66aae91517b75e0a003470c84/ghc:

Finally!  Test Trac #6056

comment:10 Changed 5 years ago by simonpj

Resolution: fixed
Status: newclosed
Test Case: simplCore/should_compile/T6056

I think I have finally nailed this. Yell if it doesn't work for you.

Simon

Note: See TracTickets for help on using tickets.