Opened 10 years ago

Last modified 3 months ago

#932 new task

Improve inlining

Reported by: simonpj Owned by: simonpj
Priority: normal Milestone:
Component: Compiler Version: 6.4.2
Keywords: Cc: samb, simonpj
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case: N/A
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Currently the contruct

  case (x# ># 0#) of ...

attracts no argument discount for x#, which is silly.

The comment in CoreUnfold says:

	  PrimOpId op  -> primOpSize op (valArgCount args)
			  -- foldr addSize (primOpSize op) (map arg_discount args)
			  -- At one time I tried giving an arg-discount if a primop 
			  -- is applied to one of the function's arguments, but it's
			  -- not good.  At the moment, any unlifted-type arg gets a
			  -- 'True' for 'yes I'm evald', so we collect the discount even
			  -- if we know nothing about it.  And just having it in a primop
			  -- doesn't help at all if we don't know something more.

But the right thing to do seems to be to fix interestingArg in SimplUtils so that it only thinks a primitive-typed thing is interesting if it knows its value (or some structure).

Here's the program that triggered this thought:

import GHC.Word
import GHC.Base
import GHC.Prim

a `shiftRLT` b | b >=# 32# = int2Word# 0#
                | otherwise = a `uncheckedShiftRL#` b

(W32# x#) `shift` (I# i#) =
{- we do an actual case analysis on i# to try to give us a discount -}
   case i# of
    {- For some bizzare reason removing the `shiftRLT` 0# makes the
       inlining fail again -}
    0# -> W32# (x# `shiftRLT` 0#)
    _ -> 
	 if i# >=# 0# then W32# (narrow32Word# (x# `shiftL#` i#))
         else W32# (x# `shiftRLT` negateInt# i#)

x `shiftR` y = x `shift` (-y)

shift7 x = x `shiftR` 7

roconnor@… initiated the thread

Attachments (1)

inliner3.dpatch (223.0 KB) - added by samb 10 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by igloo

  • Milestone set to 6.6.1
  • Test Case set to N/A

comment:2 Changed 10 years ago by samb

  • Cc samb added

Unless I am quite mistaken, this will not help unless the function actually has an argument discount for that argument? I did not observe this for *any* of the shift workers for the types based on Word# or Int#. (Which is the point of the patches I've made so far...)

So I guess you'd need to do *both* things?

Of course, I could easily be quite mistaken ;-). There doesn't appear to be any commentary on the Trac wiki about inlining at all.

Changed 10 years ago by samb

comment:3 Changed 10 years ago by samb

Hmm, that did not work right! Trac won't let people download the attachment?

Anyway, with that last idea my patches no longer make anything in nofib allocate more -- however they also don't make any of them allocate *less* :-(. They do make a couple of 'em slower, though.

And then there are things like this:

module Test2 where

import GHC.Exts

foo = word2Int#

bar = foo

Which produces this output when built with ghc/compiler/ghc-inplace -ddump-inlinings -no-recomp -O2 -c Test2.hs:

SimplBind [main:Test2.foo{v r7V} [lid]]
SimplBind [main:Test2.bar{v r7X} [lid]]
Considering inlining
    main:Test2.foo{v r7V} [lid] active: False
				arg infos []
				interesting continuation False
				is value: True
				is cheap: True
				guidance IF_ARGS 0 [] 1 0
				ANSWER = NO
...

Which gives word2Int# a "size" of 1, even though it doesn't DO anything.

comment:4 Changed 10 years ago by samb

  • Cc samb simonpj added; samb removed

comment:5 Changed 9 years ago by simonmar

  • Milestone changed from 6.6.1 to 6.8
  • Owner set to simonpj

comment:6 Changed 9 years ago by guest

comment:7 Changed 8 years ago by simonpj

  • Milestone changed from 6.8 branch to _|_

There's probably something that can usefully be improved here, but let's wait until someone yells a bit more about it, with some program they care about. Sam?

Simon

comment:8 Changed 8 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:9 Changed 8 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

comment:10 Changed 3 months ago by thomie

  • Cc samb simonpj added; samb simonpj removed
  • Type of failure set to Runtime performance bug
Note: See TracTickets for help on using tickets.