Haskell › Haskell - Glasgow-haskell-bugs

[GHC] #7041: GHC.Real.gcdInt is no longer optimized.

Classic

List

Threaded

8 messages Options

GHC

[GHC] #7041: GHC.Real.gcdInt is no longer optimized.

#7041: GHC.Real.gcdInt is no longer optimized.
------------------------------+---------------------------------------------
Reporter: int-e | Owner:
Type: bug | Status: new
Priority: normal | Component: libraries/base
Version: 7.5 | Keywords:
Os: Unknown/Multiple | Architecture: Unknown/Multiple
Failure: None/Unknown | Testcase:
Blockedby: | Blocking:
Related: |
------------------------------+---------------------------------------------
This is a regression since ghc-7.2, related to #5767. {{{GHC.Real}}}
defines

{{{
gcdInt :: Int -> Int -> Int
gcdInt a b = fromIntegral (gcdInteger (fromIntegral a) (fromIntegral b))
}}}

which used to optimize to {{{GHC.Integer.Type.gcdInt}}}. But since
{{{fromInteger = integerToInt}}} and {{{fromIntegral = smallInteger}}} are
no longer inlined, this optimization is lost. This results in unecessary
allocations.

--
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7041>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler

_______________________________________________
Glasgow-haskell-bugs mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/glasgow-haskell-bugs

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

#7041: GHC.Real.gcdInt is no longer optimized.
---------------------------------+------------------------------------------
Reporter: int-e | Owner: igloo
Type: bug | Status: new
Priority: high | Milestone: 7.6.1
Component: libraries/base | Version: 7.5
Keywords: | Os: Unknown/Multiple
Architecture: Unknown/Multiple | Failure: None/Unknown
Difficulty: Unknown | Testcase:
Blockedby: | Blocking:
Related: |
---------------------------------+------------------------------------------
Changes (by simonmar):

* owner: => igloo
* difficulty: => Unknown
* priority: normal => high
* milestone: => 7.6.1

Comment:

`GHC.Integer.Type` has this RULE:

