[GHC] #4962: Dead code fed to CorePrep because RULEs keep it alive spuriously

[GHC]

#4962: Dead code fed to CorePrep because RULEs keep it alive spuriously
--------------------------------------+-------------------------------------
  Reporter:  batterseapower           |          Owner:                  
      Type:  bug                      |         Status:  closed          
  Priority:  normal                   |      Milestone:  7.2.1           
 Component:  Compiler                 |        Version:  7.0.1           
Resolution:  fixed                    |       Keywords:                  
        Os:  Unknown/Multiple         |   Architecture:  Unknown/Multiple
   Failure:  Runtime performance bug  |     Difficulty:  Unknown         
  Testcase:                           |      Blockedby:                  
  Blocking:                           |        Related:                  
--------------------------------------+-------------------------------------
Changes (by simonpj):

  * difficulty:  => Unknown


Old description:

> This ticket is split off from #4941.
>
> I'm seeing output code that looks roughly like this in the final
> simplifier output:
>
> {{{
> let
>       {-# RULES go (Just x) = $sgo_s1lg x #-}
>       go = ... go ... $w$sgo ...
>       $sgo_s1lg = ... $w$sgo_s1mv ...
>       $w$sgo_s1mv = ... big ...
> in ... $w$sgo_s1mv ...
> }}}
>
> This is bad because $sgo is will be dead for the purposes of code
> generation, but currently GHC will allocate a closure for it at runtime
> anyway.
>
> What we should do is drop the dead binding once we have decided that it's
> not reachable via the exported RULES in the interface file.
>
> I've confirmed that this patch achieves this effect (and eliminates the
> main source of increased allocations I was seeing in #4941):
>
> {{{
> hunk ./compiler/main/TidyPgm.lhs 22
> +import OccurAnal        ( occurAnalysePgm )
> hunk ./compiler/main/TidyPgm.lhs 43
> +import qualified ErrUtils as Err
> hunk ./compiler/main/TidyPgm.lhs 47
> +import PprCore
> hunk ./compiler/main/TidyPgm.lhs 322
> +
> +          -- Occurrence analyse the input program, assuming all local
> rules are off,
> +          -- but retaining any bindings referred to by external rules.
> +          -- Occurrence information may have improved since the last run
> of the
> +          -- simplifier because some bindings will become dead once
> RULES are dropped.
> +          --
> +          -- The analysis itself will take care of dropping any newly-
> dead syntax.
> +        ; Err.dumpIfSet_dyn dflags Opt_D_dump_occur_anal "BEFORE TidyPgm
> occurrence analysis" (pprCoreBindings binds)
> +        ; binds <- return $ occurAnalysePgm Nothing ext_rules binds
> +        ; Err.dumpIfSet_dyn dflags Opt_D_dump_occur_anal "TidyPgm
> occurrence analysis" (pprCoreBindings binds)
> +
> hunk ./compiler/simplCore/OccurAnal.lhs 393
> -          rule_fvs   = idRuleVars bndr          -- See Note [Rule
> dependency info]
> +          rule_fvs   = case occ_rule_act env of Nothing -> emptyVarSet;
> Just _ -> idRuleVars bndr          -- See Note [Rule dependency info]
> +          -- FIXME: this is a terrible hack to try to have OccAnal drop
> bindings that are kept alive only due to rules at the CoreTidy stage
> }}}
>
> However the patch is a bit horrible: because attached RULES keep bindings
> alive in a LetRec even if the rules can never be activated (i.e.
> occ_rule_act env == Nothing) the bindings I want to be dropped never get
> dropped. The reason I consider my fix a hack is that just because the
> rules are inactive *now* doesn't mean that they will be inactive *later*.
>
> A better approach would perhaps be to wait until the RULES are stripped
> from the binders entirely (e.g. OccAnal the output of CoreTidy). However,
> this is not straightforward because CoreTidy has globalised Ids, so
> OccAnaling the output says that all top level bindings have no FVs and
> hence turns all LetRecs into Lets!

New description:

 This ticket is split off from #4941.

 I'm seeing output code that looks roughly like this in the final
 simplifier output:

 {{{
 let
       {-# RULES go (Just x) = $sgo_s1lg x #-}
       go = ... go ... $w$sgo ...
       $sgo_s1lg = ... $w$sgo_s1mv ...
       $w$sgo_s1mv = ... big ...
 in ... $w$sgo_s1mv ...
 }}}

 This is bad because $sgo is will be dead for the purposes of code
 generation, but currently GHC will allocate a closure for it at runtime
 anyway.

 What we should do is drop the dead binding once we have decided that it's
 not reachable via the exported RULES in the interface file.

 I've confirmed that this patch achieves this effect (and eliminates the
 main source of increased allocations I was seeing in #4941):

 {{{
 hunk ./compiler/main/TidyPgm.lhs 22
 +import OccurAnal        ( occurAnalysePgm )
 hunk ./compiler/main/TidyPgm.lhs 43
 +import qualified ErrUtils as Err
 hunk ./compiler/main/TidyPgm.lhs 47
 +import PprCore
 hunk ./compiler/main/TidyPgm.lhs 322
 +
 +          -- Occurrence analyse the input program, assuming all local
 rules are off,
 +          -- but retaining any bindings referred to by external rules.
 +          -- Occurrence information may have improved since the last run
 of the
 +          -- simplifier because some bindings will become dead once RULES
 are dropped.
 +          --
 +          -- The analysis itself will take care of dropping any newly-
 dead syntax.
 +        ; Err.dumpIfSet_dyn dflags Opt_D_dump_occur_anal "BEFORE TidyPgm
 occurrence analysis" (pprCoreBindings binds)
 +        ; binds <- return $ occurAnalysePgm Nothing ext_rules binds
 +        ; Err.dumpIfSet_dyn dflags Opt_D_dump_occur_anal "TidyPgm
 occurrence analysis" (pprCoreBindings binds)
 +
 hunk ./compiler/simplCore/OccurAnal.lhs 393
 -          rule_fvs   = idRuleVars bndr          -- See Note [Rule
 dependency info]
 +          rule_fvs   = case occ_rule_act env of Nothing -> emptyVarSet;
 Just _ -> idRuleVars bndr          -- See Note [Rule dependency info]
 +          -- FIXME: this is a terrible hack to try to have OccAnal drop
 bindings that are kept alive only due to rules at the CoreTidy stage
 }}}

 However the patch is a bit horrible: because attached RULES keep bindings
 alive in a !LetRec even if the rules can never be activated (i.e.
 occ_rule_act env == Nothing) the bindings I want to be dropped never get
 dropped. The reason I consider my fix a hack is that just because the
 rules are inactive *now* doesn't mean that they will be inactive *later*.

 A better approach would perhaps be to wait until the RULES are stripped
 from the binders entirely (e.g. !OccAnal the output of !CoreTidy).
 However, this is not straightforward because !CoreTidy has globalised Ids,
 so OccAnaling the output says that all top level bindings have no FVs and
 hence turns all !LetRecs into Lets!

--

-- 
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/4962#comment:11>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler