Merge FUN_STATIC closure with its SRT
ClosedPublic

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

Details

Summary

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%).

Results:

  • 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

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
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.
compiler/cmm/Cmm.hs
143

Should these fields be strict to force the lookups?

compiler/cmm/CmmBuildInfoTables.hs
474

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
  ]
585

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

751

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
compiler/cmm/Cmm.hs
145

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

compiler/cmm/CmmBuildInfoTables.hs
474

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

550

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.

570

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

585

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

compiler/codeGen/StgCmmClosure.hs
1048

Nit: Alignment.

rts/sm/Scav.c
1725–1727

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=]
           scavenge_fun_srt(info);
           ^~~~~~~~~~~~~~~~~~~~~~
     |
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.