Fix #14880.
AbandonedPublic

Authored by goldfire on Jun 1 2018, 9:18 PM.

Details

Reviewers
bgamari
simonpj
Trac Issues
#14880
Summary

This fix is described in Note [Removing variables with bound kinds]
in TcType. This commit also changes split_dvs to close over kinds
at the end, which seems more performant than walking over the kind
of every tyvar occurrence.

Along the way, I also updated the free-variable functions in
TyCoRep to close over kinds after gathering free variables,
as the old story was wrong, as detailed in Note [Free variables
of types]. This caused performance trouble, and so I noticed
that most uses of free-variable finding actually do not care
at all about determinacy... yet were carefully tracking order.
So I wrote tyCoVarsOfType to build a VarSet directly, without
using FV.

Sadly, there is *still* performance trouble, yet to be sorted
out.

Also while working on this, I noticed that GHC wasn't doing its
best job at keeping left-to-right ordering of type variables in
cases where it doesn't really matter, but still would be nice.
I've made some improvements in this area.

An unrelated change is also in MkId, where the type-variable
reordering logic for data constructors was wrong for newtype
instances in GADT syntax. Fixed now.

Test Plan

validate

goldfire created this revision.Jun 1 2018, 9:18 PM
simonpj added inline comments.Jun 4 2018, 3:08 AM
compiler/typecheck/TcType.hs
1161

Explain the significance of the 'bound' parameter, referring to Note [Removing variables...

compiler/typecheck/TcValidity.hs
1834

See comment:10 in Trac Trac #15142 for how we can simplify this

goldfire marked an inline comment as done.Jun 15 2018, 11:15 AM
goldfire added inline comments.
compiler/typecheck/TcValidity.hs
1834

That's part of a larger discussion, and not directly related to this ticket.

goldfire updated this revision to Diff 17280.Jul 12 2018, 7:40 AM

This fixes an egregious error I made in the last patch.

I'd like to check out this branch for myself and observe the performance failures, but this doesn't appear to be based on a commit in master, so I'm unable to do so. Can someone rebase this on master?

Also, it would be nice to apply the Haddock changes from https://ghc.haskell.org/trac/ghc/ticket/14880#comment:31 so that we can confirm the performance failures on Harbormaster.

compiler/basicTypes/MkId.hs
576
compiler/types/Type.hs
2187

This change looks somewhat suspicious to me, as I explain in https://ghc.haskell.org/trac/ghc/ticket/14880#comment:46.

simonpj added inline comments.Jul 30 2018, 9:27 AM
compiler/typecheck/TcType.hs
1182

But split_dvs True is essentially tyCoVarsOfType isn't it? Why use a more elaboate function?

compiler/types/TyCoRep.hs
1526

Maybe a little better

mapUnionVarSet close_one tcvs
where
   close_one tcv = insert tcv (tyCoVarsOfType (tyVarKind tcv))
bgamari abandoned this revision.Jan 20 2019, 7:02 PM

I believe all of the pieces of Trac #14880 have been merged.