Opened 6 years ago

Closed 6 years ago

#7865 closed bug (fixed)

SpecConstr duplicating computations

Reported by: amosrobinson Owned by:
Priority: normal Milestone:
Component: Compiler Version:
Keywords: SpecConstr Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case: simplCore/should_compile/T7865
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

In some (very rare) cases, SpecConstr can actually duplicate let bindings.

When SpecConstr sees something like

let tuple = (let x = expensive in x, simple)
case tuple of
  (a,b) -> a + a

it records the value of tuple, and replaces the case with

(let x = expensive in x) + (let x = expensive in x)

Usually we wouldn't notice this, because the Simplifier would let-float expensive out of the tuple before SpecConstr runs. In some cases though, the tuple constructor will only be exposed after specialisation happens.

To test, compile TSpecConstr_DoubleInline with

ghc TSpecConstr_DoubleInline.hs -O2 -fspec-constr -dverbose-core2core -fforce-recomp -dppr-case-as-let -dsuppress-all  | less

and search for the SpecConstr pass. The first specialisation is

$srecursive_sf3
$srecursive_sf3 =
  \ sc_seR sc_seS sc_seT sc_seU ->
    let {
      a'_sep
      a'_sep =
        (let { I# x_adP ~ _ <- expensive sc_seR } in I# (*# x_adP 2),
         sc_seR) } in
    let {
      ds_seq
      ds_seq =
        recursive
          (: sc_seR (: sc_seR (: sc_seS sc_seT)))
          (sc_seR,
           let { I# x_adP ~ _ <- expensive sc_seR } in I# (*# x_adP 2)) } in
    (let { (p_XdA, q_Xdu) ~ _ <- ds_seq } in
     let { I# x_ae7 ~ _ <- p_XdA } in
     let { I# y_aeb ~ _
     <- let { I# x_adP ~ _ <- expensive sc_seR } in I# (*# x_adP 2)
     } in
     I# (+# x_ae7 y_aeb),
     let { (p_adg, q_XdA) ~ _ <- ds_seq } in
     let { I# x_ae7 ~ _ <- q_XdA } in
     let { I# y_aeb ~ _ <- sc_seR } in I# (+# x_ae7 y_aeb))

With three calls to expensive. If you look at the -ddump-prep, one of the calls is simplified out, but there is still one too many at the end.

This is happening on at least 7.4.1 and head.

Attachments (1)

TSpecConstr_DoubleInline.hs (1.1 KB) - added by amosrobinson 6 years ago.
SpecConstr duplication

Download all attachments as: .zip

Change History (6)

Changed 6 years ago by amosrobinson

Attachment: TSpecConstr_DoubleInline.hs added

SpecConstr duplication

comment:1 Changed 6 years ago by amosrobinson

I am not sure how to fix this. It seems that SpecConstr is trying to do the job of the inliner and case of constructor. Could we make SpecConstr ignore these cases and run it multiple times, between each Simplifier pass? Or perhaps call the Simplifier from SpecConstr when this inlining needs to be done?

comment:2 Changed 6 years ago by amosrobinson

Or maybe just let-floating on constructors would fix it?

comment:3 Changed 6 years ago by simonpj

difficulty: Unknown

Excellent point. SpecConstr is (and I'm afraid must) do some on-the-fly simplification, but here is is going too far by duplicating an expensive sub-computation.

I've made the on-the-fly inlining less aggressive; and written a long comment about it Note [Work-free values only in environment]. Pushing shortly.

Thanks for identifying this so clearly.

Simon

comment:4 Changed 6 years ago by simonpj@…

commit ed54858977e98a833a5767a9c2d07b05c20e5aff

Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Fri May 3 12:00:19 2013 +0100

    Do not duplicate work in SpecConstr (fix Trac #7865)
    
    This is a bad bug, if a rare one.
    See Note [Work-free values only in environment].
    
    Thanks to Amos Robinson for finding it.

 compiler/specialise/SpecConstr.lhs |   63 +++++++++++++++++++++++++++++++++---
 1 files changed, 58 insertions(+), 5 deletions(-)

comment:5 Changed 6 years ago by simonpj

Resolution: fixed
Status: newclosed
Test Case: simplCore/should_compile/T7865
Note: See TracTickets for help on using tickets.