Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove regions with exclaves #1524

Merged
merged 5 commits into from
Aug 9, 2023
Merged

Remove regions with exclaves #1524

merged 5 commits into from
Aug 9, 2023

Conversation

lukemaurer
Copy link
Contributor

@lukemaurer lukemaurer commented Jun 26, 2023 •
edited
Loading

Remembers which regions have exclaves, then when removing a region with
exclaves, finds and removes them.

Previously, any region with an exclave (including the implicit exclave of a close-at-apply tail call) couldn't be removed and thus had to be marked as used.

@lukemaurer lukemaurer force-pushed the remove-regions-with-exclaves branch 4 times, most recently from ffa99e4 to 100b7fa Compare June 27, 2023 13:32
@lukemaurer lukemaurer mentioned this pull request Jun 27, 2023
Merged
@lukemaurer
Remove regions with exclaves
c262ded 
Remembers which regions have exclaves, then when removing a region with
exclaves, finds and removes them.
@lukemaurer
Use a variant of map_subexpressions
d31893c
@lukemaurer
Mark close-on-apply tail calls as exclaves
d1e7d77 
Previously, a region that had close-on-apply tail calls but no (other) exclaves
would skip `remove_exclaves` and therefore not change those tail calls to normal
tail calls.
@lukemaurer
Only change close-at-apply tail calls at depth 0
d617694
@lukemaurer lukemaurer force-pushed the remove-regions-with-exclaves branch from 7eae109 to d617694 Compare June 27, 2023 14:34
@lukemaurer lukemaurer marked this pull request as ready for review June 27, 2023 14:49
@lukemaurer lukemaurer requested a review from mshinwell as a code owner June 27, 2023 14:49
lukemaurer added a commit to lukemaurer/flambda-backend that referenced this pull request Jun 30, 2023
@lukemaurer
CRs ocaml-flambda#1524 and ocaml-flambda#1529
fd07032
lukemaurer added a commit to lukemaurer/flambda-backend that referenced this pull request Jun 30, 2023
@lukemaurer
Squash 'flatten-regions'
7691e17 
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`.
@lukemaurer lukemaurer mentioned this pull request Jun 30, 2023
Closed
@lukemaurer lukemaurer requested a review from lthls as a code owner August 8, 2023 17:53
@mshinwell
Copy link
Collaborator

@stedolan has approved, so merging.

@mshinwell mshinwell merged commit 166e8a9 into ocaml-flambda:main Aug 9, 2023
lukemaurer added a commit to lukemaurer/flambda-backend that referenced this pull request Aug 9, 2023
@lukemaurer
Squash 'flatten-regions'
9b05a30 
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`.
tonowak pushed a commit to tonowak/flambda-backend that referenced this pull request Aug 11, 2023
@lukemaurer @tonowak
Remove regions with exclaves (ocaml-flambda#1524)
2acfa3f
Ekdohibs pushed a commit to Ekdohibs/flambda-backend that referenced this pull request Aug 31, 2023
@lukemaurer @Ekdohibs
Remove regions with exclaves (ocaml-flambda#1524)
ea8bad1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stedolan stedolan stedolan approved these changes

@mshinwell mshinwell Awaiting requested review from mshinwell

@lthls lthls Awaiting requested review from lthls

Assignees
No one assigned
Labels
middle end
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lukemaurer @mshinwell @stedolan