Performance improvements based on Trac #11735 and #14683.
AbandonedPublic

Authored by tdammers on Feb 6 2018, 3:31 AM.

Details

Reviewers
goldfire
bgamari
simonpj
Trac Issues
#11735
Summary

This includes:

  • Refactoring coercionKind / coercionKindRole
  • Caching role in NthCo constructor and mkNthCo
  • Discard reflexive casts during Simplify
  • Additional SCC's to hunt down performance bottlenecks in Coercion, CoreTidy, and Simplify

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
wip/tdammers/T11735
Lint
Lint WarningsExcuse: Not my code, maybe someone else wants to check these?
SeverityLocationCodeMessage
Warningcompiler/coreSyn/CoreLint.hs:1756TXT3Line Too Long
Warningcompiler/coreSyn/CoreOpt.hs:937TXT3Line Too Long
Warningcompiler/coreSyn/CoreOpt.hs:991TXT3Line Too Long
Warningcompiler/main/TidyPgm.hs:370TXT3Line Too Long
Warningcompiler/main/TidyPgm.hs:379TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:131TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:132TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:149TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:268TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:277TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:869TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:870TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:871TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:872TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:938TXT3Line Too Long
Warningcompiler/simplCore/Simplify.hs:1253TXT3Line Too Long
Warningcompiler/typecheck/TcEvidence.hs:797TXT3Line Too Long
Warningcompiler/types/Coercion.hs:233TXT3Line Too Long
Warningcompiler/types/Coercion.hs:812TXT3Line Too Long
Warningcompiler/types/Coercion.hs:1173TXT3Line Too Long
Warningcompiler/types/Coercion.hs:1778TXT3Line Too Long
Warningcompiler/types/TyCoRep.hs:1485TXT3Line Too Long
Warningdocs/core-spec/core-spec.pdf:ExtJsonLint
Unit
No Unit Test Coverage
Build Status
Buildable 19440
Build 40955: [GHC] Linux/amd64: Continuous Integration
Build 40954: [GHC] OSX/amd64: Continuous Integration
Build 40953: [GHC] Windows/amd64: Continuous Integration
Build 40952: arc lint + arc unit
tdammers created this revision.Feb 6 2018, 3:31 AM

This patch does two separate things: it optimizes simplCast and it also caches the role in an NthCo. These two changes are independent. I'm OK with both, but I'm just calling this out as two orthogonal changes.

compiler/main/TidyPgm.hs
341

Do you mean to be committing all these new SCC blocks?

compiler/simplCore/Simplify.hs
1236

This is the call to isReflexiveCo that might be slowing us down. Try moving it back toward the bottom of the cases in addCoerce and see if that improves things.

simonpj requested changes to this revision.Feb 7 2018, 3:44 AM
simonpj added a subscriber: simonpj.

This patch does two separate things: it optimizes simplCast and it also caches the role in an NthCo. These two changes are independent. I'm OK with both, but I'm just calling this out as two orthogonal changes.

I think it would be much better to separate them.

  • The NthCo change affects a lot of code in a rountine way; it's just refactoring.
  • The simplCast change affects a little code, but in a subtle way.

Can we split them, please? (And omit the debuggigng SCCs)

compiler/types/Coercion.hs
812

I think we are /not/ doing this super-role business.

812

We need an assertion, in words and as an ASSERT that the role obeys the invariant

compiler/types/TyCoRep.hs
827

I desperately want an invariant:

-- Invariant:  (NthCo r i co), it is always the case that r = role of (Nth i co)
-- That is: the role of the entire coercion is redundantly cached here.
-- See Note [NthCo Cached Roles]
834

Richard, I thought we agreed /not/ to do this implicit-SubCo stuff?

Tobias might be unsure of what is necessary to remove it, though.

1156

But, after this change, looking at coercionKind and coercionRole we will NOT see that we have to know the kind! That comment refers to old code.

How about this

Note [NthCo Cached Roles]
Why do we cache the role of NthCo in the NthCo constructor?  
Because computing role(Nth i co) involves figuring out that
        co :: T tys1 ~ T tys2
using coercionKind, and finding (coercionRole co), and then looking 
at the tyConRoles of T.  Avoiding bad asymptotic behaviour here means
we have to compute the kind and role of a coercion simultaneously,
which makes the code complicated and inefficient.

This only happens for NthCo.  Caching the role solves the problem, and
allows coercionKind and coercionRole to be simple.
See Trac #11735
This revision now requires changes to proceed.Feb 7 2018, 3:44 AM
simonpj updated the Trac tickets for this revision.Feb 7 2018, 3:47 AM

This patch does two separate things: it optimizes simplCast and it also caches the role in an NthCo. These two changes are independent. I'm OK with both, but I'm just calling this out as two orthogonal changes.

I think it would be much better to separate them.

  • The NthCo change affects a lot of code in a rountine way; it's just refactoring.
  • The simplCast change affects a little code, but in a subtle way.

    Can we split them, please? (And omit the debuggigng SCCs)

Yes, will do. I was under the impression that the SCC's were desirable to keep around, but if that's not the case I'll happily leave them out.

Created new revisions, D4394 (coercionKind refactoring and role caching) and D4395 (Simplify). Suggest we continue discussions there.

compiler/types/TyCoRep.hs
834

Indeed, I am unsure how I'd go about that. Is there any recommended reading on the topic?

1156

Agree, this makes more sense. Will fix in D4394.

bgamari abandoned this revision.Feb 19 2018, 10:38 PM

Alright, it looks like this should be closed in favor of D4394.

But do let me know if I've misunderstood; the last comment on D4394 is a bit ambiguous.