Opened 9 years ago

Last modified 2 years ago

#4945 new bug

Another SpecConstr infelicity

Reported by: batterseapower Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.0.1
Keywords: SpecConstr Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case: simplCore/should_compile/T4945
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

I'm beginning to sound like a broken record, but SpecConstr still doesn't seem to be right! The last problem has been fixed, but I've found a new one.

Please observe the output of compiling the attached code with:

./ghc -fforce-recomp -c -dverbose-core2core -O2 -fno-liberate-case STUArray-Rewrite2.hs

In the output of SpecConstr we have a local letrec:

(letrec {
   $wa_s1G7 [Occ=LoopBreaker]
     :: forall s_aJm.
        Data.Array.Base.STUArray
          s_aJm GHC.Types.Int GHC.Types.Int
        -> GHC.Prim.Int#
        -> GHC.Prim.State# s_aJm
        -> (# GHC.Prim.State# s_aJm, () #)
   [LclId, Arity=3, Str=DmdType LLL]
   $wa_s1G7 =
     \ (@ s_aJm)
       (w_s1FS
          :: Data.Array.Base.STUArray
               s_aJm GHC.Types.Int GHC.Types.Int)
       (ww_s1FV :: GHC.Prim.Int#)
       (w_s1FX :: GHC.Prim.State# s_aJm) ->
       case GHC.Prim.># ww_s1FV ww_s1FN
       of wild_Xj [Dmd=Just A] {
         GHC.Types.False ->
           case w_s1FS
           of wild_aTj [Dmd=Just L]
           { Data.Array.Base.STUArray ds1_aTl [Dmd=Just U]
                                      ds2_aTm [Dmd=Just U]
                                      n_aTn [Dmd=Just U(L)]
                                      ds3_aTo [Dmd=Just A] ->
           case n_aTn
           of wild_aTs [Dmd=Just A]
           { GHC.Types.I# x_aTu [Dmd=Just L] ->
           case $wa_s1G0
                  @ s_aJm
                  w_s1FS
                  (GHC.Types.I# ww_s1FV)
                  0
                  (GHC.Prim.-# x_aTu 1)
                  w_s1FX
           of wild_XUw [Dmd=Just A]
           { (# new_s_XUB [Dmd=Just L], r_XUD [Dmd=Just A] #) ->
           $wa_s1G7
             @ s_aJm w_s1FS (GHC.Prim.+# ww_s1FV 1) new_s_XUB
           }
           }
           };
         GHC.Types.True -> (# w_s1FX, GHC.Unit.() #)
       }; } in
 $wa_s1G7)

This is a local recursive loop with an invariant first argument (w_s1FS) that is recrutinised every time! This seems deeply uncool.

This is with HEAD (7.1.20110203, incorporating the patch "Fix typo in SpecConstr that made it not work at all")

Attachments (1)

STUArray-Rewrite2.hs (783 bytes) - added by batterseapower 9 years ago.

Download all attachments as: .zip

Change History (20)

Changed 9 years ago by batterseapower

Attachment: STUArray-Rewrite2.hs added

comment:1 Changed 9 years ago by batterseapower

I see what is going on here. Since this is a local letrec, you don't generate any specialisations from the RHS call patterns unless at least one call pattern is boring. However, since the body doesn't apply *any* arguments at all to the function, SpecConstr doesn't detect any calls! How amusing :-)

A quick and easy hack that would probably solve it would be to replace SpecConstr:1486 with:

any isNothing mb_pats || null mb_pats

I can't test this right now.

comment:2 Changed 9 years ago by batterseapower

OK, I did actually manage to try my fix, and I'm still not getting specialisation of that function. That observation might be part of the problem, but it's not the whole story.

comment:3 Changed 9 years ago by batterseapower

OK, I got it. All you need to do is replace varUsage with:

varUsage :: ScEnv -> OutVar -> ArgOcc -> ScUsage
varUsage env v use = case lookupHowBound env v of
    Just RecArg -> SCU { scu_calls = emptyVarEnv
                       , scu_occs = unitVarEnv v use }
    Just RecFun -> SCU { scu_calls = unitVarEnv v [(emptyVarEnv, [])] -- Added so we get specialisations even if function used in trivial way (i.e. as a variable)
                       , scu_occs = emptyVarEnv }
    Nothing -> nullUsage

Otherwise the pattern guard on (lookupVarEnv bind_calls fn) in specialise fails, so we don't even get to call callsToPats.

I think this is the right change, because it should also mean that we get call pattern specialisations for locally-bound functions that are e.g. stored inside a data structure.

comment:4 Changed 9 years ago by simonpj

Status: newmerge
Test Case: simplCore/should_compile/T4935

Right. I came to the same conclusion.

Mon Feb  7 10:25:37 GMT 2011  simonpj@microsoft.com
  * Fix Trac #4945: another SpecConstr infelicity
  
  Well, more a plain bug really, which led to SpecConstr
  missing some obvious opportunities for specialisation.
  
  Thanks to Max Bolingbroke for spotting this.

    M ./compiler/specialise/SpecConstr.lhs -21 +35

Simon

comment:5 Changed 9 years ago by igloo

Milestone: 7.0.2

comment:6 Changed 9 years ago by simonpj

Test Case: simplCore/should_compile/T4935simplCore/should_compile/T4945

comment:7 Changed 9 years ago by igloo

Resolution: fixed
Status: mergeclosed

Only performance, and not a regression since 7.0.1 at least, so not merging at this stage of the release cycle.

comment:8 Changed 4 years ago by Thomas Miedema <thomasmiedema@…>

In 506522c95f5d43db4d469135878c56fe20eb81f6/ghc:

Testsuite: mark T4945 as expect_broken (#4945)

In commit 7d519dabd2006c9742e82fce02df55704da15482, the file
T4945.stdout was added to the repository, to make T4945 pass
validatation presumably.

When that test produces output however, there is a bug somewhere, and we
shouldn't hide it. There is a comment in the Makefile which says:

    "When SpecConstr works there are no STUArrays at all"

So here we remove T4945.stdout again, mark T4945 as expect_broken, and
reopen the ticket.

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

comment:9 Changed 4 years ago by thomie

Milestone: 7.0.27.12.1
Priority: normalhigh
Resolution: fixed
Status: closednew

Marking regression with priority high.

comment:10 Changed 4 years ago by Simon Peyton Jones <simonpj@…>

In 5d98b6828f65ce6eea45e93880928b7031955d38/ghc:

Trac #4945 is working again

This test greps in the ouput of -ddump-simpl, so it's fragile.
It stopped working for a while, but now works again. I don't know
why, but I don't have time to investigate, so I'll just mark it
as ok.

comment:11 Changed 4 years ago by simonpj

Resolution: fixed
Status: newclosed

I'm closing this again.

comment:12 Changed 4 years ago by bgamari

Resolution: fixed
Status: closednew

It seems that the patch that caused #10527 is likely responsible for this test suddenly starting to work again. Unfortunately, when we fixed #10527 the test broke again. I'm going to again mark it as expect_broken and reopen this ticket.

comment:13 Changed 4 years ago by bgamari

Resolution: fixed
Status: newclosed

Actually, it turns out that thomie's fix to the testcase, 506522c95f5d43db4d469135878c56fe20eb81f6, never made it to the ghc-7.10 branch. This resolves this issue.

comment:14 Changed 4 years ago by Thomas Miedema <thomasmiedema@…>

In 50b9a7a3/ghc:

Revert "Trac #4945 is working again"

This reverts commit 5d98b6828f65ce6eea45e93880928b7031955d38.

comment:15 Changed 4 years ago by thomie

Resolution: fixed
Status: closednew

Let's leave it open until it is fixed for good.

comment:16 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:17 Changed 4 years ago by thomie

Priority: highnormal

To summarise: test T4945 is failing.

Lowering priority, because it is just some (-O2) runtime performance issue.

comment:18 Changed 4 years ago by thomie

Milestone: 8.0.1

comment:19 Changed 2 years ago by mpickering

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