Opened 9 years ago

Closed 5 years ago

#4463 closed bug (invalid)

CORE notes break optimisation

Reported by: rl Owned by:
Priority: low Milestone:
Component: Compiler Version: 7.1
Keywords: Cc: choener@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

I think at some point we decided that Core notes shouldn't affect optimisation. Here is a case where they do:

module Foo where

foo :: Int -> Int
{-# INLINE [1] foo #-}
foo x = x+1

{-# RULES "foo/foo" forall x. foo (foo x) = x #-}
module Bar where

import Foo

bar :: Int -> Int -> Int
bar x y = foo ({-# CORE "note" #-} x `seq` foo y)

When compiled with -O2, the rule doesn't fire with the note but does fire without it. This is the Core with the note:

Bar.bar =
  \ (x_aaw :: GHC.Types.Int) (y_aax :: GHC.Types.Int) ->
    Foo.foo
      (__core_note "note"
       (case x_aaw of _ { GHC.Types.I# _ -> Foo.foo y_aax }))

For the rule to fire, GHC must move the seq to the outside but because of the note, it doesn't.

Change History (6)

comment:1 Changed 9 years ago by simonpj

I didn't konw anyone was using core notes. Are you using them in DPH? Why? Depending on their semantics the above "move the seq" might or might not be correct.

comment:2 Changed 9 years ago by rl

I don't use them, a vector user ran into this. It's just that I think we said they shouldn't affect optimisation (in particular, we made the rule matcher look through them) so I wanted to record this.

comment:3 Changed 9 years ago by choenerzs

Cc: choener@… added

I am that user ;-)

They are very good for finding out if some piece of code is being correctly optimized -- if CORE notes don't interfere with optimization. Right now I just tag the outermost layer of code instead of smaller pieces, but they were very helpful in some cases.

comment:4 Changed 9 years ago by igloo

Milestone: 7.2.1

comment:5 Changed 9 years ago by simonpj

Milestone: 7.2.1_|_
Priority: normallow

I'm really not sure what to do here. If we move the seq to the outside, we'd presumably need to move the note, which in turn might break whatever the user had in mind for the semantics of his notes.

I propose to do nothing, but I'll leave the ticket open with a milestone of bottom, just as a vague reminder that we don't really know what notes do.

Simon

comment:6 Changed 5 years ago by thomie

difficulty: Unknown
Resolution: invalid
Status: newclosed

Support for Core Notes was added in 56b5a8b862d4eaeeaa941dd53e3d1009bdeadc0d, and seems to have gotten removed in 7bb0447df9a783c222c2a077e35e5013c7c68d91.

I might be mistaken, but I think this ticket is no longer relevant.

Note: See TracTickets for help on using tickets.