# Fix #12076 by inlining trivial expressions in CorePrep.ClosedPublicActions

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

# Details

Summary

This mostly follows the plan detailed by the discussion
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
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 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

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.

Herald edited edge metadata. Jun 6 2016, 6:50 PM
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

Try again: this time handle literal case properly

Herald edited edge metadata. Jun 6 2016, 11:58 PM
ezyang updated this revision to Diff 7882.Jun 7 2016, 12:00 AM

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

Clarify some documentation

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

Simplify T12076sat test

Herald edited edge metadata. Jun 7 2016, 12:06 AM

Seems to pass tests now.

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

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.