New strategy for deletion of begin/end region primitives#1847
Closed
mshinwell wants to merge 3 commits intooxcaml:mainfrom
Closed
New strategy for deletion of begin/end region primitives#1847mshinwell wants to merge 3 commits intooxcaml:mainfrom
mshinwell wants to merge 3 commits 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.
56a04c4 to
b15f009
Compare
b15f009 to
da507bd
Compare
Collaborator
Author
|
Superceded by #1871 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_exnhas two pairs of them.The starting point for the fix is the observation that, if an
Apply_exprcan be turned into a direct call, we can examine thecontains_no_escaping_local_allocsflag on the code metadata of the callee. If this flag istrue, 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,noallocC 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_allocsfor 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_occurrencesbecause the set of variables would overlap with the existingnamessets there.Instead this patch takes a different approach. It removes the old method by which
Begin_region,Begin_try_regionandEnd_regionwere deleted when redundant, although the place in the dataflow analysis that gives a special treatment toEnd_regionis still present. Instead of deleting when redundant, the primitives are now marked that they are redundant, andTo_cmmis permitted to delete them. They are never deleted inSimplifyfor the above reason.It might seem as if
Begin_regionandBegin_try_regionshould 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 unusedBegin_try_regionprimitives could falsely holdBegin_regionprimitives alive.With this new strategy, we can now change the free names computations in
Apply_exprto simply conceal theregionvariable 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 amy_regionparameter. 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_allocsis set correctly.