Opened 4 years ago

Closed 3 years ago

#11656 closed bug (fixed)

Alllow static pointers to local closed definitions

Reported by: mboes Owned by:
Priority: normal Milestone: 8.2.1
Component: Compiler Version: 7.10.3
Keywords: Cc: facundo.dominguez, edsko, alpmestan
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: #11698 Blocking:
Related Tickets: Differential Rev(s): Phab:D2104, Phab:D2655
Wiki Page:

Description

A limitation of the original patch implementing -XStaticPointers is that while the following all work,

t1 = static not
t2 = static (\x -> case x of True -> False; False -> True)

the below does not:

t3 = static foo where foo True = False

Yet foo is perfectly "static": it's a closed expression that can readily be floated to top-level.

So in short: -XStaticPointers should not consider only expressions with top-level identifiers, but in general any closed expression (which may involve local closed identifiers too).

Change History (21)

comment:1 Changed 4 years ago by mboes

Note that the main reason that this was not implemented the first time around is that we couldn't get floating of closed local identifiers to top-level to work properly. This is likely an easy and straightforward task though - Facundo and I just don't have the requisite knowledge it seems. So any help from core developers more familiar with GHC would be greatly appreciated.

comment:2 Changed 4 years ago by simonpj

I'm happy to help, but too snowed under to take the lead.

Simon

comment:3 Changed 4 years ago by facundo.dominguez

This is the mechanism we were considering so far:

  1. Have the renamer compute whether bindings in scope are closed.
  2. When the renamer finds a static form, allow the free vars to be top-level or closed local bindings.
  3. Desugar the static form. This produces a list of floated Core bindings.
  4. For each such Core binding find the free variables of local definitions.
  5. For each found local definition traverse the enclosing top-level binding to remove it.
  6. Add each removed local definition at the top level.

If it simplifies anything, a variant is to skip (5) and have (6) copy the local definitions as local definitions of the Core bindings produced in (3). This might duplicate local definitions once per static form in which they are used. Does copying definitions like this require also renaming bindings?

Local definitions may transitively use other local definitions, in which case all of them should be floated.

We were wondering if we are the first hackers in need of a function in GHC to determine if bindings are closed or not. Maybe someone could point if this is computed elsewhere already. We only found a flag tct_closed which only applies to types, not expressions.

Is floating local definitions to the top-level disruptive for other compilation stages? We are open to proposals of alternative mechanisms.

comment:4 Changed 4 years ago by simonpj

I'm afraid I need more examples to understand these steps. Rather than do this on a ticket, would you like to start a wiki page to explain the problem, and the proposed solution, with examples complicated enough to demonstrate the need for each step.

One tantalising point is that we already have a FloatOut pass for Core that will do much of this. Once I understand the problem better we can think about whether it could be useful.

Thanks!

PS: v busy until the ICFP deadline (2 wks)

comment:5 Changed 4 years ago by facundo.dominguez

I just introduced a new section in StaticPointers#Localbindingsinthestaticform. Let me know if it is still not clear.

comment:6 Changed 4 years ago by alpmestan

Cc: alpmestan added

comment:7 Changed 4 years ago by mboes

Blocked By: 11698 added

comment:8 Changed 3 years ago by facundo.dominguez

Owner: set to facundo.dominguez

comment:9 Changed 3 years ago by facundo.dominguez

There is a working implementation in https://github.com/ghc/ghc/compare/master...tweag:fd/float-static

I hope to submit a phabricator revision next week. Currently have to update the documentation, improve error messages and update linter.

I would be thankful for any suggestion about which part of the linter to modify in order to produce errors when applications of the StaticPtr constructor appear nested within other expressions.

comment:10 in reply to:  9 Changed 3 years ago by simonpj

I would be thankful for any suggestion about which part of the linter to modify in order to produce errors when applications of the StaticPtr constructor appear nested within other expressions.

It's a bit awkward with the current structure of CoreLint. I best might be

  • Make lintCoreExpr complain if if it finds a StaticPtr constructor
  • Make the non-recursive case of lint_bind (in lintCorBindings) call a new function lintTopNonRec or something.
  • lintTopNonRec can have a case for (StaticPtr e1 e2) on its RHS, and allow it.

comment:11 Changed 3 years ago by facundo.dominguez

Differential Rev(s): Phab:D2104
Status: newpatch

comment:12 Changed 3 years ago by Facundo Domínguez <facundo.dominguez@…>

In 36d29f7c/ghc:

StaticPointers: Allow closed vars in the static form.

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

Reviewers: goldfire, simonpj, austin, hvr, bgamari

Reviewed By: simonpj

Subscribers: thomie, mpickering, mboes

Differential Revision: https://phabricator.haskell.org/D2104

GHC Trac Issues: #11656

comment:13 Changed 3 years ago by facundo.dominguez

Resolution: fixed
Status: patchclosed

comment:14 Changed 3 years ago by bgamari

Facundo, I noticed that there is an entry for this feature in the 8.0.2 release notes on master but the patch is not present on the ghc-8.0 branch. Did you expect this to go into 8.0.2?

While the patch applies fairly easily its size, it still makes me a bit nervous even if it mostly touches static pointers code.

Last edited 3 years ago by bgamari (previous) (diff)

comment:15 Changed 3 years ago by facundo.dominguez

I think I added them in 8.0.2 because it was the next release in sight. mboes?

comment:16 Changed 3 years ago by facundo.dominguez

This should affect the StaticPointers mostly. I don't think it is of high risk, but it is not risk-free either and we don't have an urgency to include it now.

comment:17 Changed 3 years ago by bgamari

Milestone: 8.2.1

Let's punt on it in that case.

comment:18 Changed 3 years ago by facundo.dominguez

Differential Rev(s): Phab:D2104Phab:D2104, Phab:D2655
Owner: facundo.dominguez deleted
Resolution: fixed
Status: closednew

comment:19 Changed 3 years ago by facundo.dominguez

Status: newpatch

comment:20 Changed 3 years ago by Facundo Domínguez <facundo.dominguez@…>

In 0b70ec0/ghc:

Have static pointers work with -fno-full-laziness.

Summary:
Before this patch, static pointers wouldn't be floated to
the top-level.

Test Plan: ./validate

Reviewers: simonpj, bgamari, austin

Subscribers: mboes, thomie

Differential Revision: https://phabricator.haskell.org/D2662

GHC Trac Issues: #11656

comment:21 Changed 3 years ago by bgamari

Resolution: fixed
Status: patchclosed
Note: See TracTickets for help on using tickets.