Improves the modes of mixed recursive definitions

[riaqn]

This PR improves the modes of mixed (not entirely functions) recursive definitions.

When a group of recursive definitions are all functions, they are allocated as a single block, and this block can be any mode. All functions will have that mode.

When the group is not entirely functions, they are allocated separately. If they are genuiely mutually recursive, they will contain backward pointers (old pointing to new) that our GC currently cannot handle. To prevent that, they will all be allocated on heap.

This PR does two things:

• Each entry in the mixed definitions has its own mode.
• Currently, axes other than locality are set to legacy as well. This PR removes this restriction.

[lthls]

This looks dangerous to me. What about the following piece of code (where force_local and force_global ensure that their argument have the corresponding locality):

let rec f () = f ()
and g () = g ()
and h = ()
in
force_global f;
force_local g;
()

This shouldn't be allowed to compile, as f and g will be part of the same closure block. But if I understand correctly this PR, it could allow it.

[lthls]

Sorry, for the locality it should error out correctly as this PR still forces the locality to be global. I'm still worried about the other modes, as f and g will still end up allocated together.

[riaqn]

@lthls Oh my impression got from @lpw25 is that if this is a mixed definition, they will be allocated separately - I will let Leo clarify.

[lthls]

It used to be the case, but since 5.2 upstream and #2394 here the pre-allocate-and-backpatch scheme is only used for regular blocks, with functions always compiled to regular closures (as a single block if there are several of them).

[riaqn]

@lthls Could you elaborate it to the degree that I can fix this PR? For example, do we still specially treat entire-functions defintions? How are they allocated in each cases?

[lthls]

In a given set of recursive definitions, we sort them into four categories:

• Non-recursive definitions (more precisely, definitions that cannot be compiled using any other scheme but were allowed since they aren't actually recursive). These are compiled as regular let-bindings, inserted before the rest.
• Constant definitions. These are definitions that have been accepted, but cannot be pre-allocated (think let rec x = let _ = x in 0). These are compiled to regular bindings too, modulo some renaming to remove references to recursive variables.
• Blocks of static size. These are pre-allocated, new values are computed from the definitions, and the contents of the new values is copied back to the original allocation (this is the legacy scheme, that was used for closures too)
• Functional values. These are defined together in a single recursive block full of functions. The definition occurs after the pre-allocation (so the functions can refer to the recursive blocks) but before the back-patching (so the block definitions can refer to the functions).

There is a special case for recursive functions that are not syntactic functions:

let rec f =
  let r = ref 0 in
  fun () -> incr r; !r (* actual code would also use [f] *)

These definitions are transformed into a pseudo-closure allocation and a syntactic function:

let rec f () = incr f_env.r; !(f_env.r)
and f_env =
  let r = ref 0 in
  { r }

The syntactic function then becomes part of the set of recursive closures, while the pseudo-closure gets pre-allocated and back-patched.

For this PR, ideally the classification that is currently done in Value_rec_compiler should be done during Value_rec_check instead, which would allow you to easily understand in which case all of the variables fall, but without that I think that it's simpler to assume that everything that is declared Static by Value_rec_check may end up as a shared closure.
Since shared closures are the main issue for you, you could also write a simple patch to Value_rec_check and Value_rec_types that distinguishes functions from the other static definitions.
Note that I don't know if the code changed by this PR is run before or after Value_rec_check.is_valid_recursive_expression; if it needs to be run before, then things become much more complicated.

[riaqn]

@lthls thank you for the detailed explanation.

What is shared closure?

if the code changed by this PR is run before or after

Currently it runs before. I'm not sure if it can be made otherwise.

[lthls]

I call a shared closure a closure block that defines several functions (i.e. let rec f () = g () and g () = f () will produce a single closure block that defines both f and g).

[lpw25]

This PR does two things:

• Each entry in the mixed definitions has its own mode.
• Currently, axes other than locality are set to legacy as well. This PR removes this restriction.

I think that all the complication in this discussion is caused by the first point, but that the user reported issue you are trying to fix is caused by the second one. Perhaps it would be worth fixing them separately?

[riaqn]

Yeah sounds good - this PR now contains only the second fix.

We will do the first fix in a future PR, as it sounds complex.

[lpw25]

LGTM