Start from emit

[gretay-js]

This PR adds a command line option "-start-from " to start compilation from a given pass. Currently, only "emit" pass is supported. The input files are expected to be in Linear format. This is analogous to the way ppx rewriting is implemented in the frontend.

This option, along with "-stop after" and "-save-ir-after", provides a way to split compilation into fine-grained phases. The motivation for this is to perform code layout optimizations: an external tool can read Linear IR from a file, manipulate it via compiler libs, save to a file, and then emit the code as usual by invoking the compiler. We are planning to release a tool that will do this.

An alternative to "-start-from emit" option would be to call Emit directly from compiler-libs. It would require replicating intricate code from asmgen in the external tool, and command line parsing to set Clflags fields that may affect the emitter and later stages. Also, the option "-start-from emit" improves the integration with build systems that can automatically avoid redundant recompilation when only the layout changes.

This PR is on top of PR #8939. The only new commit here is 02e5554.

[mshinwell]

I've made comments on commit 02e5554. This unfortunately doesn't show up as a "review".

In a similar way to what I wrote on #8939, someone else should review the general modus operandi of this patch, although I think it is generally fine. I think it would be sensible if someone more familiar with the driver (perhaps @diml or @trefis ) reviewed those parts of the patch in detail too.

[gretay-js]

I've made comments on commit 02e5554. This unfortunately doesn't show up as a "review".

It's probably because the original commit referenced in the PR description changed after a rebase, sorry. I can see your comments on the commit, and I've addressed them with the latest update.

[ghost]

I had a look at the changes to driver/. Overall they seem fine to me as well, but I'm not completely sure about opening and closing the file in the driver to determine its type, which in particular means that the compiler is going to open an input file twice. My guess is that this is not necessary?

[ghost]

Driver changes look good to me!

[damiendoligez]

It seems a bit strange to me to -stop-after some pass and then -start-from some other pass. You need to know which pass comes after which and being sure that no one introduced a new pass in the compiler.

I'd rather have a -start-after option that takes the same argument as the corresponding -stop-after.

[gretay-js]

@damiendoligez yes, -start-after is better than -start-from. I updated the PR accordingly.

[xavierleroy]

Apologies for the bikeshedding, but what about "stop before" and "start from"? My washing machine (a fine analogy for a multi-pass compiler) has a "stop before spinning" button, it's not labeled "stop after last rinse" :-)

[gasche]

The way I understood the comment from Damien, he was supposing that it was a change of semantics, not just of naming: for what you were previously using -start-from emit, he would now suggest using -start-after scheduling itself (assuming that this is to restart a compilation process that was interrupted with -stop-after scheduling). In your last commit I see only a change of names, plus the removal of the emit pass; I would also expect a change of semantics in the definition of should_start_{from,after}. Am I missing something? (Was Damien missing something, and in fact the semantics was already the one he had in mind?)

[gasche]

@xavierleroy it may be the case that the two should be offered, but I thought about this as well and I think that -stop-after is the more useful one, at least for my use-cases.

-stop-after typing means "I care about the typedtree dump, do everything you need to show me that and no more". I don't know when I would want to say "oh please make sure to not run pass X, stop right before that". Due to this asymmetry, I think -stop-after is the more natural option.

For -start-{before,after}, it is less clear to me; but if the idea is that we save some IR when we stop and use it to restart, then the nice thing with -start-after is that the pass named is the one that corresponds to the structure of the IR we should provide as input. (In addition to Damien's point that it matches the -stop-after name). But maybe some IRs do not correspond to one pass, but rather to the state between two passes; I am used to frontend stuff where each IR is clearly the output of a pass (parsing, typing).

[gretay-js]

@xavierleroy Initially, I had the three options referring to the pass that actually matters:
-stop-before emit -save-ir-before emit -start-from emit
but there was already -stop-after for other passes, so I tried to be consistent with it.

Keeping -stop-after unchanged, it seems better to have -start-after than -start-from so they talk about the same pass at least. It's still not ideal, as the user needs to know which pass comes before "emit", and if the order of passes changes, the build system rules would need to be adjusted.

Having both -stop-after and -stop-before might be confusing, and another syntax for it didn't seem worth the complexity and breaking backwards compatibility. I was thinking of -stop before,pass -stop after,pass -save-ir after,pass and so on.

@gasche

The way I understood the comment from Damien, he was supposing that it was a change of semantics, not just of naming: for what you were previously using -start-from emit, he would now suggest using -start-after scheduling itself (assuming that this is to restart a compilation process that was interrupted with -stop-after scheduling). In your last commit I see only a change of names, plus the removal of the emit pass; I would also expect a change of semantics in the definition of should_start_{from,after}. Am I missing something? (Was Damien missing something, and in fact the semantics was already the one he had in mind?)

There is a change in optcompile.ml from
should_start_from Compiler_pass.Emit
to
should_start_after Compiler_pass.Scheduling

[gasche]

I guess that I don't understand what the semantics of should_start_{before,after} is with the current implementation. (I have the feeling that it is wrong that it did not change when you changed the semantics, but you point out that you changed the fixed parameter that is passed to it; to me this means that we don't really know what this function is supposed to be doing.)

