Remove ad-hoc special case in occAnal

Authored by simonpj on Jun 7 2018, 5:03 AM.

Description

Remove ad-hoc special case in occAnal

Back in 1999 I put this ad-hoc code in the Case-handling
code for occAnal:

occAnal env (Case scrut bndr ty alts)
 = ...
      -- Note [Case binder usage]
      -- ~~~~~~~~~~~~~~~~~~~~~~~~
      -- The case binder gets a usage of either "many" or "dead", never "one".
      -- Reason: we like to inline single occurrences, to eliminate a binding,
      -- but inlining a case binder *doesn't* eliminate a binding.
      -- We *don't* want to transform
      --      case x of w { (p,q) -> f w }
      -- into
      --      case x of w { (p,q) -> f (p,q) }
  tag_case_bndr usage bndr
    = (usage', setIdOccInfo bndr final_occ_info)
    where
      occ_info       = lookupDetails usage bndr
      usage'         = usage `delDetails` bndr
      final_occ_info = case occ_info of IAmDead -> IAmDead
                                        _       -> noOccInfo

But the comment looks wrong -- the bad inlining will not happen -- and
I think it relates to some long-ago version of the simplifier.

So I simply removed the special case, which gives more accurate
occurrence-info to the case binder. Interestingly I got a slight
improvement in nofib binary sizes.


Program           Size    Allocs   Runtime   Elapsed  TotalMem

cacheprof          -0.1%     +0.2%     -0.7%     -1.2%     +8.6%

Min          -0.2%      0.0%    -14.5%    -30.5%      0.0%
Max          -0.1%     +0.2%    +10.0%    +10.0%    +25.0%

Geometric Mean -0.2% +0.0% -1.9% -5.4% +0.3%

I have no idea if the improvement in runtime is real. I did look at the
tiny increase in allocation for cacheprof and concluded that it was
unimportant (I forget the details).

Also the more accurate occ-info for the case binder meant that some
inlining happens in one pass that previously took successive passes
for the test dependent/should_compile/dynamic-paper (which has a
known Russel-paradox infinite loop in the simplifier).

In short, a small win: less ad-hoc complexity and slightly smaller
binaries.

bgamari raised a concern with this commit.Jun 7 2018, 10:17 AM
bgamari added a subscriber: bgamari.

It looks like this broke the dynamic-paper test (although strangely only on amd64).

This commit now has outstanding concerns.Jun 7 2018, 10:17 AM

Indeed it looks like the test explicitly notes the failure mode that we are now seeing:

delta1 :: Dynamic -> Dynamic
-- NB: this function behaves like a negative-recursive data type
-- and hence leads compiler into an infinite inlining loop,
-- and we get "simplifier ticks exhausted".
-- See Section 7 of the paper "A reflection on types"
delta1 dn = case fromDynamic dn of
             Just f   -> f dn
             Nothing  -> dn
loop1 = delta1 (toDynamic delta1)

Ahh, in fact the commit message explicitly mentions this and this commit marks the test as broken. It looks like the issue is an out-of-date stderr file.

The test is *supposed* to fail -- it's an example taken from the paper.

But before this patch it didn't fail, just, because each simplifier pass did only one inlining, and we stop at 4 passes. Now each pass does infinite inilining.

So it does now fail, and there should e a stderr file for it, which indeed is added.

Maybe you are saying that on some platform, it succeeds? If so I don't know why.

bgamari accepted this commit.Jun 7 2018, 5:04 PM

The test is *supposed* to fail -- it's an example taken from the paper.

But before this patch it didn't fail, just, because each simplifier pass did only one inlining, and we stop at 4 passes. Now each pass does infinite inilining.

So it does now fail, and there should e a stderr file for it, which indeed is added.

Maybe you are saying that on some platform, it succeeds? If so I don't know why.

The trouble is that the stderr file is empty, whereas GHC throws an error message. It sounds as though this is a mistake; I will fix this.

All concerns with this commit have now been addressed.Jun 7 2018, 5:04 PM

The trouble is that the stderr file is empty, whereas GHC throws an error message. It sounds as though this is a mistake; I will fix this.

Ah, yes that *is* a mistake. Sorry!