Do not drop dead code in the desugarer
Needs RevisionPublic

Authored by nomeata on Feb 3 2017, 5:30 PM.

Details

Summary

so that GHC plugins have a chance of doing something with them first.
See Trac #11179 and Trac #10823.

Test Plan

manually tested it here, bindings stay alive

nomeata retitled this revision from to Do not drop dead code in the desugarer.Feb 3 2017, 5:30 PM
nomeata updated this object.
nomeata edited the test plan for this revision. (Show Details)
nomeata added a reviewer: gridaphobe.
nomeata updated the Trac tickets for this revision.
simonpj requested changes to this revision.Feb 3 2017, 5:50 PM
simonpj added a reviewer: simonpj.
simonpj added a subscriber: simonpj.

No, let's NOT put it in the environment. This business of not dropping dead binding applies only at top level, right? If so, it's much clearer just to pass the flag down separately (just one or two levels of functions), as comment:9 suggetss.

I recall that desguaring (and the type checker) generate lots of dead bindings, so dropping them fast is good. I'm relucant to stop all dead-code elimination unless there is a powerful reason to want that.

It would also be good to know how much deadl code is thereby retained typically. This will impose a small cost on every compilation. Ideas

  • drop dead *evidence* bindings regardless. I bet the plugins don't want them.
  • don't do this unless there is at least one plugin; or have a flag.
This revision now requires changes to proceed.Feb 3 2017, 5:50 PM
nomeata updated this revision to Diff 10925.Feb 3 2017, 9:11 PM

New approach to not dropping dead code in the desugarer

This is a much cleaner approach to this. When we want to preserve all top-level
bindings, simply add their binders to the “body” of the module. This way, the
occurence analyser sees a use of all of them and preserves them.

I have a better approach now. Do you like it?

It only affects top-level bindings now; local dead code is removed as before. We could also easily filter the list concatMap bindersOf binds, e.g. for evidence variables. Do you have an example for code that would produce top-level evidence variables?

mpickering requested changes to this revision.Feb 4 2017, 4:09 AM
mpickering added a reviewer: mpickering.
mpickering added a subscriber: mpickering.

This solution feels very indirect. It is obtuse to keep dead bindings by making them live bindings in an unrelated part of the code upstream and potentially very confusing.

I think it is also undesirable to increase everyone's code size for the tiny fraction of users who use plugins AND want to access dead code.

What is wrong with adding a flag -fkeep-dead-code which is inspected when the decision to throw away dead code is made?

This revision now requires changes to proceed.Feb 4 2017, 4:09 AM

Hi,

I disagree with that review.

This solution feels very indirect. It is obtuse to keep dead bindings by making them live bindings in an unrelated part of the code upstream and potentially very confusing.

This follows the existing pattern of how variables in rules or other pragmas are being kept alive. It is also a pattern in other parts of the compiler (CallArity). It also makes sense morally: The “body” of a module is all modules importing it, so we can conservatively approximate their uses this way. If exported Id were not readily marked as such, this is precisely how I would implement keeping them alive!

I don’t see how this is an unrelated part of the code; this all happens in the occurrence analyser.

I think it is also undesirable to increase everyone's code size for the tiny fraction of users who use plugins AND want to access dead code.

I’ll send it to perf.haskell.org for measurement. Also, it can be refined to exclude evidence variable bindings, I just did not yet find out how I can recognize these.

What is wrong with adding a flag -fkeep-dead-code which is inspected when the decision to throw away dead code is made?

Becuase of bad user experience:

  • User wants to use a plugin
  • User adds the plugin to the GHC_OPTIONS, and puts in the plugin-specific bindings
  • Nothing happens.

Or maybe

  • User wants to use a plugin
  • User adds the plugin to the GHC_OPTIONS, and puts in the plugin-specific bindings
  • Plugin complains “Please also add -fkeep-dead-bindings to your list of flags”
  • User grumbles and does it.

There are of course alternatives here. A DynFlags -> IO DynFlags field in plugins to react on and possibly modify the DynFlags maybe. We’d have to add DynFlags to the occurrence analyser. Or a plugin pass before the desugarer, and a way to mark bindings as “not exported but to be kept alive”.

If I understand correctly, there should be no increase in code size. We are only keeping the binders alive inside the desugarer, they will eventually be dropped in the Core2Core pipeline.

That being said, I like the idea to make this all conditional on a flag, assuming plugins gain the ability to enable it (that seems like a useful thing anyway).

I know that @angerman has thought a bit about how the plugin interface should look. Perhaps he has something to add here.

From a very brief look at this, and assuming for a moment the plugin architecture allowed for hooks prior to the desugarer, would this change still be necessary? Thus
under the assumption we have more hooks for plugins to interact with ghc at various points during the compilation, are there any obstructions that would prevent
this to be handled by that hypothetical plugin?

I've experimented with extending hooks for the actual compilation pipeline in https://phabricator.haskell.org/D535, as a first rough cut.

@angerman perhaps not, but it's much more pleasant to work with Core than with HsSyn. I think we should avoid pushing plugin authors (and API clients) towards dealing with the giant types of HsSyn and friends. It's better to keep that complexity internal to GHC.

perf.haskell.org reports the absence of significant regressions: https://perf.haskell.org/ghc/#revision/b59c2de7abe3cd4e046f11c3536ba8e7137c4f84

From a very brief look at this, and assuming for a moment the plugin architecture allowed for hooks prior to the desugarer, would this change still be necessary

With that assumption, plugin authors might be able to do something smarter. But this is a relatively easy way of improving the situation now, and it is scratching an itch that multiple plugin authors had.

perf.haskell.org reports the absence of significant regressions: https://perf.haskell.org/ghc/#revision/b59c2de7abe3cd4e046f11c3536ba8e7137c4f84

@nomeata looks like there's a perf regression though in the testsuite (T4029).

@nomeata looks like there's a perf regression though in the testsuite (T4029).

Right. Might just be something that is now pushed over the edge, though. I find the output difference in T4900 more surprising: https://phabricator.haskell.org/harbormaster/build/20118/?l=0

I'm happy with the approach of starting with a live-var set. Much neater than messing with the guts of the analyser

mpickering resigned from this revision.Mar 1 2017, 12:48 PM

What is the plan for this, @nomeata?

austin resigned from this revision.Nov 9 2017, 11:34 AM