Primops: can_fail vs. has_side_effect

I thought that I should write a ticket about this while it's still fresh in my head after !2525 (closed).

Primops can be marked can_fail and/or has_side_effects. According to Note [PrimOp can_fail and has_side_effects] and Note [Transformations affected by can_fail and has_side_effects]

• can_fail applies to primops which may throw imprecise exceptions. In terms of transformations, we may never float these out. But we may still duplicate, drop or float them in.
• has_side_effects applies to primops which have a write effect (on memory or "the real world") or throw a precise exception (i.e. only raiseIO#). (Because of #3207 (closed), we also mark read side effects this way, but that is too conservative.) Compared to can_fail, we may not duplicate or drop these. (Only the former property seems relevant for read effects according to #3207 (closed), but I outline a design in #3207 (comment 257470) which allows duplication and floating out read-only effects.) For this reason, it is important that raiseIO# is not only can_fail, because we may not drop such a side-effect.

That made me thinking: There should be no need to ever set can_fail when has_side_effects is set, the latter implies the former. And in fact, the only way in which we use both is to define primOpOkForSideEffects/primopOkForSpeculation:

primOpOkForSpeculation :: PrimOp -> Bool
  -- See Note [PrimOp can_fail and has_side_effects]
  -- See comments with GHC.Core.Utils.exprOkForSpeculation
  -- primOpOkForSpeculation => primOpOkForSideEffects
primOpOkForSpeculation op
  =  primOpOkForSideEffects op
  && not (primOpOutOfLine op || primOpCanFail op)
    -- I think the "out of line" test is because out of line things can
    -- be expensive (eg sine, cosine), and so we may not want to speculate them

primOpOkForSideEffects :: PrimOp -> Bool
primOpOkForSideEffects op
  = not (primOpHasSideEffects op)

These are the only call sites of primOpHasSideEffects and primOpCanFail.

So why not refactor the whole can_fail/has_side_effects story to

data PrimOpTrait
  = Droppable
  -- ^ Drop the primop call.
  | Dupable
  -- ^ Duplicate the primop call.
  | Deferrable
  -- ^ Force other expressions before the primop call. Note that may entail
  -- dropping the call altogether when forcing diverges.
  | Speculatable
  -- ^ Force the primop call even if evaluation order would prescribe
  -- forcing something else first.

purePrimOpTraits, readEffectTraits, writeEffectTraits, raiseIOTraits :: [PrimOpTrait]
purePrimOpTraits  = [Droppable, Dupable, Deferrable, Speculatable]
readEffectTraits  = [Droppable,          Deferrable, Speculatable]
writeEffectTraits = [                    Deferrable] -- Although the current implementation does not defer side effects, so we are fine with raiseIO# treated as one
raiseIOTraits     = []

exprSatisfiesPrimOpTraits :: [PrimOpTrait] -> CoreExpr -> Bool

This has the advantage of compositionally describing the semantics of the different kinds of effects in terms of the clear semantics of PrimOpTraits and not having to repeat the individual traits over and over again in primops.txt.pp. Note how easy it is to define and change readEffectTraits compared to setting individual flags over and over again. Also now have a shot at defining readEffectTraits in the first place. Should there be some transformation in the future that would need another trait, that is as simple as changing the few lines I just gave.

What do you think? CC @simonpj

A fix to this ticket should also address the last two points of !2525 (comment 257945).