From 2cda6f9f6c68f5cfd202e9979fefaa40df26769e Mon Sep 17 00:00:00 2001
From: "simonpj@microsoft.com" <unknown>
Date: Mon, 25 Oct 2010 15:26:22 +0000
Subject: [PATCH] Do not (ever) use substExprSC in the simplifier

"Short-cut" substitution means "do nothing if the substitution
is empty". We *never* want do to that in the simplifier because
even though the substitution is empty, the in-scope set has
useful information:

 * We get up-to-date unfoldings; and that in turn may
   reduce the number of iterations of the simplifier

 * We avoid space leaks, because failing to substitute may
   hang on to old Ids from a previous iteration

(This is what was causing the late inlining of foo in
Trac #4428.)
---
 compiler/coreSyn/CoreSubst.lhs  |   14 ++++++++------
 compiler/simplCore/SimplEnv.lhs |   24 ++++++++++++++++--------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/compiler/coreSyn/CoreSubst.lhs b/compiler/coreSyn/CoreSubst.lhs
index 5016479..e2c07af 100644
--- a/compiler/coreSyn/CoreSubst.lhs
+++ b/compiler/coreSyn/CoreSubst.lhs
@@ -116,12 +116,14 @@ For Ids, we have a different invariant
 
 In consequence:
 
-* In substIdBndr, we extend the IdSubstEnv only when the unique changes
+* If the TvSubstEnv and IdSubstEnv are both empty, substExpr would be a
+  no-op, so substExprSC ("short cut") does nothing.
+
+  However, substExpr still goes ahead and substitutes.  Reason: we may
+  want to replace existing Ids with new ones from the in-scope set, to
+  avoid space leaks.
 
-* If the TvSubstEnv and IdSubstEnv are both empty, substExpr does nothing
-  (Note that the above rule for substIdBndr maintains this property.  If
-   the incoming envts are both empty, then substituting the type and
-   IdInfo can't change anything.)
+* In substIdBndr, we extend the IdSubstEnv only when the unique changes
 
 * In lookupIdSubst, we *must* look up the Id in the in-scope set, because
   it may contain non-trivial changes.  Example:
@@ -131,7 +133,7 @@ In consequence:
   set when we find the occurrence of x.
 
 * The requirement to look up the Id in the in-scope set means that we
-  must NOT take no-op short cut in the case the substitution is empty.
+  must NOT take no-op short cut when the IdSubst is empty.
   We must still look up every Id in the in-scope set.
 
 * (However, we don't need to do so for expressions found in the IdSubst
diff --git a/compiler/simplCore/SimplEnv.lhs b/compiler/simplCore/SimplEnv.lhs
index 10b0003..b7b4444 100644
--- a/compiler/simplCore/SimplEnv.lhs
+++ b/compiler/simplCore/SimplEnv.lhs
@@ -131,7 +131,13 @@ pprSimplEnv :: SimplEnv -> SDoc
 -- Used for debugging; selective
 pprSimplEnv env
   = vcat [ptext (sLit "TvSubst:") <+> ppr (seTvSubst env),
- 	  ptext (sLit "IdSubst:") <+> ppr (seIdSubst env) ]
+ 	  ptext (sLit "IdSubst:") <+> ppr (seIdSubst env),
+          ptext (sLit "InScope:") <+> vcat (map ppr_one in_scope_vars)
+    ]
+  where
+   in_scope_vars = varEnvElts (getInScopeVars (seInScope env))
+   ppr_one v | isId v = ppr v <+> ppr (idUnfolding v)
+             | otherwise = ppr v
 
 type SimplIdSubst = IdEnv SimplSR	-- IdId |--> OutExpr
 	-- See Note [Extending the Subst] in CoreSubst
@@ -154,7 +160,8 @@ instance Outputable SimplSR where
 	-- keep uniq _ = uniq `elemUFM_Directly` fvs
 \end{code}
 
-
+Note [SimplEnv invariants]
+~~~~~~~~~~~~~~~~~~~~~~~~~~
 seInScope: 
 	The in-scope part of Subst includes *all* in-scope TyVars and Ids
 	The elements of the set may have better IdInfo than the
@@ -190,9 +197,8 @@ seIdSubst:
 * substId adds a binding (DoneId new_id) to the substitution if 
 	the Id's unique has changed
 
-
   Note, though that the substitution isn't necessarily extended
-  if the type changes.  Why not?  Because of the next point:
+  if the type of the Id changes.  Why not?  Because of the next point:
 
 * We *always, always* finish by looking up in the in-scope set 
   any variable that doesn't get a DoneEx or DoneVar hit in the substitution.
@@ -735,12 +741,14 @@ substIdType (SimplEnv { seInScope = in_scope,  seTvSubst = tv_env}) id
 ------------------
 substExpr :: SDoc -> SimplEnv -> CoreExpr -> CoreExpr
 substExpr doc env
-  = CoreSubst.substExprSC (text "SimplEnv.substExpr1" <+> doc) 
-                          (mkCoreSubst (text "SimplEnv.substExpr2" <+> doc) env) 
+  = CoreSubst.substExpr (text "SimplEnv.substExpr1" <+> doc) 
+                        (mkCoreSubst (text "SimplEnv.substExpr2" <+> doc) env) 
   -- Do *not* short-cut in the case of an empty substitution
-  -- See CoreSubst: Note [Extending the Subst]
+  -- See Note [SimplEnv invariants]
 
 substUnfolding :: SimplEnv -> Unfolding -> Unfolding
-substUnfolding env unf = CoreSubst.substUnfoldingSC (mkCoreSubst (text "subst-unfolding") env) unf
+substUnfolding env unf = CoreSubst.substUnfolding (mkCoreSubst (text "subst-unfolding") env) unf
+  -- Do *not* short-cut in the case of an empty substitution
+  -- See Note [SimplEnv invariants]
 \end{code}
 
-- 
1.7.10.4