Opened 8 years ago

Closed 8 years ago

#5767 closed bug (fixed)

Integer inefficiencies

Reported by: rl Owned by: igloo
Priority: highest Milestone: 7.4.1
Component: Compiler Version: 7.5
Keywords: Cc: pho@…
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

Here is a small program:

module T where
foo :: RealFrac a => a -> a -> Int
{-# INLINE [0] foo #-}
foo x y = truncate $ (y-x)+2

module U where
import T
bar :: Int -> Int
bar x = foo 1 50 + x

GHC 7.2.2 generates this optimal code:

bar = \ (x_abs :: Int) -> case x_abs of _ { I# y_auX -> I# (+# 51 y_auX) }

Whereas the current HEAD generates this:

bar2 :: Integer
bar2 = __integer 2

bar1 :: Int
bar1 =
  case doubleFromInteger bar2
  of wild_arl { __DEFAULT -> I# (double2Int# (+## 49.0 wild_arl)) }

bar :: Int -> Int
bar = \ (x_a9S :: Int) -> plusInt bar1 x_a9S

If I remove the INLINE pragma from foo, the HEAD generates this:

bar1 :: Int
bar1 =
  case doubleFromInteger foo1
  of wild_asr { __DEFAULT ->
  case GHC.Float.$w$cproperFraction
         @ Int GHC.Real.$fIntegralInt (+## 49.0 wild_asr)
  of _ { (# ww1_as1, _ #) ->
  ww1_as1
  }
  }

bar :: Int -> Int
bar = \ (x_a9W :: Int) -> plusInt bar1 x_a9W

Interestingly, without the INLINE pragma 7.2.2 doesn't fare much better.

I've also seen this bit in the generated code with the HEAD but not with 7.2.2:

case integerToInt (smallInteger a_s2jL) of wild_a1dA { __DEFAULT -> f wild_a1dA }

I couldn't boil it down to a small test case yet but it leads to a significant performance regression in at least one vector benchmark. I suppose fixing this is only a matter of adding an integerToInt/smallInteger rule.

Change History (9)

comment:1 Changed 8 years ago by simonpj

difficulty: Unknown

I can see why this is. Stay tuned

comment:2 Changed 8 years ago by simonpj

Status: newmerge

Fixed by

commit 1074c2da93cc89cd183375ae414a18dc536a7b5d
Author: Simon Peyton Jones <simonpj@microsoft.com>
Date:   Fri Jan 13 15:46:56 2012 +0000

    Get the knownKeyNames for doubleFromInteger right
    
    There was a trivial typo which meant that important
    newly-added rules would never fire!

 compiler/prelude/PrelNames.lhs |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Ian, please merge; and perhaps add a test?

Simon

comment:3 Changed 8 years ago by igloo

Resolution: fixed
Status: mergeclosed

Good spot; merged as 102df6a7bc5657eef85f26d88ab6c071ec9b0b24 and I've added more cases covering this to integerConstantFolding.

comment:4 Changed 8 years ago by rl

Resolution: fixed
Status: closednew

I'm still seeing a regression compared to GHC 7.2.2 in this bit of Core:

case integerToInt (smallInteger a_s2jL) of wild_a1dA { __DEFAULT -> f wild_a1dA }

As I said, adding an integerToInt/smallInteger rule should help.

Note also that without the INLINE pragma on foo, both 7.2.2 and now the HEAD generate this code for my original example:

bar1 :: Int
bar1 =
  case GHC.Float.$w$cproperFraction @ Int GHC.Real.$fIntegralInt 51.0
  of _ { (# ww1_arU, _ #) -> ww1_arU }

bar :: Int -> Int
bar = \ (x_a9P :: Int) -> plusInt bar1 x_a9P

This isn't a regression but doesn't seem right.

comment:5 Changed 8 years ago by igloo

Milestone: 7.4.1
Owner: set to igloo
Priority: normalhighest

comment:6 Changed 8 years ago by rl

I finally found a small test case for the missing integerToInt/smallInteger rule:

foo :: (Integral a, Num a) => a -> a
{-# INLINE foo #-}
foo x = fromIntegral x

bar :: Int -> Int
bar x = foo x

The head generates this:

foo_$sfoo =
  \ (eta_B1 :: Int) ->
    case eta_B1 of _ { I# i_ara ->
    case integerToInt (smallInteger i_ara) of wild1_ard { __DEFAULT ->
    I# wild1_ard
    }
    }

bar = foo_$sfoo

Whereas 7.2.2 generates this:

bar_$sfoo = \ (eta_B1 :: Int) -> eta_B1

bar = bar_$sfoo

comment:7 Changed 8 years ago by simonpj

Note: the call to smallInteger arises from the fromIntegral method of Integral Int (in base:GHC.Real):

instance  Integral Int  where
    toInteger (I# i) = smallInteger i

So, yes we need a integerToInt (smallInteger n) RULE.

Ditto int64ToInteger and wordToInteger.

Also we'd like a RULE for (smallInteger (I# n), which should generate an Integer literal. This isn't easy right now for tiresome reasons. ToDo for 7.6. The most plausible route for doing it is to take mkIntegerLit out of Integer literals, and keep it somewhere global instead.

comment:8 Changed 8 years ago by PHO

Cc: pho@… added

comment:9 Changed 8 years ago by igloo

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.