should_stop_after has a clear semantics and it is implemented in the compiler in a fairly natural way: the compiler performs a list of (imperative) steps, and before each step we check if we "should stop" and in this case we stop.

start_after is a completely different game, there is only one pass that supports it and it is implemented in a very ad-hoc way in the driver. Looking at it, I'm not convinced that this is the right approach. (I think that if it was, there should be a more natural implementation, but I'm not sure how to implement it; maybe we would need to rethink the driver and have a proper "pass manager"?).

Currently in the compiler we use a mix of contextual information and specific flags to decide where to start from: .ml files are compiled and linked by default, but .cmo are only linked (so this is "start after cmo" I guess?). Why wouldn't we just consider that any .ir input should be compiled into an object file (and then linked into an executable if the command-line asks for executable production)?

[gretay-js]

start_after is a completely different game, there is only one pass that supports it and it is implemented in a very ad-hoc way in the driver.

The implementation in the driver uses should_start_after similarly to the way should_stop_after has already been used before in compile_common.ml in the driver.

Why wouldn't we just consider that any .ir input should be compiled into an object file (and then linked into an executable if the command-line asks for executable production)?

I like this approach! I don't think we can detect, based on the IR alone, which pass to start from. In the backend, there is more than one pass operating on the same IR. For Linear IR, this is currently only "scheduling" and "emit". For Mach, there are many passes and compilation can break if IR is not processed by the correct pass, but we don't have an option to save Mach IR yet.

We could record in the IR which pass produced it.

For Linear IR, simply starting from the first pass that consumes it would also work. Scheduling doesn't do anything on amd64 and arm64. For other targets, scheduling is not idempotent, I think, but it will still generate correct code, just not the same as "normal" compilation from .ml.

Is that what you had in mind?

[gasche]

I hadn't considered the which-pass issue, but yes I would find it natural to record in the serialized output the pass that produced the IR, and then start from the next pass when receiving the dump as input.
(I don't know much about the backend but I suppose that the different passes produce programs that, even if they share a representation, satisfy different invariants. In a sense the "which pass" information also records the static knowledge of which invariants the program should satisfy. This is important if later you decide to add an explicit check for (some) invariants (similiarly to flambda-invariants), to be able to check that the external ir-processing programs did not break those invariants. Then knowing the pass / state also tells us which invariants to check.)

The implementation in the driver uses should_start_after similarly to the way should_stop_after has already been used before in compile_common.ml in the driver.

I may be looking at the wrong place but what I see is

-  Compile_common.implementation info ~backend
+ if Clflags.(should_start_after Compiler_pass.Scheduling) then
+   emit info
+ else
+   Compile_common.implementation info ~backend

This is not a very regular integration of should_start_after into the driver; you call it once with a specific input and, if it was set you shortcut to a completely different codepath. It is not wrong, but it doesn't really feel right either. In particular I don't see how this code structure should be extended to support "start-after" on more passes in a natural way (maybe actually trying it would tell).

In contrast, here is the should_stop_after code in driver_common.ml:

    let parsed = parse_impl info in
    if Clflags.(should_stop_after Compiler_pass.Parsing) then () else begin
      let typed = typecheck_impl info parsed in
      if Clflags.(should_stop_after Compiler_pass.Typing) then () else begin
        backend info typed
      end;
    end;

I wouldn't call it nice code, but at least it is clear that it is a natural extension of a pass pipeline (parsing-typing-backend) with some conditionals to stop along the way. If I wanted to support more passes for -stop-after, I would just go to the backend part and add a conditional after each pass in this way (or maybe a slightly nicer way to avoid cumulative indentation, but you get the idea).

I realize that this is not a very constructive comment because I can describe what I'm not fond of, but I don't have suggestions for a better implementation of the feature. With -stop-after, it is pretty simple to implement, we just stop. The problem with -start-after is that you also want to decide to consume certain inputs (you cannot "start" with no input data). I don't know how to structure the code so that you decide nicely how to consume the input and go from there, with this interface.

If our pass structure was just a sequence of imperative functions with no explicit input-output:

let driver () =
  parse ();
  typecheck ();
  simplif ();
  bytegen ();
  byteemit ();

then it would be very clear how to do it:

let ifneeded pass action =
  if rank pass > !stop_after || rank_pass <= !start_after then ()
  else action ()

let driver () =
  ifneeded Parsing parse;
  ifneeded Typing typecheck;
  ifneeded Simplif simplif;
  ifneeded Bytegen bytegen;
  ifneeded Byteemit byteemit;

The idea of automatically starting from the correct pass when an IR file is given as input solves this problem with a difference interface. Again in pseucode form, this could maybe look like this:

let rec from_source source =
  let parsetree = parse source in
  if should_stop_after Parsing then ()
  else from_parsetree parsetree

and from_parsetree parsetree =
  let typedtree = typecheck parsetree in
  if should_stop_after Typing then ()
  else from_typedtree typedtree

and from_typedtree typedtree = ...

let driver () =
  let info = ... in
  match input with
  | Source source -> from_source source
  | Parsetree parsetree -> from_parsetree parsetree
  | Typedtree typedtree -> from_typedtree typedtree
  | ...

I'm not saying that the code should be like that, but at least I have the impression that we could implement it this way if we wanted something more generic, which is reassuring.

[gretay-js]

@gasche thank you for the detailed explanation.

Following your suggestion, I removed -start-from command line option. The new version uses the input filename extension to decide which pass to start from. This version simplifies code from the
underlying PR #8939, in particular there is no need for Compiler_ir module to be exposed in Clflags and for Compenv to check pass order.

The implementation goes some way towards the generic interface you outlined, with minimal changes to the existing code. Specifically, the mapping from IR to compiler pass is made by
Clflags.Compiler_pass instead of the driver itself (as in your pseudocode). The pass to start from is given explicitly as an extra argument to Optcompile.implement and the logic for inferring it from IR is in Clflags.Compiler_pass. The only pass that takes as input Linear IR is Emit, so for now there is no need to change the filename extension or format to account for the pass that produced it.

[gretay-js]

I've rebased this PR, all CI checks pass, and it should be easier to review now (after the merge of #8939).

[let-def]

I have reviewed the code. I believe the change is correct, and if I am not mistaken, the interface that is now implemented is consensual.
I think that all global state is properly managed at resumption but I am not 100% sure about the control flow of the driver. I will keep testing it tomorrow.

[let-def]

I am happy with the implementation overall, but I found one minor bug.
When passing simultaneously -save-ir-after scheduling and a .cmir-linear argument,
the file is compiled properly but is then overwritten with an empty linear program.

This is because write_linear is always called by Asmgen.compile_unit, even when resuming.
However, when resuming, the global linear_unit_info (the one saved to disk) is left empty (instead, a local linear_unit_info is used by Asmgen.linear_gen_implementation).

I am not sure what the best fix is, but I think that when resuming from a cmir-linear input nothing should be saved (scheduling is not part of the compilation pipeline in this case).

Other minor remark: the -for-pack argument when resuming should be set to the same value as in the original call. I wonder if instead the for-pack setting could be saved in the linear format. This is strictly more robust and does not make the driver any less flexible.

[gretay-js]

Thank you very much for the detailed review!

I am happy with the implementation overall, but I found one minor bug.
When passing simultaneously -save-ir-after scheduling and a .cmir-linear argument,
the file is compiled properly but is then overwritten with an empty linear program.

This is because write_linear is always called by Asmgen.compile_unit, even when resuming.
However, when resuming, the global linear_unit_info (the one saved to disk) is left empty (instead, a local linear_unit_info is used by Asmgen.linear_gen_implementation).

I am not sure what the best fix is, but I think that when resuming from a cmir-linear input nothing should be saved (scheduling is not part of the compilation pipeline in this case).

Ah, yes, thanks for catching this problem!
I think the best way to fix this problem is to record during Asmgen.compile_unit the name of the pass it starts from, and reset this info for every compilation unit. For example, ocamlopt foo.ml bar.cmir-linear would work. It also does the right thing when Asmgen.compile_implementation_linear and compile_unit are called from other places or an external driver using compiler-libs. Currently, only "emit" is possible as a "start_from" pass, so I just used a boolean variable to record it, although Clflags.Compiler_pass.t can also be used.

If this information is recorded during Compenv.process_action in Clflags.start_from for individual input files, then Clflags.should_save_ir_after can take this info into account. Also, there won't be any for changing the interface of Optcompile.implementation. It's overall nicer, but I thought that the fields of Clflags were intended for command line options that applied to all input files, not changed per file, right?

Other minor remark: the -for-pack argument when resuming should be set to the same value as in the original call. I wonder if instead the for-pack setting could be saved in the linear format. This is strictly more robust and does not make the driver any less flexible.

Done.
Clflags.for_package is used during emit only on power targets (that's how I missed it). Later stages with packing seem to be using cmx files directly, which already have the correct -for-pack name recorded from earlier compilation stages.

[let-def]

Thanks. I think it is good for merging now 👍 .

[gretay-js]

Thank you. I've updated the Changes file.

[gasche]

Unless I am missing something, there is currently no documentation for the feature introduced here and in #8939. The github issues are not great, given that the interface changed between the initial proposal and the actual implementation. In fact I don't even know what are the command-line options to use the feature, and I don't know where to find this information anymore. Could this be fixed?

(This question arose through Kakadu asking about the feature from the 4.12 changelog on Discuss.)

The feature should be documented in (at least) two places:

• the manpage man/ocamlopt.m
• the manual chapter for ocamlopt, manual/src/cmds/native.etex (or manual/manual/cmds/... on 4.12)

The documentation needs not be very detailed, I understand that this is a more experimental feature that is mostly there to enable external tools (you could of course link to those external tools, or decide not to do it), but I think there should be at-least-minimal documentation for all features.

[gretay-js]

Thanks for pointing out the discussion. I'll submit a PR to document these options asap.