Opened 2 years ago

Last modified 17 months ago

#14295 new bug

tagToEnum# leads to some silly closures

Reported by: dfeuer Owned by:
Priority: normal Milestone:
Component: Compiler Version: 8.2.1
Keywords: datacon-tags Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Runtime performance bug Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description (last modified by dfeuer)

I don't know how important this is in practice, but it looks unfortunate.

Suppose I write

foo :: (Bool -> a) -> Int# -> a
foo f x = f (tagToEnum# x)

Since tagToEnum# can fail, GHC compiles this to

foo
  = \ (@ a_a10v)
      (f_s1by [Occ=Once!] :: GHC.Types.Bool -> a_a10v)
      (x_s1bz [Occ=Once] :: GHC.Prim.Int#) ->
      let {
        sat_s1bA [Occ=Once] :: GHC.Types.Bool
        [LclId]
        sat_s1bA = GHC.Prim.tagToEnum# @ GHC.Types.Bool x_s1bz } in
      f_s1by sat_s1bA

That seems pretty bad! We know that tagToEnum# is applied to Bool, so we can transform this to something like

foo f x = case leWord# (intToWord# x) 1## of
            1# -> f $! tagToEnum# x
            _ -> f (error "tagToEnum# was used at Bool with tag ...")

which avoids an extra closure at the cost of a single Word# comparison. The same goes for arbitrary known enumeration types. I suspect the right place to fix this up is in CorePrep.

Change History (15)

comment:1 Changed 2 years ago by dfeuer

Description: modified (diff)

comment:2 Changed 2 years ago by dfeuer

Description: modified (diff)

comment:3 Changed 2 years ago by dfeuer

Specifically, any time CorePrep is tempted to produce

let x = tagToEnum# @KnownType y
in e

it should pause a moment and instead produce

case leWord# (int2Word# y) limit of
  DEFAULT -> error "blah"
  1# -> case tagToEnum# @KnownType y of x
          DEFAULT -> e

comment:4 Changed 2 years ago by dfeuer

Unsurprisingly, exactly the same thing is true for quotInt# and such.

comment:5 Changed 2 years ago by simonpj

I think we can do better. Add a BuiltInRule for tagToEnum#.

tagToEnum# Bool
===>
  \x -> case x of
    0# -> False
    1# -> True
    DEFAULT -> error "blah"

The rule can fire whenever tagToEnum# is applied to a known type, albeit perhaps one without too many constructors. It's a bit like inlining tagToEnum#. }}} The built-in rule mechanism works well; we can use it here too.

comment:6 Changed 2 years ago by simonpj

Unsurprisingly, exactly the same thing is true for quotInt# and such.

I don't understand. Can you explain?

comment:7 in reply to:  6 Changed 2 years ago by dfeuer

Replying to simonpj:

Unsurprisingly, exactly the same thing is true for quotInt# and such.

I don't understand. Can you explain?

foo :: (Int -> a) -> Int# -> Int# -> a
foo f x y = f (I# (quotInt# x y))

will suspend the division for fear of a zero denominator. We can fix that similarly by testing for 0 and forcing the division in the good branch.

comment:8 in reply to:  5 Changed 2 years ago by dfeuer

Replying to simonpj:

I think we can do better. Add a BuiltInRule for tagToEnum#.

tagToEnum# Bool
===>
  \x -> case x of
    0# -> False
    1# -> True
    DEFAULT -> error "blah"

The rule can fire whenever tagToEnum# is applied to a known type, albeit perhaps one without too many constructors. It's a bit like inlining tagToEnum#. }}} The built-in rule mechanism works well; we can use it here too.

Yes, I thought about that too. But it's probably really only a good idea if there aren't many constructors; otherwise we're inlining too much. When we don't inline, we should still perform a bounds check to avoid the thunk, no?

comment:9 Changed 2 years ago by simonpj

Really? I get

==================== STG syntax: ====================
Foo.foo
  :: forall a.
     (GHC.Types.Int -> a) -> GHC.Prim.Int# -> GHC.Prim.Int# -> a
[GblId, Arity=3, Caf=NoCafRefs, Unf=OtherCon []] =
    \r [f_s1av x_s1aw y_s1ax]
        let {
          sat_s1az [Occ=Once] :: GHC.Types.Int
          [LclId] =
              \u []
                  case quotInt# [x_s1aw y_s1ax] of wild_s1ay {
                    __DEFAULT -> GHC.Types.I# [wild_s1ay];
                  };
        } in  f_s1av sat_s1az;

comment:10 Changed 2 years ago by dfeuer

Am I reading that wrong? I thought the outer let was suspending that whole thing. Compare it to the modified version. I'm going to sleep shortly, do I can't post the comparison right now.

comment:11 Changed 2 years ago by simonpj

When we don't inline, we should still perform a bounds check to avoid the thunk, no?

Good point. I suppose so... but

  • If we did this we'd better give a different name to the primop that can't fail; otherwise the transformation is plain confusing.
  • I'm uncomfortable with transformations that are not readily expressed in Core. E.g. we could have
    case x of
      0# -> blah
      DEFAULT -> ...(noFailQuotInt# y x)...
    

since we know that x is non-zero in the default branch. But then we might float the subexpression out, and the claim would no longer hold. We don't have a solid way to prevent this.

So yes one could do it as a CorePrep tactic, but it's delicate; and we risk not getting the benefits because we don't implement the follow-on transformations.

  • I doubt it's worth doing in practice. i.e. the same engineering effort spent elsewhere might yield more benefit.

comment:12 in reply to:  11 Changed 2 years ago by dfeuer

Replying to simonpj:

So yes one could do it as a CorePrep tactic, but it's delicate; and we risk not getting the benefits because we don't implement the follow-on transformations.

What makes it delicate? For quotInt# et al, I don't imagine that there are many interesting follow-on transformations in most cases, although I could be wrong. For tagToEnum#, we should decide whether to "inline" in core2core. If we go to the trouble of a quotInt# cleanup in CorePrep, then we can just tack the tagToEnum# cleanup on in the same way.

comment:13 Changed 2 years ago by simonpj

What makes it delicate?

Just that CorePrep is trying to establish some tricky output invariants (about ANF) etc, and is not a simplification engine. If there are zero knock-on transformations then that's fine, but that's a delicate situation all by itself.

Anyway, I'm not opposed, just doubtful about the cost/benefit tradeoff.

comment:14 Changed 22 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:15 Changed 17 months ago by bgamari

Milestone: 8.6.1
Note: See TracTickets for help on using tickets.