Merge FUN_STATIC closure with its SRT

Authored by simonmar on Apr 26 2018, 11:37 AM.



The idea here is to save a little code size and some work in the GC,
by collapsing FUN_STATIC closures and their SRTs.

This is (4) in a series; see D4632 for more details.

There's a tradeoff here: more complexity in the compiler in exchange
for a modest code size reduction (probably around 0.5%).


  • GHC binary itself (statically linked) is 1% smaller
  • -0.2% binary sizes in nofib (-0.5% module sizes)

Full nofib results comparing D4634 with this: P177 (ignore runtimes,
these aren't stable on my laptop)

Test Plan

validate, nofib

Diff Detail

rGHC Glasgow Haskell Compiler
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
simonmar created this revision.Apr 26 2018, 11:37 AM
dfeuer added a subscriber: dfeuer.Apr 26 2018, 4:58 PM
dfeuer added inline comments.

Should these fields be strict to force the lookups?


Maybe this? I'm guessing you were thinking in this direction when you spoke of "blocks" in the plural.

getStaticFuns :: [CmmDecl] -> [(BlockId, CLabel)]
getStaticFuns decls =
  [ (g_entry g, lbl)
  | decl <- decls
  , CmmProc top_info _ _ g <- decl
  , Just info <- mapLookup (g_entry g) (info_tbls top_info)
  , let rep = cit_rep info
  , Just (id, _) <- cit_clo info
  , let lbl = mkLocalClosureLabel (idName id) (idCafInfo id)
  , isStaticRep rep && isFunRep rep

foldr Set.delete looks very surprising. Surely this should be foldl' (flip Set.delete).


Should some of these bindings be strict? I would think anything not expensive and anything sure to be used probably should be.

simonmar requested review of this revision.Apr 30 2018, 10:43 AM
bgamari added inline comments.May 3 2018, 2:51 PM

This comment really could use more description given that this field doesn't map to anything defined in the C info table structure.


Indeed this refactoring does seem better given that we are going to concatMap over getStaticFuns anyways.


I do wish there were type annotations on this. Working on the types of all three of these binders in my head was not easy.


A comment on this would be helpful. Perhaps this should even just be a type synonym?


Agreed; we've been trying to slowly eliminate this sort of foldl and foldr usage.


Nit: Alignment.


It's not clear to me that this will match any of the patterns suppressing gcc's -Wimplicit-fallthrough warning (note that level -Wimplicit-fallthrough=3 is the default and that the regexpr must match the entire comment). Perhaps you want to add another /* -fallthough */ comment?

simonmar updated this revision to Diff 16310.May 4 2018, 7:41 AM

address comments

It looks like there are still a number of comments that remain unaddressed.

This revision was not accepted when it landed; it landed in state Needs Review.May 16 2018, 7:36 AM
This revision was automatically updated to reflect the committed changes.

I pushed this but forgot that there were still comments to address... sorry about that, will address them in a follow up.

rts/sm/Scav.c: In function ‘scavenge_static’:

rts/sm/Scav.c:1724:7: error:
     error: this statement may fall through [-Werror=implicit-fallthrough=]
1724 |       scavenge_fun_srt(info);
     |       ^

rts/sm/Scav.c:1729:5: error:
     note: here
         case CONSTR:
1729 |     case CONSTR:
     |     ^
cc1: all warnings being treated as errors
`gcc' failed in phase `C Compiler'. (Exit code: 1)

on gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0

I actually think we may want to revert this series as it broke the build on i386.

I believe I fixed the breakage in 3310f7f14c0ba34a57fe5a77f47d2a66fe838a43 so perhaps reversion won't be necessary afterall.

on gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0

Oh, my gcc is too old to have this warning, so it validated for me. I'll fix this.

Thanks, will take a look.