Opened 6 years ago

Closed 14 months ago

Last modified 14 months 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 Difficulty: Unknown
Test Case: typecheck/should_fail/T2354 Blocked By:
Blocking: Related Tickets:

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 15 months ago.
0001-2354-add-regression-test-driver-T2354.hs.patch (1.7 KB) - added by morabbin 15 months ago.
Testsuite patch to add regression test for #2354

Download all attachments as: .zip

Change History (22)

comment:1 Changed 6 years ago by simonpj

  • Difficulty set to 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 6 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 6 years ago by igloo

  • Milestone set to 6.10.1

comment:4 Changed 6 years ago by igloo

  • Milestone changed from 6.10.1 to 6.12 branch

comment:5 Changed 6 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:6 Changed 6 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

comment:7 Changed 4 years ago by igloo

  • Milestone changed from 6.12 branch to 6.12.3

comment:8 Changed 4 years ago by igloo

  • Milestone changed from 6.12.3 to 6.14.1
  • Priority changed from normal to low

comment:9 Changed 3 years ago by igloo

  • Milestone changed from 7.0.1 to 7.0.2

comment:10 Changed 3 years ago by igloo

  • Milestone changed from 7.0.2 to 7.2.1

comment:11 Changed 3 years ago by igloo

  • Milestone changed from 7.2.1 to 7.4.1

comment:12 Changed 2 years ago by igloo

  • Milestone changed from 7.4.1 to 7.6.1
  • Priority changed from low to lowest

comment:13 Changed 20 months ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

Changed 15 months ago by morabbin

comment:14 Changed 15 months ago by morabbin

  • Type of failure set to 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 15 months 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 15 months ago by morabbin

Sure; stay tuned.

Changed 15 months ago by morabbin

Testsuite patch to add regression test for #2354

comment:17 Changed 15 months ago by morabbin

  • Status changed from new to patch

Testsuite patch attached; please review.

comment:18 Changed 15 months ago by simonmar

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

comment:19 Changed 14 months ago by simonpj

  • Resolution set to fixed
  • Status changed from patch to closed

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

comment:20 Changed 14 months ago by simonpj

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