summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSimon Peyton Jones <simonpj@microsoft.com>2021-07-16 09:22:24 +0100
committerMarge Bot <ben+marge-bot@smart-cactus.org>2021-07-19 19:38:21 -0400
commit535123e4f6505a148ccaa536c21282a87c42669c (patch)
treee971e86b039acc8e508483c09a3a0c4399b3e3b6
parent3e8b39eaf098769d33c9a46657c18a277397424c (diff)
downloadhaskell-535123e4f6505a148ccaa536c21282a87c42669c.tar.gz
Don't duplicate constructors in the simplifier
Ticket #20125 showed that the Simplifier could sometimes duplicate a constructor binding. CSE would often eliminate it later, but doing it in the first place was utterly wrong. See Note [Do not duplicate constructor applications] in Simplify.hs I also added a short-cut to Simplify.simplNonRecX for the case when the RHS is trivial. I don't think this will change anything, just make the compiler run a tiny bit faster.
-rw-r--r--compiler/GHC/Core/Opt/Simplify.hs48
-rw-r--r--testsuite/tests/simplCore/should_compile/T20125.hs18
-rw-r--r--testsuite/tests/simplCore/should_compile/T20125.stderr138
-rw-r--r--testsuite/tests/simplCore/should_compile/all.T2
4 files changed, 196 insertions, 10 deletions
diff --git a/compiler/GHC/Core/Opt/Simplify.hs b/compiler/GHC/Core/Opt/Simplify.hs
index 28c5548745..1bbb728de6 100644
--- a/compiler/GHC/Core/Opt/Simplify.hs
+++ b/compiler/GHC/Core/Opt/Simplify.hs
@@ -425,6 +425,12 @@ simplNonRecX env bndr new_rhs
| Coercion co <- new_rhs
= return (emptyFloats env, extendCvSubst env bndr co)
+ | exprIsTrivial new_rhs -- Short-cut for let x = y in ...
+ -- This case would ultimately land in postInlineUnconditionally
+ -- but it seems not uncommon, and avoids a lot of faff to do it here
+ = return (emptyFloats env
+ , extendIdSubst env bndr (DoneEx new_rhs Nothing))
+
| otherwise
= do { (env', bndr') <- simplBinder env bndr
; completeNonRecX NotTopLevel env' (isStrictId bndr') bndr bndr' new_rhs }
@@ -2682,6 +2688,27 @@ case b of { b' -> f b' }.
We could try and be more clever (like maybe wfloats only contain
let binders, so we could float them). But the need for the
extra complication is not clear.
+
+Note [Do not duplicate constructor applications]
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Consider this (#20125)
+ let x = (a,b)
+ in ...(case x of x' -> blah)...x...x...
+
+We want that `case` to vanish (since `x` is bound to a data con) leaving
+ let x = (a,b)
+ in ...(let x'=x in blah)...x..x...
+
+In rebuildCase, `exprIsConApp_maybe` will succeed on the scrutinee `x`,
+since is bound to (a,b). But in eliminating the case, if the scrutinee
+is trivial, we want to bind the case-binder to the scrutinee, /not/ to
+the constructor application. Hence the case_bndr_rhs in rebuildCase.
+
+This applies equally to a non-DEFAULT case alternative, say
+ let x = (a,b) in ...(case x of x' { (p,q) -> blah })...
+This variant is handled by bind_case_bndr in knownCon.
+
+We want to bind x' to x, and not to a duplicated (a,b)).
-}
---------------------------------------------------------
@@ -2715,19 +2742,21 @@ rebuildCase env scrut case_bndr alts cont
, let env0 = setInScopeSet env in_scope'
= do { tick (KnownBranch case_bndr)
; let scaled_wfloats = map scale_float wfloats
+ -- case_bndr_unf: see Note [Do not duplicate constructor applications]
+ case_bndr_rhs | exprIsTrivial scrut = scrut
+ | otherwise = con_app
+ con_app = Var (dataConWorkId con) `mkTyApps` ty_args
+ `mkApps` other_args
; case findAlt (DataAlt con) alts of
- Nothing -> missingAlt env0 case_bndr alts cont
- Just (Alt DEFAULT bs rhs) -> let con_app = Var (dataConWorkId con)
- `mkTyApps` ty_args
- `mkApps` other_args
- in simple_rhs env0 scaled_wfloats con_app bs rhs
- Just (Alt _ bs rhs) -> knownCon env0 scrut scaled_wfloats con ty_args other_args
- case_bndr bs rhs cont
+ Nothing -> missingAlt env0 case_bndr alts cont
+ Just (Alt DEFAULT bs rhs) -> simple_rhs env0 scaled_wfloats case_bndr_rhs bs rhs
+ Just (Alt _ bs rhs) -> knownCon env0 scrut scaled_wfloats con ty_args
+ other_args case_bndr bs rhs cont
}
where
- simple_rhs env wfloats scrut' bs rhs =
+ simple_rhs env wfloats case_bndr_rhs bs rhs =
assert (null bs) $
- do { (floats1, env') <- simplNonRecX env case_bndr scrut'
+ do { (floats1, env') <- simplNonRecX env case_bndr case_bndr_rhs
-- scrut is a constructor application,
-- hence satisfies let/app invariant
; (floats2, expr') <- simplExprF env' rhs cont
@@ -3293,6 +3322,7 @@ knownCon env scrut dc_floats dc dc_ty_args dc_args bndr bs rhs cont
| isDeadBinder bndr = return (emptyFloats env, env)
| exprIsTrivial scrut = return (emptyFloats env
, extendIdSubst env bndr (DoneEx scrut Nothing))
+ -- See Note [Do not duplicate constructor applications]
| otherwise = do { dc_args <- mapM (simplVar env) bs
-- dc_ty_args are already OutTypes,
-- but bs are InBndrs
diff --git a/testsuite/tests/simplCore/should_compile/T20125.hs b/testsuite/tests/simplCore/should_compile/T20125.hs
new file mode 100644
index 0000000000..51037d660d
--- /dev/null
+++ b/testsuite/tests/simplCore/should_compile/T20125.hs
@@ -0,0 +1,18 @@
+{-# OPTIONS_GHC -fno-cpr-anal -fno-cse #-}
+-- CSE recovers good code, but we want to expose it going bad
+-- CPR gives a stable unfolding which clutters the output
+
+module T20125 where
+
+data T = MkT Int Int
+
+f x = let y = MkT x x
+ in (y, y `seq` (y,y))
+
+{- We expect this to optimise to
+
+f x = let y = MkT x x
+ in (y, (y,y))
+
+without MkT being duplicated
+-}
diff --git a/testsuite/tests/simplCore/should_compile/T20125.stderr b/testsuite/tests/simplCore/should_compile/T20125.stderr
new file mode 100644
index 0000000000..22e5e908ad
--- /dev/null
+++ b/testsuite/tests/simplCore/should_compile/T20125.stderr
@@ -0,0 +1,138 @@
+
+==================== Tidy Core ====================
+Result size of Tidy Core
+ = {terms: 67, types: 31, coercions: 0, joins: 0/1}
+
+-- RHS size: {terms: 10, types: 8, coercions: 0, joins: 0/1}
+f :: Int -> (T, (T, T))
+[GblId,
+ Arity=1,
+ Str=<L>,
+ Cpr=1,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [0] 40 10}]
+f = \ (x :: Int) ->
+ let {
+ y :: T
+ [LclId, Unf=OtherCon []]
+ y = T20125.MkT x x } in
+ (y, (y, y))
+
+-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
+T20125.$trModule4 :: GHC.Prim.Addr#
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 20 0}]
+T20125.$trModule4 = "main"#
+
+-- RHS size: {terms: 2, types: 0, coercions: 0, joins: 0/0}
+T20125.$trModule3 :: GHC.Types.TrName
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$trModule3 = GHC.Types.TrNameS T20125.$trModule4
+
+-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
+T20125.$trModule2 :: GHC.Prim.Addr#
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 30 0}]
+T20125.$trModule2 = "T20125"#
+
+-- RHS size: {terms: 2, types: 0, coercions: 0, joins: 0/0}
+T20125.$trModule1 :: GHC.Types.TrName
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$trModule1 = GHC.Types.TrNameS T20125.$trModule2
+
+-- RHS size: {terms: 3, types: 0, coercions: 0, joins: 0/0}
+T20125.$trModule :: GHC.Types.Module
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$trModule
+ = GHC.Types.Module T20125.$trModule3 T20125.$trModule1
+
+-- RHS size: {terms: 3, types: 1, coercions: 0, joins: 0/0}
+$krep :: GHC.Types.KindRep
+[GblId, Unf=OtherCon []]
+$krep
+ = GHC.Types.KindRepTyConApp
+ GHC.Types.$tcInt (GHC.Types.[] @GHC.Types.KindRep)
+
+-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
+T20125.$tcT2 :: GHC.Prim.Addr#
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 20 0}]
+T20125.$tcT2 = "T"#
+
+-- RHS size: {terms: 2, types: 0, coercions: 0, joins: 0/0}
+T20125.$tcT1 :: GHC.Types.TrName
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$tcT1 = GHC.Types.TrNameS T20125.$tcT2
+
+-- RHS size: {terms: 7, types: 0, coercions: 0, joins: 0/0}
+T20125.$tcT :: GHC.Types.TyCon
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$tcT
+ = GHC.Types.TyCon
+ 2636760236657926773##
+ 9933143121152832090##
+ T20125.$trModule
+ T20125.$tcT1
+ 0#
+ GHC.Types.krep$*
+
+-- RHS size: {terms: 3, types: 1, coercions: 0, joins: 0/0}
+$krep1 :: GHC.Types.KindRep
+[GblId, Unf=OtherCon []]
+$krep1
+ = GHC.Types.KindRepTyConApp
+ T20125.$tcT (GHC.Types.[] @GHC.Types.KindRep)
+
+-- RHS size: {terms: 3, types: 0, coercions: 0, joins: 0/0}
+$krep2 :: GHC.Types.KindRep
+[GblId, Unf=OtherCon []]
+$krep2 = GHC.Types.KindRepFun $krep $krep1
+
+-- RHS size: {terms: 3, types: 0, coercions: 0, joins: 0/0}
+T20125.$tc'MkT1 [InlPrag=[~]] :: GHC.Types.KindRep
+[GblId, Unf=OtherCon []]
+T20125.$tc'MkT1 = GHC.Types.KindRepFun $krep $krep2
+
+-- RHS size: {terms: 1, types: 0, coercions: 0, joins: 0/0}
+T20125.$tc'MkT3 :: GHC.Prim.Addr#
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 20 0}]
+T20125.$tc'MkT3 = "'MkT"#
+
+-- RHS size: {terms: 2, types: 0, coercions: 0, joins: 0/0}
+T20125.$tc'MkT2 :: GHC.Types.TrName
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$tc'MkT2 = GHC.Types.TrNameS T20125.$tc'MkT3
+
+-- RHS size: {terms: 7, types: 0, coercions: 0, joins: 0/0}
+T20125.$tc'MkT :: GHC.Types.TyCon
+[GblId,
+ Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
+ WorkFree=True, Expandable=True, Guidance=IF_ARGS [] 10 10}]
+T20125.$tc'MkT
+ = GHC.Types.TyCon
+ 16774178122498486797##
+ 3923705917114679617##
+ T20125.$trModule
+ T20125.$tc'MkT2
+ 0#
+ T20125.$tc'MkT1
+
+
+
diff --git a/testsuite/tests/simplCore/should_compile/all.T b/testsuite/tests/simplCore/should_compile/all.T
index ed45e9dc65..5f742742d1 100644
--- a/testsuite/tests/simplCore/should_compile/all.T
+++ b/testsuite/tests/simplCore/should_compile/all.T
@@ -365,4 +365,4 @@ test('T19672', normal, compile, ['-O2 -ddump-rules'])
test('T19780', normal, compile, ['-O2'])
test('T19794', normal, compile, ['-O'])
test('T19890', [ grep_errmsg(r'= T19890.foo1') ], compile, ['-O -ddump-simpl'])
-
+test('T20125', [ grep_errmsg(r'= T20125.MkT') ], compile, ['-O -ddump-simpl -dsuppress-uniques'])