-
Notifications
You must be signed in to change notification settings - Fork 77
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
Handle nested exclaves in Cmm_helpers
#1529
Merged
Merged
Conversation
This file contains 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
Nested exclaves cannot currently occur, but that will change with region lifting, which will happily produce terms of the form `region ... (region ... (exclave ... (exclave ...)))`. A few functions in `Cmm_helpers` need to be updated to allow for this.
stedolan
reviewed
Jun 27, 2023
stedolan
reviewed
Jun 27, 2023
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Jun 30, 2023
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Jun 30, 2023
This squashes 'flatten-regions' into a single commit over 'remove-regions-with-exclaves' and 'cmm-nested-exclaves' (PRs ocaml-flambda#1524 and ocaml-flambda#1529, respectively). Squashed commit of the following: commit bd23d8bd75478abafb67f0e791ef77bbd8764b22 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 16:03:06 2023 +0100 Clean up code commit 0ac81ecfc5b84eb286cbd5f0afc66d7f73b3eb92 Merge: c3a9203f9 9449e65 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 14:59:27 2023 +0100 Merge branch 'cmm-nested-exclaves' into flatten-regions commit c3a9203f9de900d8acbe77b781debbfb686a3d52 Merge: c1b79d7 d617694 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 14:57:23 2023 +0100 Merge branch 'remove-regions-with-exclaves' into flatten-regions commit c1b79d7 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 14:49:07 2023 +0100 Add example commit 033294f Author: Luke Maurer <lmaurer@janestreet.com> Date: Thu Jun 8 11:21:22 2023 +0100 Code review and provisional fixes These are fixes to issues that are really second- or third-order effects of region lifting and probably need their own PRs, but I'm committing them here for testing purposes. commit 65ad3aa Merge: e88df82 d65f752 Author: Luke Maurer <lmaurer@janestreet.com> Date: Thu May 25 17:00:33 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit e88df82 Merge: de8cf43 9d3c1c1 Author: Luke Maurer <lmaurer@janestreet.com> Date: Thu May 11 13:21:45 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit de8cf43 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed May 10 16:47:44 2023 +0100 Follow renaming of `Tail` to `Exclave` after merge commit ceae59a Merge: 8a717aa 63997c9 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed May 10 16:37:23 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit 8a717aa Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed May 10 16:26:33 2023 +0100 Don't try to be clever with `let x = M in tail x` There's nothing actually wrong with `let x = M in tail x`, so don't try to reduce it to just `M`. This works in that narrow case, but the code that was doing this transformation didn't notice if the body of the tail is more than just `x`. Since the transformation doesn't actually gain anything, better to be rid of it than to make things more complicated trying to get it right. Also updated a few comments and added an invariant check. commit 6fa1ce6 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Apr 21 15:58:07 2023 +0100 Make `map_region_tail` look through regions as well commit d9043c9 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Apr 21 14:14:19 2023 +0100 Check for close-on-apply on method calls as well as functions commit 58bf88a Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed Apr 19 17:05:31 2023 +0100 Fix `Cmmgen` `Cmmgen` was (hopefully) the last remaining place where things were broken by the assumption that `region (region (tail (tail ...)))` never happens. commit 66132fb Author: Luke Maurer <lmaurer@janestreet.com> Date: Tue Apr 18 18:43:08 2023 +0100 Don't try to handle regions in `Flambda.fold_lets_option` It used to be much more necessary, but no more, and it interferes with the handling of regions using `enter_region`. commit 907627d Author: Luke Maurer <lmaurer@janestreet.com> Date: Tue Apr 18 18:41:18 2023 +0100 Refactor and clean up a bit commit dccb971 Author: Luke Maurer <lmaurer@janestreet.com> Date: Tue Apr 18 17:55:02 2023 +0100 Rewrite `let x = M in tail x` as `M` This should get rid of some annoying instances of extra variables introduced just to lift, and in particular stop making tail calls into non-tail calls. commit 6126e9e Merge: 5b62db1 e138734 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed Apr 12 14:31:25 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit 5b62db1 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed Nov 30 17:37:40 2022 +0000 Lift regions along with `let`s This transforms expressions of the form ``` let x = region ( let y = E1 in E2 ) in E3 ``` into ``` region ( let y = E1 in let x = E2 in tail ( E3 ) ) ``` so that `y` is available when simplifying `E3` (as is the advantage of lifting `let`s to begin with). This requires that `E3` not already have a `tail` expression (or tail call) inside an `if` branch or any other construct besides the body of a `let`, `let mutable`, `region`, or `tail`.
Closed
stedolan
approved these changes
Jul 4, 2023
mshinwell
approved these changes
Aug 1, 2023
lukemaurer
added a commit
to lukemaurer/flambda-backend
that referenced
this pull request
Aug 9, 2023
This squashes 'flatten-regions' into a single commit over 'remove-regions-with-exclaves' and 'cmm-nested-exclaves' (PRs ocaml-flambda#1524 and ocaml-flambda#1529, respectively). Squashed commit of the following: commit bd23d8bd75478abafb67f0e791ef77bbd8764b22 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 16:03:06 2023 +0100 Clean up code commit 0ac81ecfc5b84eb286cbd5f0afc66d7f73b3eb92 Merge: c3a9203f9 9449e65 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 14:59:27 2023 +0100 Merge branch 'cmm-nested-exclaves' into flatten-regions commit c3a9203f9de900d8acbe77b781debbfb686a3d52 Merge: c1b79d7 d617694 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 14:57:23 2023 +0100 Merge branch 'remove-regions-with-exclaves' into flatten-regions commit c1b79d7 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Jun 30 14:49:07 2023 +0100 Add example commit 033294f Author: Luke Maurer <lmaurer@janestreet.com> Date: Thu Jun 8 11:21:22 2023 +0100 Code review and provisional fixes These are fixes to issues that are really second- or third-order effects of region lifting and probably need their own PRs, but I'm committing them here for testing purposes. commit 65ad3aa Merge: e88df82 d65f752 Author: Luke Maurer <lmaurer@janestreet.com> Date: Thu May 25 17:00:33 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit e88df82 Merge: de8cf43 9d3c1c1 Author: Luke Maurer <lmaurer@janestreet.com> Date: Thu May 11 13:21:45 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit de8cf43 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed May 10 16:47:44 2023 +0100 Follow renaming of `Tail` to `Exclave` after merge commit ceae59a Merge: 8a717aa 63997c9 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed May 10 16:37:23 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit 8a717aa Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed May 10 16:26:33 2023 +0100 Don't try to be clever with `let x = M in tail x` There's nothing actually wrong with `let x = M in tail x`, so don't try to reduce it to just `M`. This works in that narrow case, but the code that was doing this transformation didn't notice if the body of the tail is more than just `x`. Since the transformation doesn't actually gain anything, better to be rid of it than to make things more complicated trying to get it right. Also updated a few comments and added an invariant check. commit 6fa1ce6 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Apr 21 15:58:07 2023 +0100 Make `map_region_tail` look through regions as well commit d9043c9 Author: Luke Maurer <lmaurer@janestreet.com> Date: Fri Apr 21 14:14:19 2023 +0100 Check for close-on-apply on method calls as well as functions commit 58bf88a Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed Apr 19 17:05:31 2023 +0100 Fix `Cmmgen` `Cmmgen` was (hopefully) the last remaining place where things were broken by the assumption that `region (region (tail (tail ...)))` never happens. commit 66132fb Author: Luke Maurer <lmaurer@janestreet.com> Date: Tue Apr 18 18:43:08 2023 +0100 Don't try to handle regions in `Flambda.fold_lets_option` It used to be much more necessary, but no more, and it interferes with the handling of regions using `enter_region`. commit 907627d Author: Luke Maurer <lmaurer@janestreet.com> Date: Tue Apr 18 18:41:18 2023 +0100 Refactor and clean up a bit commit dccb971 Author: Luke Maurer <lmaurer@janestreet.com> Date: Tue Apr 18 17:55:02 2023 +0100 Rewrite `let x = M in tail x` as `M` This should get rid of some annoying instances of extra variables introduced just to lift, and in particular stop making tail calls into non-tail calls. commit 6126e9e Merge: 5b62db1 e138734 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed Apr 12 14:31:25 2023 +0100 Merge remote-tracking branch 'upstream/main' into flatten-regions commit 5b62db1 Author: Luke Maurer <lmaurer@janestreet.com> Date: Wed Nov 30 17:37:40 2022 +0000 Lift regions along with `let`s This transforms expressions of the form ``` let x = region ( let y = E1 in E2 ) in E3 ``` into ``` region ( let y = E1 in let x = E2 in tail ( E3 ) ) ``` so that `y` is available when simplifying `E3` (as is the advantage of lifting `let`s to begin with). This requires that `E3` not already have a `tail` expression (or tail call) inside an `if` branch or any other construct besides the body of a `let`, `let mutable`, `region`, or `tail`.
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.
Nested exclaves cannot currently occur, but that will change with region lifting, which will happily produce terms of the form
region ... (region ... (exclave ... (exclave ...)))
. A few functions inCmm_helpers
need to be updated to allow for this.