StaticPointers: Allow closed vars in the static form.
ClosedPublic

Authored by facundominguez on Apr 11 2016, 6:46 AM.

Details

Summary

With this patch closed variables are allowed regardless of whether
they are bound at the top level or not.

The FloatOut pass is always performed. When optimizations are
disabled, only expressions that go to the top level are floated.
Thus, the applications of the StaticPtr data constructor are always
floated.

The CoreTidy pass makes sure the floated applications appear in the
symbol table of object files. It also collects the floated bindings
and inserts them in the static pointer table.

The renamer does not check anymore if free variables appearing in the
static form are top-level. Instead, the typechecker looks at the
tct_closed flag to decide if the free variables are closed.

The linter checks that applications of StaticPtr only occur at the
top of top-level bindings after the FloatOut pass.

The field spInfoName of StaticPtrInfo has been removed. It used to
contain the name of the top-level binding that contains the StaticPtr
application. However, this information is no longer available when the
StaticPtr is constructed, as the binding name is determined now by the
FloatOut pass.

Test Plan

./validate

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.
facundominguez retitled this revision from to StaticPointers: Allow closed vars in the static form..
facundominguez updated this object.
facundominguez edited the test plan for this revision. (Show Details)
facundominguez added reviewers: simonpj, austin.
facundominguez updated the Trac tickets for this revision.
facundominguez added a subscriber: mboes.
facundominguez added inline comments.Apr 11 2016, 6:53 AM
compiler/typecheck/TcExpr.hs
2499

I have a patch that improves the error message at the expense of augmenting the local environment with the free variables of each binding in scope. The error message then can explain why the variable is not closed. I'll be submitting it if/when this patch is accepted.

