Hoopl: remove dependency on Hoopl package
ClosedPublic

Authored by michalt on May 28 2017, 12:36 PM.

Details

Summary

This copies the subset of Hoopl's functionality needed by GHC to
cmm/Hoopl and removes the dependency on the Hoopl package.

The main motivation for this change is the confusing/noisy interface
between GHC and Hoopl:

  • Hoopl has Label which is GHC's BlockId but different than GHC's CLabel
  • Hoopl has Unique which is different than GHC's Unique
  • Hoopl has Unique{Map,Set} which are different than GHC's Uniq{FM,Set}
  • GHC has its own specialized copy of Dataflow, so cmm/Hoopl is needed just to filter the exposed functions (filter out some of the Hoopl's and add the GHC ones)

With this change, we'll be able to simplify this significantly.
It'll also be much easier to do invasive changes (Hoopl is a public
package on Hackage with users that depend on the current behavior)

This should introduce no changes in functionality - it merely
copies the relevant code.

Signed-off-by: Michal Terepeta <michal.terepeta@gmail.com>

Test Plan

./validate

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.
michalt created this revision.May 28 2017, 12:36 PM
bgamari edited edge metadata.May 29 2017, 11:22 PM

Looks reasonable to me if there are no objections from anyone else. That being said, I do wonder whether we want to retain the Hoopl. namespace in the long-term. Regardless, there's no reason why such renamings can't happen in a later diff.

simonmar edited edge metadata.May 30 2017, 3:42 AM

I won't object, other than to note that this is typically the opposite direction that we normally want to go in, that is to use third-party libraries via their normal APIs rather than to fork them. We're trying to go in the other direction with pretty, for example. Having said that, I know it's difficult to reconcile some of the divergence without losing performance, particularly the manual specialisation. But some of the other impedence mismatch could be worked around, by either unifying APIs or centralising a renaming layer.

There are a whole set of tradeoffs here so it's hard to make a good judgement, but I'm sure you've thought this through, so if this is the direction you want to go it's ok with me.

I won't object, other than to note that this is typically the opposite direction that we normally want to go in, that is to use third-party libraries via their normal APIs rather than to fork them. We're trying to go in the other direction with pretty, for example. Having said that, I know it's difficult to reconcile some of the divergence without losing performance, particularly the manual specialisation. But some of the other impedence mismatch could be worked around, by either unifying APIs or centralising a renaming layer.

There are a whole set of tradeoffs here so it's hard to make a good judgement, but I'm sure you've thought this through, so if this is the direction you want to go it's ok with me.

Thanks for having a look! I've actually started a thread about this on ghc-devs@ before sending out the patch:
https://mail.haskell.org/pipermail/ghc-devs/2017-May/014255.html
(where I tried to explain why I think this is reasonable; Simon PJ also had some questions about this, which I tried to address)

I'm usually a strong fan of using third-party packages for things that are somewhat orthogonal to GHC - if there are good solutions for binary serialization, pretty priting or containers, etc., we should use them! But extracting core components of a compiler (e.g., an optimizer) is IMHO far more tricky. We might still want to do that at some point, but I just don't think we're ready for that with Hoopl. And iterating on performance/design/approach is going to be much easier if we have the code in one place and don't need to worry about compatibility.

It sounds like there's a good amount of consensus that this is a reasonable direction for now.

However, I think we should be clear about our long-term intentions here: do we want to retain the hoopl namespace with the intent of later re-merging with hoopl or spinning off into our own package (ghoopl)? Or rather do we want to try to dissolve what is currently Hoopl.* into GHC proper?

It sounds like there's a good amount of consensus that this is a reasonable direction for now.

However, I think we should be clear about our long-term intentions here: do we want to retain the hoopl namespace with the intent of later re-merging with hoopl or spinning off into our own package (ghoopl)? Or rather do we want to try to dissolve what is currently Hoopl.* into GHC proper?

I don't think re-merging with Hoopl should be our long-term plan. One of my main motivations for finishing this fork is to avoid having to worry about compatibilty with Hoopl. And those two things seem contradictory.

Creating our own package doesn't restrict our ability to experiment/change things, so I'd be ok with that. Although I'm still not convinced that there is a lot of value in splitting off an optimizer-as-a-library this way. There's a tension between the generality of a library (to support various languages and representations) and its performance/simplicity. Also, there's a lot of complexity in particular optimizations/analyses themselves and we wouldn't be able to provide them (since we know nothing about the input language). From this perspective, splitting off a Cmm (C--?) backend sounds like a better idea, but that would be *a lot* of work (and it's arguable whether a lot of people would be interested considering that LLVM is covering somewhat similar space)

So I'm leaning towards just merging into GHC proper - this is relatively little code (with this diff all of cmm/Hoopl is ~700 LOC of Haskell + ~200 LOC of comments) and we can probably reduce that further by simplifying some things.

But I'd be really interested in hearing other opinions if people disagree!

bgamari accepted this revision.Jun 8 2017, 1:47 PM

It sounds like there's no objectors and your proposal sounds fine to me. Thanks @michalt!

This revision is now accepted and ready to land.Jun 8 2017, 1:47 PM

It sounds like there's no objectors and your proposal sounds fine to me. Thanks @michalt!

To be fair - there's still an ongoing thread on ghc-devs about this...

It sounds like there's no objectors and your proposal sounds fine to me. Thanks @michalt!

To be fair - there's still an ongoing thread on ghc-devs about this...

Indeed I forgot about that. I'll hold off on merging until the dust has cleared.

bgamari requested changes to this revision.Jun 8 2017, 2:24 PM
This revision now requires changes to proceed.Jun 8 2017, 2:24 PM
michalt added a subscriber: kavon.Jun 15 2017, 12:45 PM
simonmar accepted this revision.Jun 16 2017, 8:26 AM
simonmar added a subscriber: simonpj.

We're good to go here. I talked to @simonpj and we agreed that this direction is fine for now, in the future we can think about redesigning hoopl and once again extracting it from GHC.

michalt requested review of this revision.Jun 19 2017, 1:37 PM
michalt edited edge metadata.

Thanks Simon!

I've had a VC and an email exchange with Kavon Farvardin and he's also ok to go ahead with this diff.
(we discussed that it might make sense to extract it at some point to a new package, but probably not upstream Hoopl to avoid disruption)

@bgamari I'm marking this as "request review" again. Let me know if there's anything else I should look into. Thanks!

bgamari accepted this revision.Jun 21 2017, 2:39 PM

Soundso good to me.

This revision is now accepted and ready to land.Jun 21 2017, 2:39 PM
This revision was automatically updated to reflect the committed changes.