Opened 2 years ago

Closed 2 years ago

#9732 closed bug (fixed)

Pattern synonyms and unboxed values

Reported by: monoidal Owned by: cactus
Priority: normal Milestone: 7.10.1
Component: Compiler Version: 7.8.3
Keywords: PatternSynonyms Cc: cactus
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Compile-time crash Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description (last modified by monoidal)

It's possible to declare a toplevel unboxed value with a pattern synonym, which causes a panic:

{-# LANGUAGE PatternSynonyms, MagicHash #-}
pattern P = 0#

(compare with error on x = 0#). pattern P <- 0# seems to work fine.

Change History (24)

comment:1 Changed 2 years ago by monoidal

Description: modified (diff)

comment:2 Changed 2 years ago by simonpj

Owner: set to cactus

comment:3 Changed 2 years ago by cactus

pattern P <- 0# doesn't completely work either:

{-# LANGUAGE PatternSynonyms, MagicHash #-}
pattern PAT <- 0#

f PAT = 42#
g 0# = 42#

This results in an f function that always fails the pattern match:

Main.f :: GHC.Prim.Int# -> GHC.Prim.Int#
[GblId, Arity=1, Str=DmdType]
Main.f =
  \ _ [Occ=Dead] ->
      @ GHC.Prim.Int# "T9732.hs:6:1-11|function f"#

Contrast this with

Main.g :: GHC.Prim.Int# -> GHC.Prim.Int#
[GblId, Arity=1, Str=DmdType]
Main.g =
  \ (ds_dL5 :: GHC.Prim.Int#) ->
    case ds_dL5 of _ [Occ=Dead] {
      __DEFAULT ->
          @ GHC.Prim.Int# "T9732.hs:7:1-10|function g"#;
      0 -> break<0>() 42

Interestingly, if f returns a lifted Int, it all works out as expected:

f PAT = (42 :: Int)

results in

Main.f :: GHC.Prim.Int# -> GHC.Types.Int
[GblId, Arity=1, Str=DmdType]
Main.f =
  \ (ds_dLU :: GHC.Prim.Int#) ->
    let {
      cont_aLE :: GHC.Types.Int
      [LclId, Str=DmdType]
      cont_aLE = break<3>() GHC.Types.I# 42 } in
    let {
      fail_aLF :: GHC.Types.Int
      [LclId, Str=DmdType]
      fail_aLF =
          @ GHC.Types.Int "T9732.hs:6:1-19|function f"# } in
    case ds_dLU of _ [Occ=Dead] {
      __DEFAULT -> fail_aLF;
      0 -> cont_aLE

Is it a good idea to just disallow pattern synonyms of unlifted types? Or should pattern P <- 0# work at least?

comment:4 Changed 2 years ago by goldfire

I would think that pattern synonyms for unlifted types should work, but with the same restrictions as normal patterns for unlifted types. That is, you can't use these patterns to define a global variable.

comment:5 Changed 2 years ago by simonpj

Right. If it works when the pattern synonym is expanded, it should work un-expanded


comment:6 Changed 2 years ago by cactus

The problem with pattern P = 0# is that the wrapper it yields is $WP = 0# which is a top-level binding at an unboxed type. If P had any arguments, then $WP would be a function instead of a variable, and so it would be valid. So we could say that unboxed pattern synonyms are only allowed if they are either unidirectional or have arguments. But doesn't that sound a bit arbitrary?

Side question: is it even possible to change the definition of P to have an argument but still be unboxed? Something like pattern P x = (# 0#, x #) doesn't work, because unboxed tuples are not allowed in patterns.

comment:7 Changed 2 years ago by simonpj

Look at WwLib.mkWorkerArgs which deals with much the same issue. We just add a void argument to the wrapper avoid the top-level unboxed binding. I think we can do the same thing for pattern synonyms, no?


comment:8 Changed 2 years ago by cactus

Hmm. mkWorkerArgs looks like quite the rabbit hole... does it make sense to figure out the plumbing for that, or is it good enough to just steal the basic idea and implement something similar for pattern synonyms that ensures there's always at least one argument (a synthetic () if need be) to pattern synonym wrapper functions returning unboxed values?

comment:9 Changed 2 years ago by cactus

As for the unidirectional case, I was accidentally looking at the simplified output instead of just the desugared one. So with this code:

{-# LANGUAGE PatternSynonyms, MagicHash #-}
pattern PAT <- 0#

f PAT = 42#

the Core generated for f is:

Main.f :: GHC.Prim.Int# -> GHC.Prim.Int#
[LclIdX, Str=DmdType]
Main.f =
  \ (ds_dpR :: GHC.Prim.Int#) ->
    case (\ _ [Occ=Dead, OS=OneShot] ->
              @ GHC.Prim.Int# "T9732.hs:7:1-11|function f"#)
    of wild_00 { __DEFAULT ->
    (case break<1>() 42 of wild_X4 { __DEFAULT ->
     Main.$mPAT @ GHC.Prim.Int# ds_dpR wild_X4

which is close enough: it would be correct if only wild_00 wasn't floated out...

comment:10 Changed 2 years ago by cactus

Never mind, I've figured out that last bit -- it's because in this case, $mPAT is used at type Int# -> Int# -> Int# -> Int#, so it's always going to be strict in both the cont and the fail arguments.

I'll change the code generator for matchers so that both cont and fail takes an extra () argument. Another problem, then, will be making the matcher polymorphic enough that its r type argument can be either * or #...

So all in all, in this example, currently:

$mPAT :: forall (r :: *). Int# -> r -> r -> r

but it should be

$mPAT :: forall (r :: ?). Int# -> (() -> r) -> (() -> r) -> r

Does that sound reasonable?

comment:11 Changed 2 years ago by cactus

(and of course if $mPAT :: forall (r :: ?) is not kosher, we can just have two matchers, one $mPAT :: forall (r :: *) and one $mPAT# :: (forall r :: #), and just have the extra () arguments for cont/fail for the latter)

comment:12 Changed 2 years ago by simonpj

  • cont and fail should take an extra arg if (but only if) they have no other value args, and the result type is unlifted. This is easy to test.
  • Don't give them an extra arg of (). Rather use voidPrimId. This is zero bits wide, and hence takes no instructions to pass. Just like mkWorkerArgs.
  • Yes, it's fine to give the r variable an OpenTypeKind. Similar to the errorId definitions in MkId.



comment:13 Changed 2 years ago by cactus

Yes, I've found voidPrimId/voidArgId meanwhile, and I have a working implementation pushed to wip/T9732 just now.

However, I had to split the matcher into two, since otherwise I can't decide when to have the extra arg, since the result type is completely unknown at matcher generation time. So for now, I've went with

Main.$mPAT :: forall r. GHC.Prim.Int# -> r -> r -> r
Main.$m#PAT :: forall (r :: #). GHC.Prim.Int# -> (GHC.Prim.Void# -> r) -> (GHC.Prim.Void# -> r) -> r

so that takes care of using pattern synonyms when the _result_ type is unlifted: both of these now compile lint-free, and give the expected behaviour:

f PAT = 42#
g PAT = (42 :: Int)

So the only remaining TODO should be to add this extra Void# argument to the generated wrappers when there are no other arguments and the pattern type is unlifted.

comment:14 Changed 2 years ago by simonpj

Yes, I see. You don't know if the result is unlifted, but you DO know if cont has value args. (Which it does not in this case.)

I think you can just have the second of your two matchers, with forall (r :: ?). ..., something that is not possible in source Haskell, but which you can do in generated code I think.

comment:15 Changed 2 years ago by cactus

A single open-kinded matcher works, yes, if we are OK with argument-free conts and all fails taking the extra Void# argument. I will redo my implementation for this design.

comment:16 Changed 2 years ago by cactus

Status: newpatch

Implementation based on the discussion above is now pushed to wip/T9732. Also, now, patsyns in expression contexts are sometimes compiled into calls with a dummy Void# argument.

I think it's good enough to go into master but I'd prefer if Simon took a look at it first.

Last edited 2 years ago by cactus (previous) (diff)

comment:17 Changed 2 years ago by cactus

Keywords: pattern synonyms added

comment:18 Changed 2 years ago by Dr. ERDI Gergo <gergo@…>

In 7f929862388afd54043d59b37f2f5375c5315344/ghc:

If pattern synonym is bidirectional and its type is some unboxed type T#,
generate a worker function of type Void# -> T#, and redirect the wrapper
(via a compulsory unfolding) to the worker. Fixes #9732.

comment:19 Changed 2 years ago by cactus

Milestone: 7.10.1
Status: patchmerge

comment:20 Changed 2 years ago by cactus


Note that 7f929862..638991 all need to be cherry-picked to the 7.8 branch for this to work.

comment:21 Changed 2 years ago by cactus

Keywords: PatternSynonyms added; pattern synonyms removed

comment:22 Changed 2 years ago by thoughtpolice

Resolution: fixed
Status: mergeclosed

Gergo, I couldn't cleanly apply this it seems even with those patches; so I'm afraid I'll be dropping this for 7.8.4.

comment:23 Changed 2 years ago by cactus

Status: closedmerge

I have pushed a new version of the commit that applies cleanly to ghc-7.8 as a91a2af..0f1f3e1, please merge that.

Also, in the future, please try applying the patches in question at an earlier time, instead of discovering if they have problems so close to the freeze date.

comment:24 Changed 2 years ago by thoughtpolice

Status: mergeclosed

7.8.4 is already done; closing.

Note: See TracTickets for help on using tickets.