Opened 9 years ago

Closed 4 years ago

Last modified 4 years ago

#2354 closed bug (fixed)

NOINLINE pragma ignored

Reported by: guest Owned by:
Priority: lowest Milestone: 7.6.2
Component: Compiler Version: 6.9
Keywords: Cc: lennart@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: typecheck/should_fail/T2354
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Compile the following program

{-# OPTIONS_GHC -O -ddump-simpl #-}
module F(test) where

class AsInt a where
  {-# NOINLINE toInt #-}
  toInt   :: a -> Int
  {-# NOINLINE fromInt #-}
  fromInt :: Int -> a

{-# INLINE onInt #-}
onInt :: AsInt a => (Int -> Int) -> (a -> a)
onInt f x = fromInt (f (toInt x))

test :: AsInt a => (Int -> Int) -> (Int -> Int) -> (a -> a)
test h g = onInt h . onInt g

Look at the final output for test:

F.test =
  \ (@ a_a6c)
    ($dAsInt_a6m :: F.AsInt a_a6c)
    (eta_s6B :: GHC.Base.Int -> GHC.Base.Int)
    (eta1_s6C :: GHC.Base.Int -> GHC.Base.Int)
    (eta2_s6D :: a_a6c) ->
    case $dAsInt_a6m of tpl_B1 { F.:DAsInt tpl1_B2 tpl2_B3 ->
    tpl2_B3 (eta_s6B (tpl1_B2 (tpl2_B3 (eta1_s6C (tpl1_B2 eta2_s6D)))))

The method (selectors) toInt and fromInt have been inlined despite the NOINLINE pragma. The compiler should either obey the pragma, or tell me that I can't have it in that place.

Attachments (2)

T2354.hs (366 bytes) - added by morabbin 4 years ago.
0001-2354-add-regression-test-driver-T2354.hs.patch (1.7 KB) - added by morabbin 4 years ago.
Testsuite patch to add regression test for #2354

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by simonpj

difficulty: Unknown

Yes, I agree this is surprising, and arguably wrong. However, a difficulty is that it's not clear what this might mean:

class AsInt a where
  {-# INLINE toInt #-}
  toInt   :: a -> Int
  toInt x = <blah blah>

Now, does the INLINE pragma apply to the method selector (which picks the method from the dictionary record) or to the default methodn (used in instance decls when no specific method is supplied)? Or both, perhaps? I think GHC assumes the latter at the moment.

Simon

comment:2 Changed 9 years ago by guest

Good point. So there should be some way to ask for the method selector not to be inlined. And when there is no default method, then asking for it not to be inlined should probably be an error.

comment:3 Changed 9 years ago by igloo

Milestone: 6.10.1

comment:4 Changed 8 years ago by igloo

Milestone: 6.10.16.12 branch

comment:5 Changed 8 years ago by simonmar

Architecture: UnknownUnknown/Multiple

comment:6 Changed 8 years ago by simonmar

Operating System: UnknownUnknown/Multiple

comment:7 Changed 7 years ago by igloo

Milestone: 6.12 branch6.12.3

comment:8 Changed 7 years ago by igloo

Milestone: 6.12.36.14.1
Priority: normallow

comment:9 Changed 6 years ago by igloo

Milestone: 7.0.17.0.2

comment:10 Changed 6 years ago by igloo

Milestone: 7.0.27.2.1

comment:11 Changed 5 years ago by igloo

Milestone: 7.2.17.4.1

comment:12 Changed 5 years ago by igloo

Milestone: 7.4.17.6.1
Priority: lowlowest

comment:13 Changed 4 years ago by igloo

Milestone: 7.6.17.6.2

Changed 4 years ago by morabbin

Attachment: T2354.hs added

comment:14 Changed 4 years ago by morabbin

Type of failure: None/Unknown

Current behavior disallows the pragmas:

Orac:~/work/tickets $ ghc -c T2354.hs 

T2354.hs:5:3:
    The INLINE pragma for default method `toInt' lacks an accompanying binding

T2354.hs:7:3:
    The INLINE pragma for default method `fromInt' lacks an accompanying binding

The errors refer to the NOINLINE pragma lines, and seems to be in line with the last comment.

comment:15 Changed 4 years ago by simonmar

So it's now correct. Would you like to have a go at making a testsuite patch to add the test?

comment:16 Changed 4 years ago by morabbin

Sure; stay tuned.

Changed 4 years ago by morabbin

Testsuite patch to add regression test for #2354

comment:17 Changed 4 years ago by morabbin

Status: newpatch

Testsuite patch attached; please review.

comment:18 Changed 4 years ago by simonmar

Fine, except that "driver" is not the right place - perhaps simplCore?

comment:19 Changed 4 years ago by simonpj

Resolution: fixed
Status: patchclosed

I put it in typecheck in the end; simplCore doesn't have a should_fail group.

comment:20 Changed 4 years ago by simonpj

Test Case: typecheck/should_fail/T2354
Note: See TracTickets for help on using tickets.