Remove StgBinderInfo and related computation in CoreToStg
ClosedPublic

Authored by osa1 on Oct 17 2018, 6:21 AM.

Details

Summary
  • The StgBinderInfo type was never used in the code gen, so the type, related computation in CoreToStg, and some comments about it are removed. See Trac #15770 for more details.
  • Simplified CoreToStg after removing the StgBinderInfo computation: removed StgBinderInfo arguments and mfix stuff.

The StgBinderInfo values were not used in the code gen, but I still run nofib
just to make sure: 0.0% change in allocations and binary sizes.

Test Plan

Validated locally

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.
osa1 created this revision.Oct 17 2018, 6:21 AM
osa1 updated this revision to Diff 18348.Oct 17 2018, 6:23 AM
  • Remove more comments

Is there a Trac ticket for this? Shouldn't there be one?

The comment in line 1024 of CoreToStg says

-- StgBinderInfo records how it occurs; notably, we
-- are interested in whether it only occurs in saturated
-- applications, because then we don't need to build a
-- curried version.

and that makes sense. If we know there are only saturated calls, we don't need a slow entry point. Maybe we aren't exploiting this opportunity right now, but could we not do so?

Even if we decide to do so, it'd be better to add this info in the (upcoming) finall free-var pass just before code gen.

osa1 added a comment.Oct 18 2018, 3:24 AM

I found some relevant comments/code about this optimisation, and filed a ticket: Trac #15770

I think we could remove this, it's a very small code-size optimisation and would require a fair bit more work to implement, not at all clear that it's worth it. I commented more on the ticket.

osa1 updated this revision to Diff 18375.Oct 19 2018, 2:54 AM
  • Rebase
  • Remove more comments
osa1 edited the summary of this revision. (Show Details)Oct 19 2018, 2:55 AM

Generally I'm fine with this. Worthwhile simplification

compiler/codeGen/StgCmmClosure.hs
626

I'd be sad to lose this comment entirely. For a start it lists all the stuff we generate for each closure, which is a useful thing to day. It would be a useful overview esp if it points to the places in the places in the code generator where we actually generate the stuff

Second, pointing out that we could sometimes omit some of it, but decided not to, is useful to. Pointing to the ticket where we discuss why not to do it.

compiler/stgSyn/CoreToStg.hs
458

This is completely separate from the declared purpose of this patch, right? I suggest making it a separate patch

osa1 added inline comments.Oct 19 2018, 1:14 PM
compiler/stgSyn/CoreToStg.hs
458

Ah, sorry, this shouldn't be in this patch. This is yet another unused optmisation. I'll revert.

osa1 updated this revision to Diff 18398.Oct 22 2018, 2:39 AM
  • Revert removed dead case bndr update
osa1 marked 2 inline comments as done.Oct 22 2018, 2:41 AM
osa1 added a subscriber: sgraf.

I offered my opinion in https://ghc.haskell.org/trac/ghc/ticket/15770#comment:5:

I'm conveniently using satCallsOnly when performing ​late lambda lifting. It would not be terribly upsetting to see this go, but I found it convenient not having to compute the same analysis info myself.

The same arguments as for free variables apply, though: It's occurrence information which is probably fragile enough to just be generated on demand.

I looked at StgBinderInfo when I was looking at https://ghc.haskell.org/trac/ghc/ticket/15560 in the past.

So I think it could be useful. But then we could always re-add it later as well.

bgamari added a comment.EditedOct 28 2018, 11:27 AM

What is the status of this, @osa1?

sgraf added a comment.EditedNov 5 2018, 10:13 AM

It would be great if we could merge this, as this affects D5224.

Omer, is this good to go? If so, commit!

sgraf accepted this revision.Nov 11 2018, 12:12 PM
This revision is now accepted and ready to land.Nov 11 2018, 12:12 PM
osa1 added a comment.Nov 11 2018, 9:50 PM

Sorry everyone, I was on vacation last week and didn't have access to a computer for 8 days! I now rebased the branch and will push shortly after a validate.

This revision was automatically updated to reflect the committed changes.