Skip to content

Delete unused local allocation regions in Flambda 2 #809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Sep 22, 2022

Conversation

mshinwell
Copy link
Collaborator

Flambda 2 can't currently delete unused Begin_region and End_region constructs (unlike the equivalent in Flambda 1), which I suspect is leaving some amount of unnecessary code around, together with inhibiting code motion in To_cmm.

This patch aims to rectify this. It equips the primitives that allocate (and Set_of_closures) with region variables identifying which region is to be allocated in (this will always be the most recent by construction, and will remain that way throughout the pipeline). Functions now take a my_region parameter which is substituted for the current region upon inlining.

Begin_region is now marked as having only generative effects rather than arbitrary effects, which still inhibits code motion in To_cmm (until such time as we figure out how to do that safely for these constructs), but allows deletion when the region variable it returns is unused. End_region remains as having arbitrary effects but a special case is added (in Simplify_let_expr and Expr_builder) to do two things. Firstly for the purposes of Data_flow the region variable in End_region is not counted as a use. Secondly when rebuilding a Let binding an End_region we consult Data_flow to see if the region variable (the argument to End_region) was actually used. If it was unused, then the End_region is dropped, which will in turn cause the Begin_region to be cleaned up without further ado.

There is one outstanding problem relating to the region stack during CPS conversion (Lambda_to_flambda) not containing the try_region variable. Currently there is a hack in place so I can try some examples but this needs to be fixed properly. There are also a few things to fix in Fexpr.

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Sep 6, 2022
@mshinwell mshinwell force-pushed the delete-unused-regions branch from 67e107b to df3bd9a Compare September 6, 2022 16:58
@mshinwell mshinwell marked this pull request as draft September 6, 2022 16:58
@mshinwell
Copy link
Collaborator Author

As a note I'm not planning on implementing full support for these enhancements in Fexpr yet. We will need to decide what to do about the maintenance of that code and update it to match the main Flambda 2 code in due course (there are also some other areas of divergence).

@mshinwell
Copy link
Collaborator Author

This PR also now aims to ensure that we don't end up with Local alloc modes appearing in terms, likewise no Begin_region or End_region primitives, when stack allocation is disabled.

@mshinwell mshinwell force-pushed the delete-unused-regions branch from 0d177ce to d54b424 Compare September 12, 2022 13:08
Copy link
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a number of comments, some of them applying to code that wasn't modified by this PR.
The only one that I think is serious is the one in to_cmm_env.ml about the extra binding for my_region.

@mshinwell mshinwell force-pushed the delete-unused-regions branch from 4749264 to bd807d5 Compare September 21, 2022 15:11
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.

3 participants