Close over kinds exactly once per var (#14880)
AbandonedPublic

Authored by tdammers on Sep 13 2018, 3:44 AM.

Details

Reviewers
simonpj
goldfire
bgamari
Trac Issues
#14880
Summary

As discussed in Trac:14880, comment:123, we have the issue that we want to
avoid processing the same var more than once. The original plan was to move
closing over kinds to the very end of the tyCoVarsOfType function, however,
this turns out to be inefficient and unnecessary.

Instead, we simply change the code in ty_co_vars_of_type such that
closing over kinds doesn't happen if we've already seen the var in question.

Test Plan

./validate, nofib

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
wip/T14880-2-step2-c123
Lint
Lint WarningsExcuse: not my code
SeverityLocationCodeMessage
Warningcompiler/types/TyCoRep.hs:1615TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1618TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1655TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1656TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1657TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1658TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1659TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1664TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1682TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1686TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1715TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1772TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1775TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1776TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1778TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1806TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1809TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1810TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1853TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1856TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1857TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1858TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1859TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1860TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1865TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 23042
Build 54468: [GHC] Linux/amd64: Continuous Integration
Build 54467: [GHC] OSX/amd64: Continuous Integration
Build 54466: [GHC] Windows/amd64: Continuous Integration
Build 54465: arc lint + arc unit
tdammers created this revision.Sep 13 2018, 3:44 AM

Fine, but see above

We'll need a big Note about all this

compiler/types/TyCoRep.hs
1950

Breaks the FV abstraction, but never mind.

But given that you are breaking the abstraction, it'd be better to do this:

tyCoFVsOfType (TyVarTy v)        f bound_vars (acc_list, acc_set)
   | not (f v)                               = (acc_list, acc_set)    -- NB!
   | v `elemVarSet` bound_vars = (acc_list, acc_set)
   | v `elemVarSet` acc_set = (acc_list, acc_set)
   | otherwise = tyCoFVsOfType (tyVarKind v)) f emptyVarSet (v:acc_list, extendVarSet acc_set v)
tdammers added inline comments.Sep 13 2018, 6:37 AM
compiler/types/TyCoRep.hs
1950

It does break the abstraction, but I'm not sure how you'd write this without doing so.

The Interesting Function check obviously needs to be added.

tdammers updated this revision to Diff 17981.Sep 13 2018, 6:40 AM

Use Interesting Function

Use Interesting Function

But you did not take advantage of my suggestion

| otherwise = tyCoFVsOfType (tyVarKind v)) f emptyVarSet (v:acc_list, extendVarSet acc_set v)

unitFV will just duplicate three tests you have already made!

Use Interesting Function

But you did not take advantage of my suggestion

| otherwise = tyCoFVsOfType (tyVarKind v)) f emptyVarSet (v:acc_list, extendVarSet acc_set v)

unitFV will just duplicate three tests you have already made!

Oooooh... right, of course... I missed that part completely.

tdammers updated this revision to Diff 17982.Sep 13 2018, 9:01 AM

Use extendVarSet, not unionFV

We'll need a big Note about all this

Yes, I'll see what I can come up with.

tdammers updated this revision to Diff 17985.Sep 13 2018, 3:09 PM
  • Add Note [Closing over free variable kinds]
tdammers updated this revision to Diff 17986.Sep 13 2018, 3:12 PM
  • Add Note [Closing over free variable kinds]

Assembled a Note from various snippets from the Trac issue and surrounding discussion. Feel free to edit rigorously.

Assembled a Note from various snippets from the Trac issue and surrounding discussion. Feel free to edit rigorously.

I'll do that -- but what you have is a good start, and I think I'll wait until it's committed so that the Note is describing the final version.

tdammers updated this revision to Diff 18295.Oct 12 2018, 4:32 AM
  • Fix some test wibbles
bgamari abandoned this revision.Jan 20 2019, 6:55 PM

I believe all of the pieces of Trac #14880, which this was one, have been merged. See the ticket for details.

@goldfire, do correct me if I'm wrong.