Opened 5 years ago

Last modified 16 months ago

#7803 new bug

Superclass methods are left unspecialized

Reported by: akio Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.6.2
Keywords: Inlining Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

In the attached code, Foo.foo1 gets compiled (by ghc -O2) into a call to the unspecialized version of Lib.exp5Tail, even though all the related functions are marked INLINE or SPECIALIZE.

The problem seems to be that GHC creates a top-level overloaded function after the specialization pass, so the function never gets specialized.

This might be the same issue as #7785, but the workaround I posted there does not work here.

Attachments (3)

Foo.hs (396 bytes) - added by akio 5 years ago.
Lib.hs (1.9 KB) - added by akio 5 years ago.
analyze.log (189.6 KB) - added by bgamari 22 months ago.
nofib-analyze output

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by akio

Attachment: Foo.hs added

Changed 5 years ago by akio

Attachment: Lib.hs added

comment:1 Changed 5 years ago by akio

This particular test case needs ConstraintKinds but I found that the general problem can be reproduced without it. I can try to make a test case that doesn't use ConstraintKinds if that seems useful.

comment:2 Changed 5 years ago by igloo

difficulty: Unknown
Milestone: 7.8.1

Thanks for the report

comment:3 Changed 4 years ago by simonpj

OK I know what is going on. At the moment we do *no* inlining before specialisation; but in your program you really, really want those INLINE functions to inline before specialisation takes place.

So I made a tiny change to SimplCore:

diff --git a/compiler/simplCore/SimplCore.lhs b/compiler/simplCore/SimplCore.lhs
index 62e167a..86f4f4e 100644
--- a/compiler/simplCore/SimplCore.lhs
+++ b/compiler/simplCore/SimplCore.lhs
@@ -179,7 +179,7 @@ getCoreToDo dflags
                        (base_mode { sm_phase = InitialPhase
                                   , sm_names = ["Gentle"]
                                   , sm_rules = rules_on   -- Note [RULEs enabled in SimplGently]
-                                  , sm_inline = False
+                                  , sm_inline = True
                                   , sm_case_case = False })
                           -- Don't do case-of-case transformations.
                           -- This makes full laziness work better

Bingo. We get great code for foo1:

Foo.foo1 =
  \ (z_ahb :: GHC.Types.Double) (v_ahc :: GHC.Types.Double) ->
    case v_ahc of _ { GHC.Types.D# x_arD ->
    case z_ahb of _ { GHC.Types.D# x1_Xsx ->
    case GHC.Prim.logDouble# x_arD of wild2_ari { __DEFAULT ->
    case GHC.Prim.logDouble# x1_Xsx of wild3_Xsf { __DEFAULT ->
    GHC.Types.D#
      (GHC.Prim.-##
         (GHC.Prim.*##
            x_arD (GHC.Prim./## (GHC.Prim.negateDouble# wild2_ari) 120.0))
         (GHC.Prim.*##
            x1_Xsx (GHC.Prim./## (GHC.Prim.negateDouble# wild3_Xsf) 120.0)))
    } } } }

(Actually to be fair I also eta-expanded foo and bar, but the specialisation takes place nicely either way.)

I'll try this out on a full nofib run next week.

Simon

comment:4 in reply to:  3 Changed 4 years ago by akio

Replying to simonpj:

I'll try this out on a full nofib run next week.

How was the result?

comment:5 Changed 4 years ago by simonpj

Thanks for reminding. Still pending I'm afraid. Maybe next week. Simon

comment:6 Changed 4 years ago by thoughtpolice

Milestone: 7.8.37.10.1

Moving to 7.10.1

comment:7 Changed 3 years ago by thomie

Simon's suggested change from comment:3 did not make it into the ghc codebase. I am not sure what the status of this ticket is.

Last edited 3 years ago by thomie (previous) (diff)

comment:8 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:9 Changed 2 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:10 Changed 22 months ago by thomie

Milestone: 8.0.1

comment:11 Changed 22 months ago by crockeea

Can someone take another look at this ticket? It sounds like Simon's suggestion just needs to be merged.

comment:12 Changed 22 months ago by bgamari

I've started a pair of builds; I'll put up nofib results once they have finished.

comment:13 Changed 22 months ago by bgamari

Here is a nofib comparison based on 38af3d1db2889423a12a2232b9d52181bba23d75 with and without the following patch,

  • compiler/simplCore/SimplCore.hs

    diff --git a/compiler/simplCore/SimplCore.hs b/compiler/simplCore/SimplCore.hs
    index 6badbf8..c573b45 100644
    a b getCoreToDo dflags 
    182182                       (base_mode { sm_phase = InitialPhase
    183183                                  , sm_names = ["Gentle"]
    184184                                  , sm_rules = rules_on   -- Note [RULEs enabled in SimplGently]
    185                                   , sm_inline = False
     185                                  , sm_inline = True
    186186                                  , sm_case_case = False })
    187187                          -- Don't do case-of-case transformations.
    188188                          -- This makes full laziness work better

Unfortunately things don't look terribly promising,

        Program           Size    Allocs   Runtime   Elapsed  TotalMem
--------------------------------------------------------------------------------
            Min          -0.3%    -95.0%     -0.6%     -1.2%      0.0%
            Max          +0.9%    +10.6%    +14.3%    +14.3%   +100.0%
 Geometric Mean          +0.4%     -2.8%     +1.6%     +1.5%     +0.8%

Changed 22 months ago by bgamari

Attachment: analyze.log added

nofib-analyze output

comment:14 Changed 16 months ago by mpickering

Keywords: Inlining added
Note: See TracTickets for help on using tickets.