bgamari added inline comments.Apr 13 2016, 11:46 AM
docs/users_guide/8.0.1-notes.rst
219 ↗(On Diff #7253)

Sorry, @facundominguez, I think this may need to wait until 8.0.2 at the earliest.

docs/users_guide/8.0.1-notes.rst
219 ↗(On Diff #7253)

So be it. Should I insert this elsewhere instead?

bgamari added inline comments.Apr 18 2016, 3:01 AM
docs/users_guide/8.0.1-notes.rst
219 ↗(On Diff #7253)

Yes, but the new release notes file isn't yet present in the repo.

facundominguez edited edge metadata.

Move StaticPointers bullet point to 8.0.2-notes.rst.

facundominguez marked 3 inline comments as done.Apr 18 2016, 1:14 PM
simonpj accepted this revision.Apr 26 2016, 4:13 PM
simonpj edited edge metadata.

Great stuff Facundo. Sorry it's taken me a while.

I have a few suggestions above.

I would also like an overview Note [Grand plan for static forms] somewhere, which lays out the plan

  • Keep free vars
  • Check for closed-ness via typechecker's knowledge
  • Desugar to use of magic constructor (defined in module X of base)
  • Float to top level
  • TidyPgm collects those top-level SPT entries into the SPT

etc.

compiler/coreSyn/CoreLint.hs
417

I don't like this approach.. it seems wrong to have an environment field (le_static_ptr_found) for something so local. Yuk

Better, I think:

  • In lintSingleBinding call lintRhs (passing the top-level flag) instead of lintExpr for the rhs
  • Define new function lintRhs which (for top level only) has a first case for a saturated application of StaticPtr; but otherwise does lintCoreExpr.

I think that'll work out better.

compiler/deSugar/Desugar.hs
313–314

Delete, in due course

compiler/deSugar/DsExpr.hs
413–414

We never typeset with Latex (that was years ago) so you can nuke all this \underline stuff.

420

Aha. We can do better here. Every module now has a top-level binding for the module, including its package etc, generated by TcTypeable.mkModIdBindings. So you should be albe to simply refer to that Id here, not have strings.

I think it has a fingerprint too. Do we need a fingerprint for the <module,n> pair, or could we just carry that pair around as the unique key (simpler).

482

It's very odd that this counter is in DynFlags. It should really be in the DsGblEnv, I think.

Does it matter that we use the same index as for foreign exports?? That seems odd too. Doesn't it use up SPT entries?

compiler/hsSyn/HsBinds.hs
388

three!

compiler/main/StaticPtrTable.hs
28

I'm not sure I understand the details here, but what I think should happen is this

We have a binding

`
blah = StaticPtr <fingerprint> function-closure

Basically we just want to insert blah into the SPT. I think that's what you are doing. But it all seems complicated by the Fingerprint type, which gives an extra level of nesting. In ghc-prim:GHC.Types you'll see that we flatten out the fingerprint to avoid this. Maybe you can do the same here.

Or perhaps StaticPtr will eventually look like

data StaticPr where
  SP :: Module -> Int -> a -> TypeRep a -> StaticPtr

where Module is the one in GHC.Types. And now I grant you that the key for the SPT is the fingerpritn from the Module plus the Int.

compiler/main/TidyPgm.hs
649–659

This is a bit horrible. Can't we just make them an exported Id (as in mkExportedLocalId, like dfun ids) right off the bat when we create them?

compiler/simplCore/SimplCore.hs
207

Point to an overview Note or static forms

compiler/typecheck/TcExpr.hs
596

No harm in just leaving fvs, is there?

This revision is now accepted and ready to land.Apr 26 2016, 4:13 PM
facundominguez marked 5 inline comments as done.Apr 28 2016, 5:01 PM

Still working on addressing comments.

compiler/main/TidyPgm.hs
649–659

The FloatOut pass creates these. Should we modify FloatOut to create exported Ids when they have a StaticPtr application at the top?

simonpj added inline comments.Apr 28 2016, 5:44 PM
compiler/main/TidyPgm.hs
649–659

Oh, good point.

OK, well then, why do they need to be external? They are only used internally to initialise the SPT.

If they must be external then I suppose this'll do; but add a note to explain why

facundominguez marked an inline comment as done.Apr 29 2016, 1:14 PM
facundominguez added inline comments.
compiler/deSugar/DsExpr.hs
420

Will do the move to using $trModule. I would submit it in a different patch so this update is not mixed with the support for local bindings.

GHC.Types.Module does not include a fingerprint, so I'll leave the fingerprint computation as is.

482

As long as the counter is module-scoped, the implementation should produce the same fingerprints if the module is recompiled. I'm using the only such counter I found when I implemented static pointers.

Could certainly have a dedicated counter for static pointers, but is more of a stylistic concern, unless there were issues like running out of Ints because of sharing it with foreign exports.

compiler/main/StaticPtrTable.hs
28

I'm gonna flatten the fingerprint.

compiler/main/TidyPgm.hs
649–659

The C stub which inserts the pointer in the SPT declares:

extern StgPtr <symbol_name>;

The C linker cannot find the symbol if it is not exported when linking the stub.

I'm adding a comment about it.

facundominguez marked 2 inline comments as done.
facundominguez edited edge metadata.

Addressed simonpj's comments.

Ready to land, I think. Will wait a while in case anyone has something else to add.

compiler/coreSyn/CoreLint.hs
391

This optimization arose from the need to compile GHC.StaticPtr without checking applications of StaticPtr within expressions. Otherwise the linter would complain that the binding

StaticPtr = \a b1 b2 b3 b4 -> StaticPtr a b1 b2 b3 b4

contains an application of StaticPtr nested within an expression.

This revision was automatically updated to reflect the committed changes.

A couple of follow-ups. I hope you can think about them even though this Phab is closed

compiler/coreSyn/CoreLint.hs
391

Facundo, this comment is very useful, but it's in Phab, not the code. Can you add it to Note [Checking StaticPtrs]?

compiler/deSugar/DsExpr.hs
420

Maybe it should include a fingerprint, if it's important in our application.

In which case you could make that change in the same follow-up patch

facundominguez added inline comments.May 5 2016, 8:18 AM
compiler/coreSyn/CoreLint.hs
391

Will do.

compiler/deSugar/DsExpr.hs
420

I think I understand what you want, but I'm failing to see the need or the benefit of the change.

If the key becomes a module fingerprint and an integer:

  1. we don't need to compute a fingerprint per static pointer
  2. we have a larger key to communicate between nodes
  3. we need to hash a pair to insert and lookup in the SPT
simonpj added inline comments.May 5 2016, 8:27 AM
compiler/deSugar/DsExpr.hs
420

Hmm. Yes I see. OK, let's leave it.