[GHC] #11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration
-------------------------------------+-------------------------------------
Reporter: nomeata | Owner:
Type: bug | Status: new
Priority: normal | Milestone:
Component: Compiler | Version: 8.1
Keywords: | Operating System: Unknown/Multiple
Architecture: | Type of failure: None/Unknown
Unknown/Multiple |
Test Case: | Blocked By:
Blocking: | Related Tickets:
Differential Rev(s): | Wiki Page:
-------------------------------------+-------------------------------------
After quite a while of staring at the core of nofib’s `fft2` benchmark,
where my dynamic entry counting code (#10613), I found the problem. Here
is a small example:
{{{
foo :: Int -> Int -> Int
foo 10 c = 0
foo n c =
let bar :: Int -> Int
bar n = n + c
{-# NOINLINE bar #-}
in bar n + foo (bar (n+1)) c
}}}
Clearly, `bar` is not single-entry. But the demand analyzer believes it
is:
{{{
Rec {
-- RHS size: {terms: 32, types: 12, coercions: 0}
foo [Occ=LoopBreaker] :: Int -> Int -> Int
[LclIdX,
Arity=2,
Str=] :: Int) (c [Dmd=] ->
case ds of ds {
__DEFAULT ->
let {
bar [InlPrag=NOINLINE, Dmd=m {axl->},
Unf=Unf{Src=<vanilla>, TopLvl=False, Value=True, ConLike=True,
WorkFree=True, Expandable=True, Guidance=IF_ARGS [0] 30
0}]
bar =
\ (n [Dmd=, OS=OneShot] :: Int) ->
$fNumInt_$c+ n c } in
case bar wild of _ [Occ=Dead, Dmd=] ->
case foo (bar (I# (+# ds 1#))) c
of _ [Occ=Dead, Dmd=] ->
I# (+# x y)
}
};
10# -> lvl
}
}
end Rec }
}}}
The reason is that during the first fixed-point iteration for `foo`, `foo`
itself is assumed to not put any demand on its arguments. Under this
assumption, it is correct to find that `bar` is called at most once. This
is then noted in the lambda binder. The second iteration corrects the
demand, but not the one-shot annotation, because that is only added by the
demand analyzer, never dropped:
{{{#!hs
setOneShotness :: Count -> Id -> Id
setOneShotness One bndr = setOneShotLambda bndr
setOneShotness Many bndr = bndr
}}}
This can be fixed by changing that code (from `DmdAnal.hs`) to
{{{#!hs
setOneShotness :: Count -> Id -> Id
setOneShotness One bndr = setOneShotLambda bndr
setOneShotness Many bndr = clearOneShotLambda bndr
}}}
But this would have other consequences, e.g. erasing any possible manual
one-shot annotations using `oneShot`.
Or maybe `setOneShotness` should not be set by the demand analyzer during
its work, but once at the end, or maybe even the next simplifier pass
should take care of that.
--
Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770
GHC http://www.haskell.org/ghc/
The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): Yikes! That's terrible! As well as that outright bug, there's a strange duplication: * the demand analyser goes to some effort to annotate one-shot info (e.g. `annLamWithShotness`) * but so does the occurrence analyser (see the `occ_one_shots` field of `OccEnv`) Let's do it in the occurrence analyser, as now. So try this: * Get rid of all one-shot setting in the demand analyser, letting the occurrence analyser do the work. The demand analyser would then annotate only demand info. * BUT in `OccAnal.oneShotGroup` I'm suspicious of that `updOneShotInfo` which keeps one-shot info if it was there before. I don't think it's an actual bug, but I'd be much more comfortable with using `setIdOneShotInfo` instead. That's an orthogonal change. Would you like to try those two changes (ideally independently) and see what effect it has? -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:1 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration
-------------------------------------+-------------------------------------
Reporter: nomeata | Owner:
Type: bug | Status: new
Priority: normal | Milestone:
Component: Compiler | Version: 8.1
Resolution: | Keywords:
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s):
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Joachim Breitner

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by nomeata):
Get rid of all one-shot setting in the demand analyser, letting the occurrence analyser do the work. The demand analyser would then annotate only demand info.
But would that not mean that useful one-shot information determined by the demand analyzer will not be reflected here, and hence not used in later passes, e.g. by the simplifier when trying to to floating a let into a lambda?
I'm suspicious of that updOneShotInfo which keeps one-shot info if it was there before.
Note that due to the magic `oneShot` function, there might be one-shot info worth preserving. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:3 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

But would that not mean that useful one-shot information determined by
#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by simonpj): Replying to [comment:3 nomeata]: the demand analyzer will not be reflected here, and hence not used in later passes, e.g. by the simplifier when trying to to floating a let into a lambda? No, that'll be fine: the occurrence analyser runs before each pass of the simplifier.
Note that due to the magic `oneShot` function, there might be one-shot info worth preserving.
Good point! Please comment this prominently with `updOneShotInfo`. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:4 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

