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: mass refactoring (avoid diff noise)
SeverityLocationCodeMessage
Warningcompiler/types/TyCoRep.hs:1573TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1576TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1577TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1579TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1607TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1610TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1611TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1654TXT3Line 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:1660TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1661TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1666TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1684TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1688TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1717TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1785TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1790TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1791TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1793TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1794TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1795TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1796TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1797TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 22652
Build 53071: [GHC] Linux/amd64: Continuous Integration
Build 53070: [GHC] OSX/amd64: Continuous Integration
Build 53069: [GHC] Windows/amd64: Continuous Integration
Build 53068: 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
1754

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
1754

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.