Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compile_common: remove wrap_compilation #1944

Merged
merged 3 commits into from Aug 6, 2018

Conversation

Projects
None yet
5 participants
@gasche
Copy link
Member

commented Jul 30, 2018

wrap_compilation makes the compilation pipeline non-modular by
exposing a split between two fixed passes, a frontend and a backend,
in a .mli interface. I need a finer-grained interface for a feature
I've been using in my Menhir-parser branch, and it is likely that
other users also will need to be finer-grained than that.

This PR pushes the error/ressource handling contained with
wrap_compilation into its producers (note: this change assume that
only typecheck_impl needs Stypes.dump, and that only the optcompile
backend may generate obj and cmx files), so that the logic in
"wrap" becomes very simple, and then inlines it in the two users in
{opt,}compile.ml.

(no change entry needed)

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

cc @Drup

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

I was not particularly satisfied with wrap_compilation, but I felt it was the best way to formalize how one is supposed to catch errors produced by the various pieces of the compiler. Basically, with this change, you again have to open the implementation and copy paste it to write an actual pipeline, which is one of the thing I wanted to avoid.

That being said, I understand your point. An alternative solution to your problem would just be to distinguished frontend into "parsing" and "typing", but I guess you would argue a similar problem could arise for one of those phases.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

I would argue that with wrap_implementation you still have to look at the implementations and copy (and maybe adapt) code, to know what to give as frontend and backend functions.

(One thing missing from your comment is a judgment call. Do you think that the patchset should be accepted or rejected?)

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

Well, your remarks are right, you have a clear usecase, and I don't have something straight better to propose, so I would tend to accept.

If it was a personaly library, I would do some GADT wankery based on list of composable functions, but I know better than doing that in the compiler. :p

@gasche gasche force-pushed the gasche:compile-common-remove-wrapping branch from f3659fe to ecdc7d8 Jul 30, 2018

@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

