Opened 10 years ago

Closed 10 years ago

#4064 closed bug (fixed)

SpecConstr broken for NOINLINE loops in 6.13

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



foo :: Int -> Int -> Int
{-# NOINLINE foo #-}
foo n k | n <= 0    = k
        | otherwise = foo (n-1) (k+1)

This is what GHC generates:

Rec {
X.foo_$sfoo [Occ=LoopBreaker]
  :: GHC.Prim.Int# -> GHC.Prim.Int# -> GHC.Types.Int
[GblId, Arity=2, Caf=NoCafRefs, Str=DmdType LL]
X.foo_$sfoo =
  \ (sc_sjW :: GHC.Prim.Int#) (sc1_sjX :: GHC.Prim.Int#) ->
    case GHC.Prim.<=# sc1_sjX 0 of _ {
      GHC.Bool.False ->
          (GHC.Types.I# (GHC.Prim.-# sc1_sjX 1))
          (GHC.Types.I# (GHC.Prim.+# sc_sjW 1));
      GHC.Bool.True -> GHC.Types.I# sc_sjW
    } [InlPrag=NOINLINE (sat-args=2), Occ=LoopBreaker]
  :: GHC.Types.Int -> GHC.Types.Int -> GHC.Types.Int
[GblId, Arity=2, Caf=NoCafRefs, Str=DmdType U(L)U(L)m] =
  \ (n_aaj :: GHC.Types.Int) (k_aak :: GHC.Types.Int) ->
    case n_aaj of _ { GHC.Types.I# x_ajn ->
    case GHC.Prim.<=# x_ajn 0 of _ {
      GHC.Bool.False ->
        case k_aak of _ { GHC.Types.I# x1_aiX ->
          (GHC.Types.I# (GHC.Prim.-# x_ajn 1))
          (GHC.Types.I# (GHC.Prim.+# x1_aiX 1))
      GHC.Bool.True -> k_aak
end Rec }

------ Local rules for imported ids --------
"SC:X.foo0" [NEVER]
    forall {sc_sjW :: GHC.Prim.Int# sc1_sjX :: GHC.Prim.Int#} (GHC.Types.I# sc1_sjX) (GHC.Types.I# sc_sjW)
      = X.foo_$sfoo sc_sjW sc1_sjX

At the moment, we don't worker/wrapper NOINLINE things because doing so would generate a wrapper which wouldn't be inlined. But apparently, we do SpecConstr NOINLINE functions, generating rules and specialisations which can never be used, like above.

The trivial solution is to have SpecConstr ignore NOINLINE functions. Perhaps we could also think about transforming foo to this instead:

{-# NOINLINE foo #-}
foo = foo'

foo' n k | n <= 0    = k
         | otherwise = foo' (n-1) (k+1)

Now, we can apply worker/wrapper and SpecConstr to foo' without any restrictions. One could argue that foo has been "inlined" into its own rhs but I can't imagine how that could ever be a problem.

Change History (2)

comment:1 Changed 10 years ago by simonpj

When GHC generates a specialisation for a function (in Specialise or SpecConstr) it transfers the inline-activation to the new rule. Thus if if you say NOINLINE[1] for the function, the rule will be inactive until phase 1, just like the inlining. Why? Because you said NOINLINE for a reason, probably to make sure that a rule would fire. Eg

f :: Int -> Int
{-# NOINLINE[1] f #-}
f x = ...

{-# RULE "Blob" forall a.  h (f x) = k x #-}

The NOINLINE makes sure 'f' doesn't get inlined until the RULE has had a chance to fire. And exactly the same goes for any automatically generated specialisations of f. We don't want to generate a rule

{-# RULE "Spec" forall x. f (I# x) = $sf x #-}

because firing that rule will prevent rule "Blob" from firing.

In this case you'd marked the function NOINLINE, so it'll never be inlined and its rules will never fire. You're right that in that case it is stupid to generate specialisations that will never be used. I will fix that.

Mind you, it's a bit odd to have a NOINLINE on a recursive function in the first place, isn't it? GHC doesn't inline recursive functions anyway. Perhaps the reason is to make sure rules will fire?

Your foo=foo' idea is quite attractive, although it seems a little ad-hoc (eg NOLININE[1] things are presumably unaffected). And it's not so clear what to do in the case of mutual recursion. I'm inclined to leave this for the programmer to do, at least unless it becomes common.

comment:2 Changed 10 years ago by simonpj

Resolution: fixed
Status: newclosed

This patch stops SpecConstr doing anything for NOINLINE things

* Don't do SpecConstr on NOINLINE things (Trac #4064)
  Since the RULE from specialising gets the same Activation as
  the inlining for the Id itself there's no point in specialising
  a NOINLINE thing, because the rule will be permanently switched
  See Note [Transfer activation] in SpecConstr
  and Note [Auto-specialisation and RULES] in Specialise.

    M ./compiler/specialise/SpecConstr.lhs -1 +9

I think that's enough to close the ticket


Note: See TracTickets for help on using tickets.