Opened 2 years ago

Last modified 12 months ago

#14186 patch bug

CSE fails to CSE two identical large top-level functions

Reported by: nomeata Owned by:
Priority: normal Milestone:
Component: Compiler Version: 8.3
Keywords: CSE Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #13589 Differential Rev(s): Phab:D3939
Wiki Page:

Description (last modified by bgamari)

Consider this code:

module CSEBug where

data Succs a = Succs a [a]

instance Functor Succs where
    fmap f (Succs o s) = Succs (f o) (map f s)

foo, bar :: (a -> b) -> (b -> c) -> Succs a -> Succs c
foo f g x = fmap (g . f) x
bar f g x = fmap (g . f) x

If I compile this with -O, foo and bar are not CSEd, which can be seen with -ddump-cse.

Removing either the first or the second argument of Succs makes CSE work.

Is there a size limit on CSE?

Change History (8)

comment:1 Changed 2 years ago by nomeata

Summary: CSE fails to CSECSE fails to CSE two identical large top-level functions

comment:2 Changed 2 years ago by simonpj

Ha ha. This is caused by two things

  1. We don't CSE things with an INLINE pragma: see Note [CSE for INLINE and NOINLINE] in CSE.hs
  1. The worker/wrapper pass (right after demand analysis) gives the wrapper an inline pragma of INLINE[0].

So (2) means that (1) means no CSE for foo and bar. Sigh. What to do?

Well

  • InlinePragma already distinguishse between the inl_inline field and inl_act. See Note [inl_inline and inl_act] in BasicTypes.
  • So we should be able to distinguish between a user-supplied INLINE pragma inl_inline and one supplied the w/w pass.
  • If we could distinguish then CSDE could fail on the user-supplied kind, but succeed on the w/w supplied kind.

comment:3 Changed 2 years ago by nomeata

So we should be able to distinguish between a user-supplied INLINE pragma inl_inline and one supplied the w/w pass.

Are you saying that we can do that already, without changes to BasicTypes, or that we should extend BasicTypes to be able to do that?

The current code in WorkWrap is

            wrap_act  = ActiveAfter NoSourceText 0
            wrap_prag = InlinePragma { inl_src = SourceText "{-# INLINE"
                                     , inl_inline = Inline
                                     , inl_sat    = Nothing
                                     , inl_act    = wrap_act
                                     , inl_rule   = rule_match_info }

and unless I check for NoSourceText, this seems to be indistinguishable from a INLINE[0] in the source.

comment:4 Changed 2 years ago by simonpj

I think perhaps the wrap_prag can be EmptyInlineSpec. That might do it.

comment:5 Changed 2 years ago by nomeata

Differential Rev(s): Phab:D3939
Status: newpatch

Phab:D3939 has a flight train result.

comment:6 Changed 2 years ago by bgamari

Description: modified (diff)
Keywords: CSE added

#13589 is also somewhat relevant here.

Last edited 12 months ago by bgamari (previous) (diff)

comment:7 Changed 2 years ago by Joachim Breitner <mail@…>

In fe35b85a/ghc:

Add testcase for #14186

and move the generally useful helpers check_errmsg and grep_errmsg to
testlib.py. Some documentation can be found on
https://ghc.haskell.org/trac/ghc/wiki/Building/RunningTests/Adding

comment:8 Changed 2 years ago by Joachim Breitner <mail@…>

In fe04f378/ghc:

Allow CSE'ing of work-wrapped bindings (#14186)

the worker/wrapper creates an artificial INLINE pragma, which caused CSE
to not do its work. We now recognize such artificial pragmas by using
`NoUserInline` instead of `Inline` as the `InlineSpec`.

Differential Revision: https://phabricator.haskell.org/D3939
Note: See TracTickets for help on using tickets.