Opened 4 years ago

Last modified 7 months ago

#5780 infoneeded bug

-faggressive-primops change caused a failure in perf/compiler/parsing001

Reported by: simonmar Owned by:
Priority: normal Milestone: 7.12.1
Component: Compiler Version: 7.2.2
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

The commit 601c983dd0bada6b49bdadd8f172fd4eacac4b0c apparently caused a regression in perf/compiler/parsing001, so I'm reverting it pending investigation. The patch should have made no changes to performance, because the new functionality was only enabled by a new flag.

Change History (9)

comment:1 Changed 4 years ago by marlowsd@…

commit 395ef284545bb5e7a57c180cc1f2a1b66fa30f37

Author: Simon Marlow <[email protected]>
Date:   Mon Jan 16 12:41:56 2012 +0000

    Revert "Add -faggressive-primops plus refactoring in CoreUtils" (#5780)
    
    This reverts commit 601c983dd0bada6b49bdadd8f172fd4eacac4b0c.

 compiler/coreSyn/CoreArity.lhs    |    4 +-
 compiler/coreSyn/CoreUtils.lhs    |  237 ++++++++++++++++--------------------
 compiler/main/StaticFlagParser.hs |    1 -
 compiler/main/StaticFlags.hs      |    6 -
 compiler/prelude/PrimOp.lhs       |   59 ++-------
 compiler/simplCore/FloatIn.lhs    |   10 +--
 compiler/simplCore/OccurAnal.lhs  |    4 +-
 compiler/simplCore/SimplUtils.lhs |   10 +-
 compiler/simplCore/Simplify.lhs   |   20 ++--
 9 files changed, 137 insertions(+), 214 deletions(-)

comment:2 Changed 3 years ago by simonpj

BTW here's the original commit message (the one that was reverted):

commit 601c983dd0bada6b49bdadd8f172fd4eacac4b0c
Author: Simon Peyton Jones <[email protected]>
Date:   Fri Jan 13 17:50:00 2012 +0000

    Add -faggressive-primops plus refactoring in CoreUtils
    
    I'm experimenting with making GHC a bit more aggressive about
      a) dropping case expressions if the result is unused
            Simplify.rebuildCase, CaseElim equation
    
      b) floating case expressions inwards
            FloatIn.fiExpr, AnnCase
    
    In both cases the new behaviour is gotten with a static (debug)
    flag -faggressive-primops.  The extra "aggression" is to allow
    discarding and floating in for side-effecting operations.  See
    the new, extensive Note [PrimOp can_fail and has_side_effects]
    in PrimoOp.
    
    When discarding a case with unused binders, in the lifted-type
    case it's definitely ok if the scrutinee terminates; previously
    we were checking exprOkForSpeculation, which is significantly
    worse.
    
    So I wanted a new function CoreUtils.exprCertainlyTerminates.
    In doing this I ended up with a significant refactoring in
    CoreUtils.  The new structure has quite a lot of nice sharing:
    
        exprIsCheap             = exprIsCheap' isHNFApp
        exprIsExpandable        = exprIsCheap' isConLikeApp
    
        exprIsHNF               = exprIsHNFlike isHNFApp
        exprIsConLike           = exprIsHNFlike isConLikeApp
        exprCertainlyTerminates = exprIsHNFlike isTerminatingApp

 compiler/coreSyn/CoreArity.lhs    |    4 +-
 compiler/coreSyn/CoreUtils.lhs    |  237 ++++++++++++++++++++----------------
 compiler/main/StaticFlagParser.hs |    1 +
 compiler/main/StaticFlags.hs      |    6 +
 compiler/prelude/PrimOp.lhs       |   59 +++++++--
 compiler/simplCore/FloatIn.lhs    |   10 ++-
 compiler/simplCore/OccurAnal.lhs  |    4 +-
 compiler/simplCore/SimplUtils.lhs |   10 +-
 compiler/simplCore/Simplify.lhs   |   20 ++--
 9 files changed, 214 insertions(+), 137 deletions(-)

comment:3 Changed 3 years ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

comment:4 Changed 3 years ago by igloo

  • Milestone changed from 7.6.2 to 7.8.1

comment:5 Changed 15 months ago by thoughtpolice

  • Milestone changed from 7.8.3 to 7.10.1

Bumping priority down (these tickets haven't been closely followed or fixed in 7.4), and moving out to 7.10 and out of 7.8.3.

comment:6 Changed 15 months ago by thoughtpolice

  • Priority changed from high to normal

Actually dropping priority. :)

comment:7 Changed 8 months ago by thomie

  • Status changed from new to infoneeded

SPJ: this ticket does not appear on your list. Should it stay open? The reverted commit would need some work to make it compatible with HEAD.

comment:8 Changed 8 months ago by simonpj

Yes I think it should stay open. I've forgotten the context to be honest. I'll add it to my list! (But I don't expect to pay attention to it for some time, so others feel free to grab it.)

Simon

comment:9 Changed 7 months ago by thoughtpolice

  • Milestone changed from 7.10.1 to 7.12.1

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

Note: See TracTickets for help on using tickets.