{{{
{-# RULES "gcdInteger/Int" forall a b.
gcdInteger (S# a) (S# b) = S# (gcdInt a b)
#-}
}}}

but I imagine it doesn't fire any more. I guess this is one more to add
to `PrelRules`, Ian?

--
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7041#comment:1>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler

_______________________________________________
Glasgow-haskell-bugs mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/glasgow-haskell-bugs

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

In reply to this post by GHC

#7041: GHC.Real.gcdInt is no longer optimized.
---------------------------------+------------------------------------------
Reporter: int-e | Owner: igloo
Type: bug | Status: new
Priority: high | Milestone: 7.6.1
Component: libraries/base | Version: 7.5
Keywords: | Os: Unknown/Multiple
Architecture: Unknown/Multiple | Failure: None/Unknown
Difficulty: Unknown | Testcase:
Blockedby: | Blocking:
Related: |
---------------------------------+------------------------------------------

Comment(by simonpj):

Ian, or Paolo are you on this? In general there probably should be NO
rules matching on S#; worth checking.

Simon

--
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7041#comment:2>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler

_______________________________________________
Glasgow-haskell-bugs mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/glasgow-haskell-bugs

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

In reply to this post by GHC

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

In reply to this post by GHC

#7041: GHC.Real.gcdInt is no longer optimized.
-------------------------------+--------------------------------------------
Reporter: int-e | Owner: igloo
Type: bug | Status: closed
Priority: high | Milestone: 7.6.1
Component: libraries/base | Version: 7.5
Resolution: fixed | Keywords:
Os: Unknown/Multiple | Architecture: Unknown/Multiple
Failure: None/Unknown | Difficulty: Unknown
Testcase: | Blockedby:
Blocking: | Related:
-------------------------------+--------------------------------------------

Comment(by simonpj):

Here's are the commits for this ticket
{{{
commit a07f118c08ac5fd5923b6e0b2c994f4185121604
Author: Ian Lynagh <[hidden email]>
Date: Fri Jul 13 19:51:08 2012 +0100

Tweak RULEs; fixes #7041

In particular, the gcd rule now uses smallInteger rather than S#,
which
means that it actually fires.

Also fixed a bug when the result is minBound :: Int.

>---------------------------------------------------------------

GHC/Integer/Type.lhs | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/GHC/Integer/Type.lhs b/GHC/Integer/Type.lhs index
be3ebea..a402f37 100644
--- a/GHC/Integer/Type.lhs
+++ b/GHC/Integer/Type.lhs
@@ -135,11 +135,6 @@ int64ToInteger i = if ((i `leInt64#` intToInt64#
0x7FFFFFFF#) &&

integerToInt :: Integer -> Int#
{-# NOINLINE integerToInt #-}
-{-# RULES "integerToInt" forall i. integerToInt (S# i) = i #-}
--- Don't inline integerToInt, because it can't do much unless
--- it sees a (S# i), and inlining just creates fruitless
--- join points. But we do need a RULE to get the constants
--- to work right: 1::Int had better optimise to (I# 1)!
integerToInt (S# i) = i
integerToInt (J# s d) = integer2Int# s d

@@ -287,8 +282,10 @@ lcmInteger a b = if a `eqInteger` S# 0# then S#
0#
where aa = absInteger a
ab = absInteger b

+-- This rule needs to use absInteger so that it works correctly when
+-- the result is minBound :: Int
{-# RULES "gcdInteger/Int" forall a b.
- gcdInteger (S# a) (S# b) = S# (gcdInt a b)
+ gcdInteger (smallInteger a) (smallInteger b) = absInteger
+ (smallInteger (gcdInt a b))
#-}
gcdInt :: Int# -> Int# -> Int#
gcdInt 0# y = absInt y
}}}
and
{{{
commit 4b780b93beac863b7945d7e050ffa6cdaf256eb7
Author: Ian Lynagh <[hidden email]>
Date: Fri Jul 13 20:04:51 2012 +0100

Add another gcdInteger rule

This one is better when the result is converted to an Int

>---------------------------------------------------------------

GHC/Integer/Type.lhs | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/GHC/Integer/Type.lhs b/GHC/Integer/Type.lhs index
a402f37..b9d9c3c 100644
--- a/GHC/Integer/Type.lhs
+++ b/GHC/Integer/Type.lhs
@@ -283,9 +283,14 @@ lcmInteger a b = if a `eqInteger` S# 0# then S#
0#
ab = absInteger b

-- This rule needs to use absInteger so that it works correctly when
--- the result is minBound :: Int
-{-# RULES "gcdInteger/Int" forall a b.
- gcdInteger (smallInteger a) (smallInteger b) = absInteger
(smallInteger (gcdInt a b))
+-- the result is minBound :: Int. But that isn't necessary when the
+-- result is converted to an Int.
+{-# RULES
+"gcdInteger/Int" forall a b.
+ gcdInteger (smallInteger a) (smallInteger b)
+ = absInteger (smallInteger (gcdInt a b))
+"integerToInt/gcdInteger/Int" forall a b.
+ integerToInt (gcdInteger (smallInteger a) (smallInteger b)) =
+gcdInt a b
#-}
gcdInt :: Int# -> Int# -> Int#
gcdInt 0# y = absInt y
}}}

--
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7041#comment:4>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler

_______________________________________________
Glasgow-haskell-bugs mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/glasgow-haskell-bugs

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

In reply to this post by GHC

#7041: GHC.Real.gcdInt is no longer optimized.
-------------------------------+--------------------------------------------
Reporter: int-e | Owner:
Type: bug | Status: new
Priority: high | Milestone: 7.6.1
Component: libraries/base | Version: 7.5
Resolution: | Keywords:
Os: Unknown/Multiple | Architecture: Unknown/Multiple
Failure: None/Unknown | Difficulty: Unknown
Testcase: | Blockedby:
Blocking: | Related:
-------------------------------+--------------------------------------------
Changes (by simonpj):

* owner: igloo =>
* status: closed => new
* resolution: fixed =>

Comment:

1. There is no mention of `smallInteger` in the wiki page.
[http://hackage.haskell.org/trac/ghc/wiki/Commentary/Libraries/Integer]
It took me a while, but I got there, and augmented the page. Can you
check I’m right?

2. There are ''some'' rules in `PrelRules` with a (correct) comment saying
{{{
-- These rules below don't actually have to be built in, but if we
-- put them in the Haskell source then we'd have to duplicate them
-- between all Integer implementations
}}}
But then in `integer-gmp:GHC/Integer/Type.hs` we have other RULES that
also should be duplicated in other integer packages (but aren’t). One of
these is the gcd rule altered in this patch. Why did you decide to do one
thing for some rules and another for the others?

3. Also what is special about GCD here? What about
{{{
integerToInt (plusInteger (smallInteger a) (smallInteger b))
}}}
Maybe it’s just that the result of gcd is guaranteed to be small? Even in
the case, presumably integerToInt is undefined if the result doesn’t fit
in an Int, so we could indeed say that
{{{
integerToInt (plusInteger (smallInteger a) (smallInteger b)) = plusInt
a b
}}}
But is this common enough to worry about? It seems most odd to have a
RULE like this for GCD but not for +!!

4. Coming back to this ticket #7041 it seems utterly bonkers that in
`base:GHC.Real` we have
{{{
gcdInt :: Int -> Int -> Int
gcdInt a b = fromIntegral (gcdInteger (fromIntegral a) (fromIntegral b))
}}}
This is then laboriously converted (by the new RULE ) to, guess what, a
call to
`ghc-integer:GHC.Integer.Type.gcdInt`, which is defined (in that module)
{{{
gcdInt :: Int# -> Int# -> Int#
gcdInt 0# y = absInt y
gcdInt x 0# = absInt x
gcdInt x y = gcdInt# (absInt x) (absInt y)
}}}
This is madness. Why don’t we define `gcdInt` in that way in
`base:GHC.Real`, and get rid of the bizarre special treatment of GCD.
Which we only needed because its definition in `GHC.Real` stupidly went
via Integer.

Simon

--
Ticket URL: <http://hackage.haskell.org/trac/ghc/ticket/7041#comment:5>
GHC <http://www.haskell.org/ghc/>
The Glasgow Haskell Compiler

_______________________________________________
Glasgow-haskell-bugs mailing list
[hidden email]
http://www.haskell.org/mailman/listinfo/glasgow-haskell-bugs

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

In reply to this post by GHC

GHC

Re: [GHC] #7041: GHC.Real.gcdInt is no longer optimized.

In reply to this post by GHC