Grouping symbols for closures and code, and closure projection substitution#164
Grouping symbols for closures and code, and closure projection substitution#164mshinwell merged 5 commits intooxcaml:mainfrom
Conversation
b46de79 to
1a68c8a
Compare
1a68c8a to
7f91178
Compare
12ee0b0 to
2b3d1c6
Compare
2b3d1c6 to
47b8369
Compare
|
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 ? |
47b8369 to
bdd538f
Compare
bdd538f to
49600a4
Compare
lthls
left a comment
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.