Skip to content

New strategy for deletion of begin/end region primitives#1847

Closed
mshinwell wants to merge 3 commits intooxcaml:mainfrom
mshinwell:flambda2-stop-apply-expr-needing-region-all-the-time-option-b
Closed

New strategy for deletion of begin/end region primitives#1847
mshinwell wants to merge 3 commits intooxcaml:mainfrom
mshinwell:flambda2-stop-apply-expr-needing-region-all-the-time-option-b

Conversation

@mshinwell
Copy link
Copy Markdown
Collaborator

We currently have a problem in flambda2 with unnecessary begin/end region operations being emitted. The specific cases I'll consider here are those where application expressions are holding onto region variables, even though they are not necessary.

There are some examples below. The last two functions shouldn't contain any begin/end region operations in the Cmm output, but they do at present. Indeed g_exn has two pairs of them.

The starting point for the fix is the observation that, if an Apply_expr can be turned into a direct call, we can examine the contains_no_escaping_local_allocs flag on the code metadata of the callee. If this flag is true, then I believe it is the case that the function does not need the region argument, irrespective of the return mode on the application. Likewise, noalloc C calls do not need the region argument.

We could just drop the region from the application expression, e.g. by making it optional, in these cases. However it is possible that, during a subsequent round of simplification, precision might regress and the application could be demoted to an indirect call. It seems as if this should never change the setting of contains_no_escaping_local_allocs for the callee, but this is not entirely obvious, so this patch takes a different approach.

Another possibility might be to track "weak region variables", i.e. ones which occur in applications where the region is redundant (as per the criteria above). However this is difficult to implement nicely in Name_occurrences because the set of variables would overlap with the existing names sets there.

Instead this patch takes a different approach. It removes the old method by which Begin_region, Begin_try_region and End_region were deleted when redundant, although the place in the dataflow analysis that gives a special treatment to End_region is still present. Instead of deleting when redundant, the primitives are now marked that they are redundant, and To_cmm is permitted to delete them. They are never deleted in Simplify for the above reason.

It might seem as if Begin_region and Begin_try_region should not need to have this marker because of the number-of-occurrences-of-bound-variables indication on let-expressions. However they are in fact annotated as to whether they are definitely unused, because otherwise unused Begin_try_region primitives could falsely hold Begin_region primitives alive.

With this new strategy, we can now change the free names computations in Apply_expr to simply conceal the region variable in the cases where it is not necessary. Yet, if later a direct call regresses to an indirect one, such variable is still there and the corresponding begin/end region primitives should automatically update to say that they are now required.

The last two examples below now contain no begin/end region operations in the Cmm code.

I am hoping it is now the case that we could enforce the following: if a function says contains_no_escaping_local_allocs, it should not have a my_region parameter. This check would have caught at least one bug in the past that lead to a space leak.

This PR includes #1784 to ensure that contains_no_escaping_local_allocs is set correctly.

let[@inline never] allocs_in_parent i = [%exclave] (local_ (ref i))

let f (x : int) =
  (* region needs to remain *)
  let local_ p = x, x in
  let f1 = fst p in
  let f2 = snd p in
  let r = allocs_in_parent f1 in
  let s = sin (float_of_int f1) in
  let c = cos (float_of_int f2) in
  s, c, !r

let f_exn (x : int) =
  (* region needs to remain *)
  let local_ p = x, x in
  try
    (* try-region needs to remain *)
    let f1 = fst p in
    let f2 = snd p in
    let r = allocs_in_parent f1 in
    let s = sin (float_of_int f1) in
    let c = cos (float_of_int f2) in
    s, c, !r
  with _exn -> 0.0, 1.0, 2

let g (x : int) =
  (* region should be removed *)
  let local_ p = x, x in
  let f1 = fst p in
  let f2 = snd p in
  let s = sin (float_of_int f1) in
  let c = cos (float_of_int f2) in
  s, c

let g_exn (x : int) =
  (* region should be removed *)
  let local_ p = x, x in
  try
    (* try-region should be removed *)
    let f1 = fst p in
    let f2 = snd p in
    let s = sin (float_of_int f1) in
    let c = cos (float_of_int f2) in
    s, c
  with _exn -> 0.0, 1.0

stedolan and others added 2 commits September 20, 2023 17:57
Texp_function {region} indicates whether the function has a region,
but Lfunction {region} indicates whether the function may allocate
locals into the region of its caller. These used to coincide, but do
not since exclaves were added, so need to be computed differently.
@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Sep 20, 2023
@mshinwell mshinwell force-pushed the flambda2-stop-apply-expr-needing-region-all-the-time-option-b branch from 56a04c4 to b15f009 Compare September 20, 2023 17:39
@mshinwell mshinwell force-pushed the flambda2-stop-apply-expr-needing-region-all-the-time-option-b branch from b15f009 to da507bd Compare September 20, 2023 18:13
@mshinwell
Copy link
Copy Markdown
Collaborator Author

Superceded by #1871

@mshinwell mshinwell closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flambda2 Prerequisite for, or part of, flambda2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants