Commit 407c11b8 authored by Simon Peyton Jones's avatar Simon Peyton Jones
Browse files

Bottoming expressions should not be expandable

This patch changes isExpandableApp and isWorkFreeApp to respond
False to bottoming applications.  I found that if we had

  x = undefined <dict-expr>

then prepareRhs was ANF'ing it to

  d = <dict-expr>
  x = undefined d

which is stupid (no gain); and worse it made the simplifier iterate
indefinitely.  It showed up when I started marking 'x' as a bottoming
Id more aggresssively than before; but it's been a lurking bug for
ages.

It was convenient to make isWorkFreeApp also return False for
bottoming applications, and I see no reason not to do so.

That leaves isCheapApp.  It currently replies True to bottoming
applications, but I don't see why that's good..  Something to try
later.
parent a0b7b100
......@@ -83,7 +83,7 @@ import DynFlags
import FastString
import Maybes
import ListSetOps ( minusList )
import BasicTypes ( Arity )
import BasicTypes ( Arity, isConLike )
import Platform
import Util
import Pair
......@@ -1168,22 +1168,11 @@ type CheapAppFun = Id -> Arity -> Bool
-- isCheapApp
-- isExpandableApp
-- NB: isCheapApp and isExpandableApp are called from outside
-- this module, so don't be tempted to move the notRedex
-- stuff into the call site in exprIsCheapX, and remove it
-- from the CheapAppFun implementations
notRedex :: CheapAppFun
notRedex fn n_val_args
= n_val_args == 0 -- No value args
|| n_val_args < idArity fn -- Partial application
|| isBottomingId fn -- OK to duplicate calls to bottom;
-- it certainly doesn't need to be shared!
isWorkFreeApp :: CheapAppFun
isWorkFreeApp fn n_val_args
| notRedex fn n_val_args
| n_val_args == 0 -- No value args
= True
| n_val_args < idArity fn -- Partial application
= True
| otherwise
= case idDetails fn of
......@@ -1192,11 +1181,11 @@ isWorkFreeApp fn n_val_args
isCheapApp :: CheapAppFun
isCheapApp fn n_val_args
| notRedex fn n_val_args
= True
| isWorkFreeApp fn n_val_args = True
| isBottomingId fn = True -- See Note [isCheapApp: bottoming functions]
| otherwise
= case idDetails fn of
DataConWorkId {} -> True
DataConWorkId {} -> True -- Actually handled by isWorkFreeApp
RecSelId {} -> n_val_args == 1 -- See Note [Record selection]
ClassOpId {} -> n_val_args == 1
PrimOpId op -> primOpIsCheap op
......@@ -1208,21 +1197,24 @@ isCheapApp fn n_val_args
isExpandableApp :: CheapAppFun
isExpandableApp fn n_val_args
| notRedex fn n_val_args
= True
| isConLikeId fn
= True
| isWorkFreeApp fn n_val_args = True
| otherwise
= case idDetails fn of
DataConWorkId {} -> True
DataConWorkId {} -> True -- Actually handled by isWorkFreeApp
RecSelId {} -> n_val_args == 1 -- See Note [Record selection]
ClassOpId {} -> n_val_args == 1
PrimOpId {} -> False
_ -> all_pred_args n_val_args (idType fn)
_ | isBottomingId fn -> False
-- See Note [isExpandableApp: bottoming functions]
| isConLike (idRuleMatchInfo fn) -> True
| all_args_are_preds -> True
| otherwise -> False
where
-- See if all the arguments are PredTys (implicit params or classes)
-- If so we'll regard it as expandable; see Note [Expandable overloadings]
-- See if all the arguments are PredTys (implicit params or classes)
-- If so we'll regard it as expandable; see Note [Expandable overloadings]
all_args_are_preds = all_pred_args n_val_args (idType fn)
all_pred_args n_val_args ty
| n_val_args == 0
= True
......@@ -1235,7 +1227,35 @@ isExpandableApp fn n_val_args
| otherwise
= False
{- Note [Record selection]
{- Note [isCheapApp: bottoming functions]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I'm not sure why we have a special case for bottoming
functions in isCheapApp. Maybe we don't need it.
Note [isExpandableApp: bottoming functions]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
It's important that isExpandableApp does not respond True to bottoming
functions. Recall undefined :: HasCallStack => a
Suppose isExpandableApp responded True to (undefined d), and we had:
x = undefined <dict-expr>
Then Simplify.prepareRhs would ANF the RHS:
d = <dict-expr>
x = undefined d
This is already bad: we gain nothing from having x bound to (undefined
var), unlike the case for data constructors. Worse, we get the
simplifier loop described in OccurAnal Note [Cascading inlines].
Suppose x occurs just once; OccurAnal.occAnalNonRecRhs decides x will
certainly_inline; so we end up inlining d right back into x; but in
the end x doesn't inline because it is bottom (preInlineUnconditionally);
so the process repeats.. We could elaborate the certainly_inline logic
some more, but it's better just to treat bottoming bindings as
non-expandable, because ANFing them is a bad idea in the first place.
Note [Record selection]
~~~~~~~~~~~~~~~~~~~~~~~~~~
I'm experimenting with making record selection
look cheap, so we will substitute it inside a
......
......@@ -1564,8 +1564,8 @@ occAnalNonRecRhs env bndr bndrs body
certainly_inline -- See Note [Cascading inlines]
= case idOccInfo bndr of
OneOcc { occ_in_lam = in_lam, occ_one_br = one_br }
-> not in_lam && one_br && active && not_stable
_ -> False
-> not in_lam && one_br && active && not_stable
_ -> False
dmd = idDemandInfo bndr
active = isAlwaysActive (idInlineActivation bndr)
......@@ -1654,15 +1654,19 @@ definitely inline the next time round, and so we analyse x3's rhs in
an ordinary context, not rhsCtxt. Hence the "certainly_inline" stuff.
Annoyingly, we have to approximate SimplUtils.preInlineUnconditionally.
If we say "yes" when preInlineUnconditionally says "no" the simplifier iterates
indefinitely:
If (a) the RHS is expandable (see isExpandableApp in occAnalApp), and
(b) certainly_inline says "yes" when preInlineUnconditionally says "no"
then the simplifier iterates indefinitely:
x = f y
k = Just x
k = Just x -- We decide that k is 'certainly_inline'
v = ...k... -- but preInlineUnconditionally doesn't inline it
inline ==>
k = Just (f y)
v = ...k...
float ==>
x1 = f y
k = Just x1
v = ...k...
This is worse than the slow cascade, so we only want to say "certainly_inline"
if it really is certain. Look at the note with preInlineUnconditionally
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment