Opened 7 years ago

Closed 2 years ago

Last modified 2 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 Revisions:

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

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 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 7 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 7 years ago by igloo

  • Milestone set to 6.10.1

comment:4 Changed 7 years ago by igloo

  • Milestone changed from 6.10.1 to 6.12 branch

comment:5 Changed 7 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:6 Changed 7 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

comment:7 Changed 5 years ago by igloo

  • Milestone changed from 6.12 branch to 6.12.3

comment:8 Changed 5 years ago by igloo

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

comment:9 Changed 5 years ago by igloo

  • Milestone changed from 7.0.1 to 7.0.2

comment:10 Changed 4 years ago by igloo

  • Milestone changed from 7.0.2 to 7.2.1

comment:11 Changed 4 years ago by igloo

  • Milestone changed from 7.2.1 to 7.4.1

comment:12 Changed 3 years ago by igloo

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

comment:13 Changed 3 years ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

Changed 3 years ago by morabbin

comment:14 Changed 3 years 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 3 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 3 years ago by morabbin

Sure; stay tuned.

Changed 3 years ago by morabbin

Testsuite patch to add regression test for #2354

comment:17 Changed 3 years ago by morabbin

  • Status changed from new to patch

Testsuite patch attached; please review.

comment:18 Changed 3 years ago by simonmar

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

comment:19 Changed 2 years 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 2 years ago by simonpj

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