Move some code from Asmgen to the middle end directory etc.#2288
Move some code from Asmgen to the middle end directory etc.#2288chambart merged 3 commits intoocaml:trunkfrom
Conversation
fef0ba2 to
5c04b13
Compare
|
@chambart @lthls Could one of you please review and approve this if you are happy? I've fixed the mistake in the comment that @lthls told me about. This is looking like quite a nice patch now, it's simplified various parts in the backend and made the middle-end interfaces consistent. Please double-check that |
|
P.S. For reviewing |
fff405a to
4e6cc05
Compare
chambart
left a comment
There was a problem hiding this comment.
This looks good. The comments are minor things except the thing about new_structured_constants that should be fixed.
The result is quite cleaner, this is a good change.
asmcomp/asmpackager.ml
Outdated
| in | ||
| Asmgen.compile_implementation_flambda | ||
| prefixname ~backend ~required_globals:Ident.Set.empty ~ppf_dump flam; | ||
| Asmgen.compile_implementation ~backend |
There was a problem hiding this comment.
That call could be factorised out.
let program, middle_end =
if Config.flambda then
...
in
Asmgen.compile_implementation ...
I'm not certain that this is better.
There was a problem hiding this comment.
It does seem to be better.
asmcomp/asmgen.ml
Outdated
| (clambda:clambda_and_constants) = | ||
| (clambda : Clambda.with_constants) = | ||
| Emit.begin_assembly (); | ||
| Cmmgen.reset (); |
There was a problem hiding this comment.
Why was that required ? Ins't the one at the beginning of compile_fundecl sufficient ?
By the way, by quickly looking at it, no reset would be required if Cmmgen_state.constants and data_items where cleaning their state.
There was a problem hiding this comment.
I've fixed things so reset is not needed.
| |> (fun { Flambda_to_clambda. expr; preallocated_blocks; | ||
| structured_constants; exported; } -> | ||
| Compilenv.set_export_info exported; | ||
| let clambda = Un_anf.apply ~ppf_dump expr in |
There was a problem hiding this comment.
This code has a linkage_name, it could be printed. It doesn't really matter.
There was a problem hiding this comment.
This should be fixed now.
| let str = Format.asprintf "%a" Flambda.print_named named in | ||
| let str_const = | ||
| let sym = | ||
| Compilenv.new_structured_constant (Uconst_string str) ~shared:true |
There was a problem hiding this comment.
This doesn't just create a symbol name, it also records the structured constant. This should probably be replaced by 'new_const_symbol'
Or at least, flambda should use clear_structured_constants if this is used.
| in | ||
| let str_const = | ||
| let sym = | ||
| Compilenv.new_structured_constant (Uconst_string str) ~shared:true |
4e6cc05 to
67a4e71
Compare
|
@chambart All comments addressed; if you are happy and this passes CI please squash and merge. |
|
This looks nice. merged. |
@chambart This is the followup to #2281. It moves some code that, morally speaking, belongs in the middle end but has thus far lived in
Asmgen. The interfaces for Closure and Flambda are harmonised and various typos (e.g. "clambda" instead of "Closure") have been fixed. In the spirit of not reaching back into the middle end from the backend,Un_anfis not called fromCmmgenany more, but always invoked fromFlambda_middle_end. We also pass constants used for instrumentation directly out ofFlambda_to_clambdaso they flow toCmmgenwithout going throughCompilenv(this will be important for a later patch).This patch includes #2280 (just merged into trunk), #2281 and #2284. For the moment just read changeset 4a3b0a8.