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.

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'])