Fix #12076 by inlining trivial expressions in CorePrep.
ClosedPublic

Authored by ezyang on Jun 6 2016, 12:29 PM.

Details

Summary

This mostly follows the plan detailed by the discussion
Simon and I had, with one difference: instead of grabbing
the free variables of the trivial expressions to get the
embedded Ids, we just use getIdFromTrivialExpr to extract
out the Id. Because we only ever inline trivial expressions,
and there is never a literal in the function position,
we satisfy the invariant for this function.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

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.
ezyang updated this revision to Diff 7875.Jun 6 2016, 12:29 PM
ezyang retitled this revision from to Fix #12076 by inlining trivial expressions in CorePrep..
ezyang updated this object.
ezyang edited the test plan for this revision. (Show Details)
ezyang added a reviewer: simonpj.
ezyang updated the Trac tickets for this revision.
ezyang planned changes to this revision.Jun 6 2016, 1:47 PM

Unfortunately, this patch doesn't validate. Investigating.

simonpj edited edge metadata.Jun 6 2016, 3:23 PM

OK by me modulo comments

compiler/coreSyn/CorePrep.hs
1269

I don't think it's all that helpful to say what used to happen.

Better to explain what the env is for. It's (a) to support cloning of all local Ids (see item (6) of overview,
(b) to support beta-reduction of runRW
(c) to get rid of trivial RHSs of let-bindings (lazyId magic)

compiler/coreSyn/CoreUtils.hs
809–811

Note [getIdfromTrivialExpr]

826

See Note [getIdFromTrivialExpr]

ezyang updated this revision to Diff 7879.Jun 6 2016, 6:50 PM
ezyang marked an inline comment as done.
ezyang edited edge metadata.

Address simonpj comments and fix probablem with data constructors

ezyang planned changes to this revision.Jun 6 2016, 11:41 PM

Aie! It still doesn't work; function arguments still need to be saturated.

ezyang updated this revision to Diff 7881.Jun 6 2016, 11:58 PM
ezyang edited edge metadata.

Try again: this time handle literal case properly

ezyang updated this revision to Diff 7882.Jun 7 2016, 12:00 AM
ezyang edited edge metadata.

Add missing test file

ezyang updated this revision to Diff 7883.Jun 7 2016, 12:03 AM
ezyang edited edge metadata.

Clarify some documentation

ezyang updated this revision to Diff 7884.Jun 7 2016, 12:06 AM
ezyang edited edge metadata.

Simplify T12076sat test

Seems to pass tests now.

simonpj accepted this revision.Jun 8 2016, 4:46 PM
simonpj edited edge metadata.

Good. But just remind me: why did we not expand exprIsTrivial thus:

exprIsTrivial (App lazyId e) = exprIsTrivial e

Then any let x = lazy y in ... would be inlined with all the x's replaced by lazy y.

I'm sure we discussed this, but it seems so obvious that I can't see why we didn't adopt it?

Worth adding to the Notes somewhere that we have to do this lazy nonsense in CorePrep because the lazy stuff must appear in unfoldings, and must prevent call-by-value (which CorePrep implements) for catch# (see lazyId magic in MkId).

This revision is now accepted and ready to land.Jun 8 2016, 4:46 PM
ezyang added a comment.Jun 8 2016, 5:02 PM

It was the very first thing we talked about. But you thought it was annoying, because exprIsTrivial was very simple and it was not nice to have to add some extra cases to it—so then you proposed extending CorePrepEnv.

This revision was automatically updated to reflect the committed changes.