No, that'll be fine: the occurrence analyser runs before each pass of
#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Wiki Page: | -------------------------------------+------------------------------------- Comment (by nomeata): the simplifier. Aha, the OccurAnal code actually looks at `idDemandInfo` of a let-bound expression. Didn’t know that, but it makes sense now. I’ll add that to the relevant note.
Good point! Please comment this prominently with updOneShotInfo.
Done, in `wip/T11770` for now. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:5 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: patch Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2070 Wiki Page: | -------------------------------------+------------------------------------- Changes (by nomeata): * status: new => patch * differential: => Phab:D2070 Comment: The branch with this patch on has passed through validation a few times, so I think it’s safe to merge this; put it up for a brief review at Phab:D2070. Code removal is always nice. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:6 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration
-------------------------------------+-------------------------------------
Reporter: nomeata | Owner:
Type: bug | Status: patch
Priority: normal | Milestone:
Component: Compiler | Version: 8.1
Resolution: | Keywords:
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D2070
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Joachim Breitner

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: closed Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2070 Wiki Page: | -------------------------------------+------------------------------------- Changes (by nomeata): * status: patch => closed * resolution: => fixed -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:8 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2070 Wiki Page: | -------------------------------------+------------------------------------- Changes (by nomeata): * status: closed => new * resolution: fixed => Comment: I must have mis-validated this, so I did not notice noticeable performance regressions; reverted it in 6ea42c72dc924eddba3f2ee22fa4e514084fa5cc. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:9 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2070 Wiki Page: | -------------------------------------+------------------------------------- Comment (by nomeata): The performance regressions (have not looked at details yet) are very annyoing: My impression was that this change would not make a difference, i.e. the same one-shot annotations would be set, only in a slightly later phase (occurrence analyzer instead of demand analyzer). With the exception, of course, of wrongly set one-shot annotations; this is of course what we want to fix. So if this causes performance regressions, then it must have been the case that GHC was doing something to the code that was not solidly warranted (inlined into a lambda that was not guaranteed to be one-shot) and could have gone wrong, but indeed improved matters (either because the lambda was one-shot after all, or the duplicated work was less than the benefits of inlining). So by fixing a bug, we reduce performance. Reminds me awful lot of trying to make the state hack less aggressive. There is collateral damage. Also, there were some improvements, so in some cases, GHC did make a bold, unwarranted change to the code and it indeed went wrong. So I guess the proper thing to do is to look at all the regressions and decide whether we can improve the compiler in other ways to make this work, before being allowed to push this bug fix. How tedious, especially with performance regressions in the compiler. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:10 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2070 Wiki Page: | -------------------------------------+------------------------------------- Comment (by nomeata): TODO: Calculate called-once-demend on the wrapper in `splitFun`, and get rid of `oneShot` annotation calculation in `mkWwBodies`. Might fix the performance regressions. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:11 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: new Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2070 Wiki Page: | -------------------------------------+------------------------------------- Comment (by nomeata): I gave it a shot, but the test suite is still unhappy. I’m running out of my time slice for this week (and I’m traveling from tomorrow until Wednesday) but I pushed what I have to `wip/T11770`. If someone wants to review it, or even continue working on it, feel free! -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:12 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: patch Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2085 Wiki Page: | -------------------------------------+------------------------------------- Changes (by nomeata): * status: new => patch * differential: Phab:D2070 => Phab:D2085 Comment: Ok, I found that the occurrence analyzer was not reliably setting OneShot annotations, and fixed it, and now there are no significatn performance changes at all.. Phew. So this seems to be safe to apply. I pushed it to phab (Phab:D2085) for review. -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:13 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration
-------------------------------------+-------------------------------------
Reporter: nomeata | Owner:
Type: bug | Status: patch
Priority: normal | Milestone:
Component: Compiler | Version: 8.1
Resolution: | Keywords:
Operating System: Unknown/Multiple | Architecture:
| Unknown/Multiple
Type of failure: None/Unknown | Test Case:
Blocked By: | Blocking:
Related Tickets: | Differential Rev(s): Phab:D2085
Wiki Page: |
-------------------------------------+-------------------------------------
Comment (by Joachim Breitner

#11770: Demand analysis: Wrong one-shot annotation due to fixed-point iteration -------------------------------------+------------------------------------- Reporter: nomeata | Owner: Type: bug | Status: closed Priority: normal | Milestone: Component: Compiler | Version: 8.1 Resolution: fixed | Keywords: Operating System: Unknown/Multiple | Architecture: | Unknown/Multiple Type of failure: None/Unknown | Test Case: Blocked By: | Blocking: Related Tickets: | Differential Rev(s): Phab:D2085 Wiki Page: | -------------------------------------+------------------------------------- Changes (by nomeata): * status: patch => closed * resolution: => fixed -- Ticket URL: http://ghc.haskell.org/trac/ghc/ticket/11770#comment:15 GHC http://www.haskell.org/ghc/ The Glasgow Haskell Compiler
participants (1)
-
GHC