Rename type variables fairly
ClosedPublic

Authored by nomeata on Jul 13 2016, 9:21 AM.

Details

Summary

So that

:t (id,id,id)

produces
(id,id,id) :: (a3 -> a3, a2 -> a2, a1 -> a1)
instead of
(id,id,id) :: (a2 -> a2, a1 -> a1, a -> a)

The branch (wip/T12382) has two patches, one that recators tidyType to look at
all forall'ed variables at once, and one to improve the actual renaming.

perf.haskell.org show now performance regressions. One test case exhibits
higher peak usage due to the first(!) patch, but peak usage is anyways not very
reliable, is it?

Test Plan

Lots of examples already in the test suite.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nomeata updated this revision to Diff 8196.Jul 13 2016, 9:21 AM
nomeata retitled this revision from to Rename type variables fairly.
nomeata edited the test plan for this revision. (Show Details)
nomeata updated the Trac tickets for this revision.
nomeata updated this object.

perf.haskell.org show now performance regressions.

I think you mean "no" instead of "now"? I don't see any clear regressions in these commits:
https://perf.haskell.org/ghc/#revision/18ac80ff729eb19ec370ead9f9275b3bc32c1f81
https://perf.haskell.org/ghc/#revision/ef48eedb9b366afd3a40e42e5a9e9112b881d5b0.

One test case exhibits higher peak usage

For those wondering: it is test T4029.

In D2402#69634, @thomie wrote:

perf.haskell.org show now performance regressions.

I think you mean "no" instead of "now"?

Indeed, now that you say it, I shouldn’t have written no, not now. :-)

One test case exhibits higher peak usage

For those wondering: it is test T4029.

bgamari accepted this revision.Jul 14 2016, 2:54 AM
bgamari edited edge metadata.

Assuming that the regression is only in peak residency and not in overall allocations this looks fine to me.

This revision is now accepted and ready to land.Jul 14 2016, 2:54 AM

Assuming that the regression is only in peak residency and not in overall allocations this looks fine to me.

That is the case, yes.

I’ll push the branch.

This revision was automatically updated to reflect the committed changes.