PLT-871: ThunkRecursions conservative mode #5560
Conversation
bezirg
force-pushed
the
bezirg/fix-thunkrecursions
branch
3 times, most recently
from
fcba41d
to
5085623
Compare
| At the moment we only do this for bindings whose RHS is potentially impure, although in | ||
| principle it could be an improvement in other cases because it would allow using the faster | ||
| strict binding in the body. Unclear. | ||
| Using the default optimization settings, (a) the order of effects may change and (b) |
There was a problem hiding this comment.
I'd like to see some discussion of why, maybe with an example!
| let (toNonStrictify, rest) = NE.partition needsNonStrictify bs | ||
| -- MAYBE: use some prism/traversal to keep the original arrangement of the let group | ||
| -- this is not a semantic problem, but just a stylistic/debugging issue where the original | ||
| -- let-group will have reordered the (lazified) bindings |
There was a problem hiding this comment.
One way you could maybe do this is to avoid the partitioning and do more stuff unconditionally. Something like:
strictifyIfNecessary ::Binding -> Binding
strictifyIfNecessary b | needsNonStrictify b = ...
strictifyIfNecessary b = b
...
strictifiedBindings = fmap strictifyIfNecessary bs
-- or I guess you could again put the needsNonStrictify condition here
strictifiers = fmap mkStrictifierB bs
I think we can add the strictifying bindings unconditionally: if the RHS is a strict variable (which is the only case in which we currently omit them), then the inliner should remove them.
e.g.
let rec
!f = /\ a ...
!x = \y ....
in t
--->
let rec
~f = /\ a ....
!x = \y ...
in
let
!f = f
!x =x
in t
The inliner should deal with the x=x binding in this case. I actually think this sort of thing can be quite nice: you emit worse code but rely on the simplifier to clean it up. Not sure overall, though.
There was a problem hiding this comment.
I am also not sure if i want to rely on a later pass
| needsStrictifier b = | ||
| if opts^.coPreserveStrictness | ||
| then needsNonStrictify b -- all that were non-strictified | ||
| else needsNonStrictify b && isStrictEffectful b -- introduce strictifier only for those that are impure |
There was a problem hiding this comment.
mmmm, I think we probably want to do it unconditionally. Strictifying things is generally good, it means we only evaluate them once. It's not an advantage to avoid it!
There was a problem hiding this comment.
And as suggested above, I think you could in fact insert these for all the bindings, and the simplifier would clean it up.
| -- alternative if we don't care about the original strictness, we can skip the re-strictification of those pure, non-strictified | ||
| needsStrictifier :: Binding tyname name uni fun a -> Bool | ||
| needsStrictifier b = | ||
| if opts^.coPreserveStrictness |
There was a problem hiding this comment.
Generally I don't think this is something the user is going to care about. They might care about us messing up their effects, but I think we're generally pretty free to mess with strictness in a way that helps us.
| , "monoMap" | ||
| , "errorBinding" | ||
| ] | ||
| testNested "thunkRecursions" |
There was a problem hiding this comment.
I'd be tempted to add some evaluation tests - that's the gold standard way to show that the effects definitely happen in a particular order!
There was a problem hiding this comment.
I also thought of adding that, but I was hit by an unrelated trace-effect missing bug
| @@ -241,7 +249,7 @@ compileReadableToPlc (Program a v t) = | |||
| >=> NonStrict.compileNonStrictBindings False | |||
| >=> through check | |||
| >=> (<$ logVerbose " !!! thunkRecursions") | |||
| >=> (withBuiltinsInfo . fmap pure . flip ThunkRec.thunkRecursions) | |||
| >=> (\t' -> withBuiltinsInfo $ \semvar -> withCcOpts $ \opts -> pure $ ThunkRec.thunkRecursions semvar opts t') | |||
There was a problem hiding this comment.
at this point it's easier to just write an inline \t -> do ..., I think?
There was a problem hiding this comment.
this whole thing sucks a bit. I pushed the monadreader compilationctx down to each pass and then I was super excited how the code looked great. But then half of the test cases broke, so i reverted it
| needsNonStrictify b = | ||
| if opts^.coPreserveEffectOrder | ||
| then isProblematic b || isStrictEffectful b | ||
| else isProblematic b |
There was a problem hiding this comment.
I'd like to see some discussion about why it's worth having two modes! Why not just do the conservative thing always? It doesn't look like a lot of savings: the advantage of non-conservative mode is that we keep the recursive binding strict, and so we avoid some overhead from that. But it's not clear to me how much difference that makes. And it only applies in a niche case where we have something that is of function type (so doesn't get strictified anyway), but is impure.
So I'd be pretty inclined to just make the conservative mode the only mode.
bezirg
force-pushed
the
bezirg/fix-thunkrecursions
branch
2 times, most recently
from
6dc9bae
to
4d5f6c9
Compare
plutus-core/plutus-ir/src/PlutusIR/Transform/ThunkRecursions.hs
Outdated
Show resolved
Hide resolved
wait a moment I have to update 9.6 scripts output |
bezirg
force-pushed
the
bezirg/fix-thunkrecursions
branch
from
4d5f6c9
to
d048d40
Compare
The current thunk recursions transformation has 2 problems:
This PR fixes (1) and (2)
Pre-submit checklist: