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

Initial refactoring of To_cmm #619

Merged
merged 38 commits into from
Apr 25, 2022
Merged

Conversation

mshinwell
Copy link
Collaborator

There should be no changes here at all except refactoring. Best read commit by commit. The main things are:

  • To_cmm_helper is no longer a dependency of To_cmm_env and To_cmm_result. This means we can share more functions between different source files.
  • The primitive simplification code has been moved to To_cmm_primitive.
  • The code for dealing with the Flambda_unit.t itself is now in To_cmm and the old To_cmm was moved to To_cmm_expr.
  • The recursive binding in To_cmm_expr has been significantly shrunk in size, which I think helps to understand the flow of what's going on, although it does mean that some of the helper functions are now further away. Various functions have been simply inlined out; it was kind of hard to follow what was going on before, and this also reduces the risk of variable names diverging (which they had in some places).
  • Some minor changes such as removing unnecessary res parameters in some places.

One of the main aims is to break the code down into smaller pieces so it's easier to check critical invariants, e.g. relating to linear use of the environment.

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Apr 15, 2022
Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good but I left comments and I have a few overall remarks:

  • outside of one or two shared/helper modules, code in to_cmm should never directly build cmm expressions using the datatype constructors; everything should go through smart constructors (as if the cmm types were private)
  • we need a better and clearer distinction for what belongs in the to_cmm_helper module. what I propose is the following:
    • have all smart constructors for cmm be in either cmm_helpers or to_cmm_helpers (and thus if you prefer to not have a to_cmm_helper file, move all constructors to the cmm_helpers file) (and again ideally all of these should in my opinion be in the cmm module directly, but it's probably too late for that)
    • have simple/arg_list functions that consume flambda2 things and produce cmm be shared be in a to_cmm_shared file

middle_end/flambda2/to_cmm/to_cmm_env.mli Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_env.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm.ml Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_env.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_result.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_helper.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_expr.ml Outdated Show resolved Hide resolved
Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a lot of small comments, which have to be addressed.

Some notes:

  • the formatting of cmm_helpers destroyed a lot of formatting in comments which have to be restored, and some parts of the code building some large cmm expressions became (even more) unreadable
  • we will really need a second PR to harmonize cmm_helpers.ml: put the basic smart constructors for cmm expression at the top, and re-use them throughout the file, and try and decide on a common pattern for function signatures (i.e. labelled arguments or not, debuginfo as first or last argument, etc...).

backend/cmm_helpers.ml Outdated Show resolved Hide resolved
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
backend/cmm_helpers.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_set_of_closures.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_set_of_closures.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_set_of_closures.ml Outdated Show resolved Hide resolved
middle_end/flambda2/to_cmm/to_cmm_set_of_closures.ml Outdated Show resolved Hide resolved
ocaml/utils/target_system.mli Show resolved Hide resolved
@mshinwell
Copy link
Collaborator Author

Regarding a second PR to deal with moving functions around in Cmm_helpers, I think we should wait. The desired situation is that Flambda 2 becomes the only middle-end, at which point a lot of the original Cmm generation code can be deleted. That seems like the right time to make further improvements to the file.

Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good, waiting for CI to be green.

@mshinwell mshinwell merged commit 7d0155c into ocaml-flambda:main Apr 25, 2022
lpw25 added a commit that referenced this pull request May 19, 2022
fe8a98b0c flambda-backend: Save Mach as Cfg after Selection (#624)
2b205d886 flambda-backend: Clean up algorithms (#611)
524f0b435 flambda-backend: Initial refactoring of To_cmm (#619)
0bf75de86 flambda-backend: Refactor and correct the "is pure" and "can raise" (port upstream PR#10354 and PR#10387) (#555)
d234bfdbe flambda-backend: Cpp mangling is now a configuration option (#614)
20fc614bf flambda-backend: Check that stack frames are not too large (#10085) (#561)
5fc2e9503 flambda-backend: Allow CSE of immutable loads across stores (port upstream PR#9562) (#562)
2a650deec flambda-backend: Backport commit fc9534746bf5d08a4c109f22e344cf49d5d46d54 from trunk (#584)
31651b87e flambda-backend: Improved ARM64 code generation (port upstream PR#9937) (#556)
f0b6d68e8 flambda-backend: Simplify processing and remove dead code (error paths) in asmlink (port upstream PR#9943) (#557)
90c674687 flambda-backend: Improve code-generation for inlined comparisons (port upstream PR#10228) (#563)

git-subtree-dir: ocaml
git-subtree-split: fe8a98b0cd38bf1872b8faf3b93542caebabad18
mshinwell added a commit that referenced this pull request May 20, 2022
lpw25 added a commit to lpw25/flambda-backend that referenced this pull request May 20, 2022
fe8a98b0c flambda-backend: Save Mach as Cfg after Selection (ocaml-flambda#624)
2b205d886 flambda-backend: Clean up algorithms (ocaml-flambda#611)
524f0b435 flambda-backend: Initial refactoring of To_cmm (ocaml-flambda#619)
0bf75de86 flambda-backend: Refactor and correct the "is pure" and "can raise" (port upstream PR#10354 and PR#10387) (ocaml-flambda#555)
d234bfdbe flambda-backend: Cpp mangling is now a configuration option (ocaml-flambda#614)
20fc614bf flambda-backend: Check that stack frames are not too large (#10085) (ocaml-flambda#561)
5fc2e9503 flambda-backend: Allow CSE of immutable loads across stores (port upstream PR#9562) (ocaml-flambda#562)
2a650deec flambda-backend: Backport commit fc9534746bf5d08a4c109f22e344cf49d5d46d54 from trunk (ocaml-flambda#584)
31651b87e flambda-backend: Improved ARM64 code generation (port upstream PR#9937) (ocaml-flambda#556)
f0b6d68e8 flambda-backend: Simplify processing and remove dead code (error paths) in asmlink (port upstream PR#9943) (ocaml-flambda#557)
90c674687 flambda-backend: Improve code-generation for inlined comparisons (port upstream PR#10228) (ocaml-flambda#563)

git-subtree-dir: ocaml
git-subtree-split: fe8a98b0cd38bf1872b8faf3b93542caebabad18
mshinwell added a commit that referenced this pull request May 24, 2022
454150b1a7 flambda-backend: Speed up testsuite (#658)
8362f9e90c flambda-backend: Speed up builds (#585)
a527cabbb2 flambda-backend: Update backends for changes from ocaml-jst
ce8883357f Merge flambda-backend changes
b7506bbc69 Revert "Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (#12)"
183f688179 Add config option to enable/disable stack allocation (#22)
ee7c849eb8 If both the type and mode of an ident are wrong, complain about the type. (#19)
44bade06c7 Allow submoding during module inclusion checks (#21)
de3bec9aea Add subtyping between arrows of related modes (#20)
fe8a98b0cd flambda-backend: Save Mach as Cfg after Selection (#624)
2b205d8861 flambda-backend: Clean up algorithms (#611)
93d861556b Enable the local keywords even when the local extension is off (#18)
524f0b435e flambda-backend: Initial refactoring of To_cmm (#619)
81dd85ee19 Documentation for local allocations
b05519f16c Fix a GC bug in local stack scanning (#17)
9f879dea8c Fix __FUNCTION__ (#15)
0bf75de86a flambda-backend: Refactor and correct the "is pure" and "can raise" (port upstream PR#10354 and PR#10387) (#555)
d234bfdbe4 flambda-backend: Cpp mangling is now a configuration option (#614)
20fc614bff flambda-backend: Check that stack frames are not too large (#10085) (#561)
5fc2e95038 flambda-backend: Allow CSE of immutable loads across stores (port upstream PR#9562) (#562)
2a650deeca flambda-backend: Backport commit fc9534746bf5d08a4c109f22e344cf49d5d46d54 from trunk (#584)
a78975eb57 Optimise "include struct ... end" in more cases (ocaml/ocaml#11134)
b819c66154 Cherry-pick of ocaml/ocaml 1eeb0e7fe595f5f9e1ea1edbdf785ff3b49feeeb (#12)
bb363d4e70 Optimise the allocation of optional arguments (#11)
31651b87ef flambda-backend: Improved ARM64 code generation (port upstream PR#9937) (#556)
f0b6d68e86 flambda-backend: Simplify processing and remove dead code (error paths) in asmlink (port upstream PR#9943) (#557)
90c6746877 flambda-backend: Improve code-generation for inlined comparisons (port upstream PR#10228) (#563)

git-subtree-dir: ocaml
git-subtree-split: 454150b1a78ad6b30cd6892f07f56db2f0b63aa8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants