Grouping symbols for closures and code, and closure projection substitution #164
Conversation
Keryan-dev
force-pushed
the
symbol-grouping
branch
from
b46de79 to
1a68c8a
Compare
Keryan-dev
force-pushed
the
symbol-grouping
branch
from
1a68c8a to
7f91178
Compare
Keryan-dev
force-pushed
the
symbol-grouping
branch
from
12ee0b0 to
2b3d1c6
Compare
Keryan-dev
force-pushed
the
symbol-grouping
branch
from
2b3d1c6 to
47b8369
Compare
Keryan-dev
requested review from
chambart,
Gbury,
lukemaurer and
poechsel
as code owners
|
Sorry for the request, I just rebased preventively on top of my other PRs. |
|
Is this still a draft @Keryan-dev, or is it ready for review ? |
Keryan-dev
force-pushed
the
symbol-grouping
branch
from
47b8369 to
bdd538f
Compare
Keryan-dev
force-pushed
the
symbol-grouping
branch
from
bdd538f to
49600a4
Compare
There was a problem hiding this comment.
I'm fine with the code, though I have made a few remarks, mostly on naming.
The strongly connected components part is a bit ugly, but factorising the code with the one in Lifted_constant_state turned out to be tricky, so we're going to work on it in a separate PR.
| let code = | ||
| try Code_id.Map.find code_id all_code | ||
| (fun (bound_symbols, static_consts) group_id -> | ||
| let bound_symbol, static_const = |
There was a problem hiding this comment.
Despite the singular used in bound_symbol, this is a Bound_symbol.Pattern.t which can include several symbols (if binding a set of closures). So despite the fact that currently we don't generate static sets of closures with more than one closure in Closure_conversion, I'd prefer if we used a different name like bound_pattern.
There was a problem hiding this comment.
It can actually bind several symbols if you write some dumb code like
let rec f x = x + 1
and g y = y -1There was a problem hiding this comment.
I thought when looking at the code that it would generate two distinct Bound_symbols.Pattern.t, one for each function. Did I miss something ?
There was a problem hiding this comment.
The code never dissociates Lletrec containing several functions. It is lifted as it is as long as there is no closure variables.
There was a problem hiding this comment.
Ah, indeed. It tracks the approximations and substitutions for individual symbols, but it does lift a single set of closures.
We could actually split the set of closures into individual closures (and it might be actually helpful), but we can reconsider this later.
|
@mshinwell This PR is done, and can be merged. |
…tution (#164) * lifting of statically closed functions in Lambda_to_flambda * Ordering all code and closures symbols * Closure projection substitution in lifted code * Restrict lifting to classic mode * Renaming and cleanup
Original PR: ocaml-flambda/ocaml#541, needs #162.