StaticPointers: Allow closed vars in the static form.

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



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

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


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

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
219 ↗(On Diff #7253)

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

219 ↗(On Diff #7253)

So be it. Should I insert this elsewhere instead?

bgamari added inline comments.Apr 18 2016, 3:01 AM
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



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.


Delete, in due course


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


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


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?




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.


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?


Point to an overview Note or static forms


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.


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

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.

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.


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.


I'm gonna flatten the fingerprint.


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.


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


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


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

Will do.


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

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