Another SpecConstr infelicity

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")

Trac metadata

Trac field	Value
Version	7.0.1
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

mentioned in issue #4941

changed weight to 5

added Tbug Trac import labels

Attached file STUArray-Rewrite2.hs ($559).

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.

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.

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.

mentioned in commit 28ca359b

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

Trac metadata

Trac field	Value
Test case	→ simplCore/should_compile/T4935

added 1 deleted label

changed milestone to %7.0.2

Trac metadata

Trac field	Value
Test case	simplCore/should_compile/T4935 → simplCore/should_compile/T4945

closed

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

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

removed 1 deleted label

mentioned in commit 506522c9

reopened

removed milestone

changed weight to 7

Marking regression with priority high.

Trac metadata

Trac field	Value
Priority	normal → high
Resolution	ResolvedFixed → Unresolved

mentioned in issue #10527 (closed)

mentioned in commit 5d98b682

closed

I'm closing this again.

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

reopened

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

Trac metadata

Trac field	Value
Resolution	ResolvedFixed → Unresolved

closed

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

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

mentioned in commit 50b9a7a3

reopened

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

Trac metadata

Trac field	Value
Resolution	ResolvedFixed → Unresolved

changed milestone to %8.0.1

Milestone renamed

changed weight to 5

To summarise: test T4945 is failing.

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

Trac metadata

Trac field	Value
Priority	high → normal

added runtime perf label

added Pnormal label

As of 128fd85c, the T4945 test is on master and enabled.

Is there anything left to do here?