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

Move some middle-end files around #2281

Merged
merged 14 commits into from Apr 1, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 4, 2019

This pull request contains very little code and is mostly formed of moves tracked by git.

This work has its roots in the gdb support. In that work, having spent a very long time struggling with various problems relating to object file symbols and references thereto, I decided to write patches to change the representation of symbols throughout the compiler. This enables tracking of certain extra information which can be used to check, in the backend, that we are not going outside the envelope of expressions involving symbols that we know to be correctly assembled and resolved across all architectures.

I extracted these symbol-related changes recently with a view to submitting them separately. However it then became clear that due to the complexity of existing code that handles symbols, other refactoring was going to be required first, to provide a coherent story. The main area of attention was the boundary between the middle end and backend, around Compilenv, where there is currently a lot of complexity and code that is hard to understand. I think the integration of Flambda made matters significantly worse here; we should have been more careful (guilty).

The ultimate aim of this patch is to decouple the middle end from the backend. By doing this, we can provide a clean interface, and separate "middle end" types (for example for variables and symbols) from "backend" types. We can then stop passes such as Cmmgen from reaching back into the middle end (the principle from #2280). We share types as appropriate between the Closure and Flambda middle ends; and move code specific to only one of these middle ends into separate files (some of this happens in a subsequent patch). We also remove the necessity to cherry-pick object files from the backend when linking executables such as ocamlobjinfo; the middle-end library will now suffice. A new file_formats/ directory is provided to collect together all of the files representing the formats of compilation artifacts; this seemed on balance like the best approach, given the other possibilities for places for such files to live.

We don't get all the way to the ultimate goal with this GPR alone. A subsequent smaller one, which moves some middle-end code from Asmgen into the middle_end/ directory, and the patch that provides a middle-end-only replacement for Compilenv and changes the symbol types are required for that. However this is the first step along the road.

@chambart Could you please look at this?

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@chambart @lthls I'm in the process of rebasing this and fixing the compilation problems.

@mshinwell mshinwell force-pushed the mshinwell:middle_end_moves branch from 82734a9 to 3237355 Mar 27, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

@chambart @lthls I've pushed a new version of this, extending the scope slightly to cover moving the Lambda language and its passes into a lambda directory at the same time. I think this is a beneficial change and it's probably worth grouping these changes into a single PR, as there will be the potential for stale build artifacts after this has been merged.

I believe I have now fixed up the dune files.

@chambart
Copy link
Contributor

left a comment

I can't comment on the dune part but I think this is good to go as soon as the CI is happy.

@@ -33,6 +33,7 @@ module String = Misc.Stdlib.String
emit.mlp files for certain other targets; the reference here ensures
that when releases are being prepared the .depend files are correct
for all targets. *)
[@@@ocaml.warning "-66"]

This comment has been minimized.

Copy link
@chambart

chambart Mar 27, 2019

Contributor

Was that required ?

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 27, 2019

Author Contributor

It appears to be required with the dune build, because I think it sets different warning levels. (I also think it is unfortunate that open! is falling foul of warning 66.)

@lthls

lthls approved these changes Mar 27, 2019

Copy link
Contributor

left a comment

The code changes look good, I didn't look into the build system changes in too much depth but if CI passes, it should be fine.

Makefile Outdated
middle_end/base_types asmcomp/debug driver toplevel; \
lambda file_formats middle_end/closure middle_end/flambda \
middle_end/flambda/base_types asmcomp/debug \
driver toplevel; \

This comment has been minimized.

Copy link
@lthls

lthls Mar 27, 2019

Contributor

Indentation

This comment has been minimized.

Copy link
@mshinwell

mshinwell Mar 27, 2019

Author Contributor

Fixed

@mshinwell mshinwell force-pushed the mshinwell:middle_end_moves branch from db3bd9d to 1a78171 Mar 29, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

If CI passes, this is now ready to merge.

Please be aware that as a result of source files having been moved, stale build artifacts may be left in trees after this PR is merged. I recommend using "git clean -dfx" to clean them out (but beware, this will remove any files that are not tracked by git).

@mshinwell mshinwell force-pushed the mshinwell:middle_end_moves branch from 1a78171 to 6a4132f Apr 1, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

I've rebased this, so let's try again. I have had to disable the ocamldebug and ocamldoc builds under dune because Dynlink is not built correctly. We need to have the dune build follow the same procedure as the make build. For the debugger in particular we cannot use the hack of linking against ocamlcommon instead of Dynlink_compilerlibs due to sharing of the Symtable module. I will work with colleagues to try to get these parts of the dune build resurrected as soon as possible.

@mshinwell mshinwell force-pushed the mshinwell:middle_end_moves branch from 6a4132f to f113d09 Apr 1, 2019

@mshinwell mshinwell merged commit 72ea849 into ocaml:trunk Apr 1, 2019

2 checks passed

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

chambart added a commit that referenced this pull request May 10, 2019

smuenzel added a commit to smuenzel/ocaml that referenced this pull request Jun 26, 2019

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