This might be buggy, since is_pure_clambda incorrectly considers Pdivint as pure while it can raise an exception (would need to check if the previous uses of is_pure_clambda are affected).

The bug is_pure_clambda is a real one. Currently, the following does not raise when compiled with ocamlopt (and it should):

type t = {x: int; y:int}
let f x = {x; y = x/0}.x
let () = ignore (f 1)

Fixed Closure.is_pure_clambda.

Wouldn't it be interesting to avoid catch-all patterns on instructions or primitive in this function? It would force us to consider each case and could uncover other similar bugs today, or tomorrow when a new instruction is added.

@gasche Agreed. Note that the dangerous catch-all is on primitives (they are assumed to be pure "by default"), not on expressions. But yes, being explicit would be a good idea in both cases.

Btw, array accesses look a bit fishy as well. At the level of Clambda, the checkbound is not yet explicit, and is_pure_clambda still assumes loads from array to be pure.

Indeed:

type t = {x: int; y:int}
let x = {x = 1; y = [||].(0)}.x

doesn't raise when compiled with ocamlopt.

Also suspicious: Plazyforce, Prevapply, Pdirapply, Pmodint, Pdivbint, Pmodbint, Pstring_load_*, Pbigstring_load_*.

On the other hand, I don't see why Pduprecord is considered non pure. Does anyone know?

Closure.is_pure is also suspicious...

Pduprecord, at least if it corresponds to {x with ...}, may either do a initialization of all fields (pure) or a memcopy followed by field-by-field updates. I suppose people had the latter implementation in mind when marking it impure -- you could argue that it only mutates uniquely-owned memory, but I am not sure how true that is (could GC effects share the value or something?).

Mmh, I don't see how the "memcopy + updates" version would break invariants expected from pure expressions (in this context, "pure" is used to avoid evaluating an expression when its result is not observed; not e.g. to change the evaluation order (which would require to mark all memory reads as also "impure").

Ok, I've committed a version with an exhaustive match on primitives. I did not try to take the "unsafe" arguments into account.

Can this code just use Semantics_of_primitives instead, which already has a carefully-checked exhaustive list (not that I promise it has no bugs)?

(We may need a flag to allow it to accept primitives that are illegal in Flambda mode, but that's easy.)

Yes, indeed! I'm a bit reluctant to have Closure depends on middle-end/. Can we merge this module into Lambda? One could imagine that either Simplif or alternative backend a-la Bucklescript use the information as well.

I think the middle-end dependency may be there already (for Debuginfo), so I'm not sure we should worry about that. If it really must be moved I'd rather it stayed as its own file and was moved properly with git to avoid introducing any merge conflicts with other branches in progress.

@mshinwell

Can you comment on:

  | Prevapply
  | Pdirapply
  | Psequand
  | Psequor ->
    Misc.fatal_errorf "The primitive %a should have been eliminated by the \
        [Closure_conversion] pass."
      Printlambda.primitive prim

?

Aren't Prevapply/Pdirapply instead eliminated by Simplif (also for the non-flambda pipeline)?

About Psequand/Pseqor: would you be ok to support them in Semantics_of_primitive (returning No_effects, No_coeffects) instead of failing as above?

I think the middle-end dependency may be there already (for Debuginfo), so I'm not sure we should worry about that.

The middle-end is not linked in the bytecode compiler, only in tools/objinfo.

Ok, implemented @mshinwell's suggestion of reusing Semantics_of_primitives (after moving it to bytecomp/). I removed error cases in this function, so that it can be applied in more contexts.

@alainfrisch Can you check that [Flambda_invariants] checks for absence of the primitives for which you have removed the error cases? (And make it check if it doesn't already.)

@mshinwell This is the case for Psequand, Pseqor, Pdirapply, Prevapply, but not Ploc. At least this is implemented in Flambda_invariants.primitive_invariants (but I cannot guarantee this function is applied).

Is it worth adding a case for Ploc?

Yeah, I think we might as well. The more checks the better...

Ok, done. (I'm not sure, but it seems Plazyforce is also expanded by translcore. And contrary to the error message in flambda_invariants, it seems that Prevapply/Pdirapply are handled by Simplif.)

Feel free to fix the message!

The message might be sign that flambda does some extra useless work; it would be good to check first.

@alainfrisch Please change the Flambda_invariants message for Pdirapply and Prevapply to reference Simplif instead of Closure_conversion; I think they are indeed handled there. I've made some other comments. OK after that.

@alainfrisch I'm not sure about the Ploc case actually. Shouldn't that "have effects"? It's semantics depends on it not being moved, I thought. (If we can't pin the semantics down properly it should maybe be a fatal error instead.)

Ploc evaluates to a compile-time constant, no? Which effect could it have, and why could it not be moved?

Well it seems to me that either Ploc has been compiled out, and if we only expect Semantics_of_primitives to be used on code after that compilation, we should have it produce a fatal error if it encounters this primitive.

However (reasonably enough) it seemed you were in favour of having Semantics_of_primitives do the right thing for any Lambda code, not just on that subset that we happen to generate at the moment, and in that world if Ploc exists I thought it would need to be treated as effectful to reflect the fact that if it were moved, it would produce a different result.

to reflect the fact that if it were moved, it would produce a different result.

I guess that by moving, you mean "changing its source location", while I was rather interpreting moving as changing its position in "the tree" (Parsetree, Typedtree, Lambda, etc).

Currently, Ploc is expanded during the Typedtree->Lambda translation, but it could easily be done later, since the Lprim node also keeps the source location. "Moving" Ploc within Lambda is fine as long as you don't change the Location.t attached to the Lprim node.

By moving I meant a code motion transformation. However from what you say it sounds like it's ok to do that anyway, as long as you don't change the location. I think this is fine then.