Opened 2 years ago

Last modified 2 years ago

#13589 new bug

Possible inconsistency in CSE's treatment of NOINLINE

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

Description

While debugging #13535 I noticed the following inconsistency in CSE:

Note [CSE for INLINE and NOINLINE] states that we need to take care when adding expressions bound to binders with inline pragmas to the CSEnv. To see why, consider the following,

{-# NOINLINE bar #-}
bar = <rhs>     -- Same rhs as foo

foo = <rhs>

Given this program, we need to avoid producing foo = bar since doing so would mean that we would lose the ability to inline foo's original RHS.

The note then goes on to give the following rule,

We should not add

<rhs> :-> bar

to the CSEnv if bar has any constraints on when it can inline; that is, if its 'activation' not always active. Otherwise we might replace <rhs> by bar, and then later be unable to see that it really was <rhs>.

This rule is implemented in noCSE with,

  not (isAlwaysActive (idInlineActivation id))

However, it's quite unclear to me that this rule avoids the issue we set out to solve. Afterall, NOINLINE bar is always active, but it still means that rewriting foo to foo=bar would lose us the ability to see foo's original RHS.

Change History (4)

comment:1 Changed 2 years ago by simonpj

I think the rule does address the issue. If CSE fires we'll get

bar = rhs
foo = bar

Before CSE, if foo is inlined, then the context into which it inlines will see <rhs>. After CSE, if we inline foo we'll see bar not <rhs> so we want bar to be able to inline too.

Afterall, NOINLINE bar is always active,

No: a NOINLINE pragma cause a NeverActive activation.

comment:2 Changed 2 years ago by simonpj

That said, it is silly that we can't CSE

{-# INLINE [0] f #-}
f x = blah

{-# INLINE [0] g #-}
g x = blah

How could we solve this? Something like this:

  • Have cs_map :: CoreMap (OutExpr, Activation)
  • Add blah :-> (f, ActiveAfter 0) when extending the cs_map in cse_bind.
  • In tryForCSE, when called from cse_bind, pass the Activation of the binding (g in this example), and check that the Activation of the new Id is the same.

comment:3 Changed 2 years ago by bgamari

No: a NOINLINE pragma cause a NeverActive activation.

Ahh, there's my misunderstanding.

How could we solve this? Something like this:

Sounds reasonable to me.

comment:4 Changed 2 years ago by simonpj

Keywords: CSE added
Note: See TracTickets for help on using tickets.