[commit: ghc] master: Bottoming expressions should not be expandable (407c11b)
git at git.haskell.org
git at git.haskell.org
Fri Aug 25 11:57:29 UTC 2017
Repository : ssh://git@git.haskell.org/ghc
On branch : master
Link : http://ghc.haskell.org/trac/ghc/changeset/407c11b880325f4f327982d4f6b9f9cba4564016/ghc
>---------------------------------------------------------------
commit 407c11b880325f4f327982d4f6b9f9cba4564016
Author: Simon Peyton Jones <simonpj at microsoft.com>
Date: Fri Aug 25 09:00:31 2017 +0100
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.
>---------------------------------------------------------------
407c11b880325f4f327982d4f6b9f9cba4564016
compiler/coreSyn/CoreUtils.hs | 74 ++++++++++++++++++++++++++---------------
compiler/simplCore/OccurAnal.hs | 14 +++++---
2 files changed, 56 insertions(+), 32 deletions(-)
diff --git a/compiler/coreSyn/CoreUtils.hs b/compiler/coreSyn/CoreUtils.hs
index e9dc8a9..1b92a7f 100644
--- a/compiler/coreSyn/CoreUtils.hs
+++ b/compiler/coreSyn/CoreUtils.hs
@@ -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
diff --git a/compiler/simplCore/OccurAnal.hs b/compiler/simplCore/OccurAnal.hs
index 1620c91..113f8bd 100644
--- a/compiler/simplCore/OccurAnal.hs
+++ b/compiler/simplCore/OccurAnal.hs
@@ -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
More information about the ghc-commits
mailing list