It would seem better to replace Compiler_common.wrap_compilation by Compiler_common.implementation, that does the same kind of thing that Compiler_common.interface does: calls parse, typecheck, backend. It would be parameterized over the backend, since that differs between bytecode and native. But at least until the backend, ocamlc and ocamlopt would run the same code, which would be good, and should not be constraining (since Compilenv is a native module, I suspect the Compilenv.reset call in optcompile.ml can be delayed until before the backend, so it shouldn't get in the way)

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

@sliquister I would really like that, but since my knowledge of the backend is limited, I didn't attempted any work at trying to really unify the bytecode and native backends.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

I have thought harder about it this morning and I am skeptical that we can factorize the two implementations more than what is done in this patch, in a useful way.

I initially wanted to propose a function wrap_compilation of the form wrap_compilation : '_a -> ('_a -> 'b) -> 'b, for a certain configuration type '_a, that would remove the remaining duplication between the two implementation functions. But looking at it again, this is not very convincing:

If we want to have Compmisc.with_ppf_dump be part of this common wrapper, then we need to add ~outputprefix to the configuration/input type. The reason to have this input is only to call with_ppf_dump, it is only used once. I don't think there are strong reasons to prefer

wrap ~fileprefix:(...) @@ fun ppf_dump ->
...

over

Compmisc.with_ppf_dump ~fileprefix:(...) @@ fun ppf_dump ->
wrap @@ fun () ->
...

The former is not a simplification of the later, just a longer chain of argument passing.

For Profile.record_call info.sourcefile, my reasoning is essentially the same. I would need to add sourcefile as a parameter of wrap, for the sole reason of profiling. Explaining to users "you need to pass this because we profile inside" is not an actual simplification over "please profile yourself", just shuffling concerns around.

This leaves Warnings.check_fatal (); as the only thing left that is worth factorizing over, and I'm not convinced that not duplicating this single call deserves a higher-order interface.

Of course, there may be further reasons to factorize in the future, but personally I would rather provide users with a nicely-defined collection of helper functions for them to easily build their pipeline, rather than trying to take control from them with higher-order interfaces.

@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

To be clear, this is what I'm thinking of: sliquister@5751d53 . outputprefix needs to be passed for many reasons, not just dumping output. Same for sourcefile, it's not just profile. You argue against Compile_common.interface it seems, so would you inline this one too despite the fact that you don't need to do so for -stop-after-parse?

I find it clear that Compile_common.implementation is an improvement: it actually prevents copying and divergence between bytecode and native code. People can still build their pipelines by using the other functions, but ocamlc and ocamlopt do the same thing.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Thanks! I'm not fully sure that this is a better approach long-term, but the patch looks nice and I think it is closer in spirit to what @Drup had in mind when he wrote the refactoring. It is easy to implement -stop-after-parse on top of it, so it also fits my bill. I'll adopt it in my PR.

@Drup

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Yes, I like @sliquister's version a lot more. It's exactly what I had in mind with wrap_implementation, but pushed a bit further.

@sliquister

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Cool. To be clear: this is not strictly a refactoring because: ocamlopt now dumps to the right file (.cmx suffix), ocamlc deletes .cmo on compilation failure (same as native code does now), and Asmgen.reset is still called later than it used to (I still think this is just fine, but haven't checked in any more details).

@gasche gasche force-pushed the gasche:compile-common-remove-wrapping branch from ecdc7d8 to 4c13c82 Jul 31, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Compilenv.reset is still called later than it used to (I still think this is just fine, but haven't checked in any more details).

I have tried to look systematically at all the mutable state touched by this function, and as far as I can tell it is only used in asmcomp/, middle_end/ and toplevel/opttoploop.ml, so I believe this is safe indeed.

@Drup

Drup approved these changes Aug 3, 2018

sourcefile:string ->
outputprefix:string ->
unit
(** The complete compilation pipeline for interfaces. *)

This comment has been minimized.

Copy link
@sliquister

sliquister Aug 4, 2018

Contributor

Very minor copy paste error here (on my part): can you replace "interfaces" by "implementations"?

This comment has been minimized.

Copy link
@gasche

gasche Aug 4, 2018

Author Member

Sure, done. Thanks for the catch.

@gasche gasche force-pushed the gasche:compile-common-remove-wrapping branch from 4c13c82 to 3d42265 Aug 4, 2018

gasche and others added some commits Jul 30, 2018

compile_common: remove wrap_compilation
wrap_compilation makes the compilation pipeline non-modular by
exposing a split between two fixed passes, a frontend and a backend,
in a .mli interface. I need a finer-grained interface for a feature
I've been using in my Menhir-parser branch, and it is likely that
other users also will need to be finer-grained than that.

This PR pushes the error/ressource handling contained with
wrap_compilation into its producers (note: this change assume that
only typecheck_impl needs Stypes.dump, and that only the optcompile
backend may generate `obj` and `cmx` files), so that the logic in
"wrap" becomes very simple, and then inlines it in the two users in
{opt,}compile.ml.
@sliquister

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2018

@gasche I took a deeper look at Compilenv.reset and got to the same conclusion you did (I also grepped for registration at toplevel, but there are none except for Location.register_error).

@gasche gasche merged commit 4bdf910 into ocaml:trunk Aug 6, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

It might be of interest to others that this PR broke some of our internal code that relies on a local extension of the code generator. The reason is that after this PR, .annot files are generated before code generation, but Printtyp calls Ctype.normalize_type, which changes types; so when -annot is enabled, the code generator sees different types. In particular, some expanded abbreviations are rolled back (Tobject -> Tconstr). This turned out to impact our local extension to the code generator (problem fixed at this level), but perhaps other things could be impacted as well.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

It might be of interest to others that this PR broke some of our internal code that relies on a local extension of the code generator. The reason is that after this PR, .annot files are generated before code generation, but Printtyp calls Ctype.normalize_type, which changes types; so when -annot is enabled, the code generator sees different types. In particular, some expanded abbreviations are rolled back (Tobject -> Tconstr). This turned out to impact our local extension to the code generator (problem fixed at this level), but perhaps other things could be impacted as well.

Another reason to get rid of -annot (#2141)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.