Classic mode generates .cmx #336
Conversation
Keryan-dev
requested review from
chambart,
Gbury,
lthls,
lukemaurer,
mshinwell and
poechsel
as code owners
There was a problem hiding this comment.
Looks good overall (though I didn't look at the cmx handling changes). However, this PR conflicts with other re-factoring PR currently opened, and I suspect the rebase will be non trivial, so let's wait until the re-factoring PR are merged before merging this one ?
Keryan-dev
force-pushed
the
classic-cmx
branch
from
3629938 to
6ae749c
Compare
Keryan-dev
force-pushed
the
classic-cmx
branch
from
6ae749c to
484954c
Compare
Keryan-dev
force-pushed
the
classic-cmx
branch
from
9f5cce0 to
85d41d2
Compare
Keryan-dev
force-pushed
the
classic-cmx
branch
from
85d41d2 to
08156ae
Compare
Keryan-dev
force-pushed
the
classic-cmx
branch
2 times, most recently
from
5ceae03 to
b682975
Compare
There was a problem hiding this comment.
I've re-reviewed the PR. It's mostly fine, but I've left comments in a few places that we'll need to come back to at some point.
There are also a few small patches I'd like to make to the conversions between types and approximations, I'll approve when I've pushed them.
| in | ||
| let used_closure_vars = | ||
| match used_closure_vars with | ||
| | Known used_closure_vars -> used_closure_vars |
There was a problem hiding this comment.
We should be able to remove the Or_unknown wrapper around used_closure_vars now (but we can do it in a separate PR).
| Name_occurrences.remove_continuation t.free_names cont | ||
| (* We don't remove the continuation from [t.continuation_applications] | ||
| here because we need this information of the module block to escape | ||
| its scope to build the .cmx in [Closure_conversion.close_program]. *) |
There was a problem hiding this comment.
That doesn't look like the cleanest way to solve this problem, but it works for now. We can come back with a better fix later.
| let alloc_mode = | ||
| match alloc_mode with | ||
| | Known am -> am | ||
| | Unknown -> Alloc_mode.Heap |
There was a problem hiding this comment.
This is a bit fishy, but I'm not sure Local allocations reachable from a symbol make sense anyway
|
|
||
| let prepare_cmx_from_approx ~approxs ~exported_offsets ~used_closure_vars | ||
| all_code = | ||
| if Flambda_features.opaque () |
There was a problem hiding this comment.
We could add a || not (Flambda_features.classic_mode ()) to save some time when not in classic mode.
* Conversions for approximations * Refactor: Allow sharing of cmx loading outside Simplify * Fix code loading * Expose functions using approximations in Flambda_cmx * make fmt * Fix tests * WIP cmx from classic mode * Free names bugfix + cleanup * Handle some edge-cases * filter approximation of imported symbols * retrict inlining to classic mode * Comment and typo * fix offsets exportation * proper exported offsets * Small cleanup in approx conversions Co-authored-by: Vincent Laviron <[email protected]>
From #203, relevant diff.
This improves #203 by generating .cmx for units compiled in classic mode, allowing the inlining to be done beyond the local unit.
Thanks to the work of @lthls, the cmx of both classic mode and simplify are compatible.