Fix a bug in the handling of regions with exclaves#1784
Fix a bug in the handling of regions with exclaves#1784stedolan wants to merge 1 commit intooxcaml:mainfrom
Conversation
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.
|
It's worth mentioning that we'd like to rename the badly-named
|
lpw25
left a comment
There was a problem hiding this comment.
Looks good. One small suggestion.
| ~tail:(loop ~depth) | ||
| ~non_tail:ignore (* Exclaves are in tail position *) | ||
| in | ||
| loop ~depth:0 lam |
There was a problem hiding this comment.
I think this part is no different than the original code other than supporting nested Lregion. Note that nested regions are not currently supported and would break in a few places in lambda, so this is isn't strictly necessary.
| let loc = of_location ~scopes e.exp_loc in | ||
| let body = if region then maybe_region_layout return body else body in | ||
| let lam = lfunction ~kind ~params ~return ~body ~attr ~loc ~mode ~region in | ||
| let may_locally_allocate_in_parent = may_allocate_in_region body in |
There was a problem hiding this comment.
This feels a bit wasteful in the case where the function has a region and does no local allocations: we end up looking at all it's allocations twice. Is there a simple way to only do one traversal here?
|
I believe this has been merged as part of another PR. |
There is a well-named field in
Typedtree.Texp_functioncalledregion: it is true if this function has a region.There is a badly-named field in
Lambda.lfunctioncalledregion: it is true if this function cannot allocate locals in its caller's region.These two ideas used to coincide, but they don't since the addition of exclaves, which allow a function with a region (
Texp_function{region = true}) to allocate locals in its caller's region inside anexclave_. This means it is incorrect to directly initialise theregionfield oflfunctionwith that ofTexp_function.This patch fixes the issue by computing the
regionfield of lfunction by examining the body. It also weakens the invariants slightly on lfunction: it is no longer the case thatregion=falseimpliesnlocals>0. (This only mattered to inhibit a function-merging optimisation in Simplif that is not valid in this case, but that optimisation is already inhibited byregion=falsealone).