Possible inconsistency in CSE's treatment of NOINLINE (#13589) · Issues · Glasgow Haskell Compiler / GHC

Possible inconsistency in CSE's treatment of NOINLINE

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.

Trac metadata

Trac field	Value
Version	8.0.1
Type	Bug
TypeOfFailure	OtherFailure
Priority	normal
Resolution	Unresolved
Component	Compiler
Test case
Differential revisions
BlockedBy
Related
Blocking
CC
Operating system
Architecture

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,

```hs
{-# 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,

```hs
  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.

<details><summary>Trac metadata</summary>

| Trac field             | Value        |
| ---------------------- | ------------ |
| Version                | 8.0.1        |
| Type                   | Bug          |
| TypeOfFailure          | OtherFailure |
| Priority               | normal       |
| Resolution             | Unresolved   |
| Component              | Compiler     |
| Test case              |              |
| Differential revisions |              |
| BlockedBy              |              |
| Related                |              |
| Blocking               |              |
| CC                     |              |
| Operating system       |              |
| Architecture           |              |

</details>