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

Don't generate Clambda constants during Cmmgen, etc. #2280

Merged
merged 2 commits into from Mar 5, 2019

Conversation

@mshinwell
Copy link
Contributor

commented Mar 4, 2019

This patch is a prerequisite of a forthcoming patch that improves the types used to represent symbols (names of statically-allocated entities) throughout the middle end and backend. This in turn is needed for the Asm_directives layer on which the DWARF support is based.

I think a good general principle, when writing a pass that operates on some particular IR, is to avoid generating constructs from previous IRs and sending them back through part of the current pass. Instead, it is better to refactor the current pass so that the constructs you want can be generated directly in your current language.

This patch changes Cmmgen to follow this principle for constants. The reason this is necessary is because, with the forthcoming patch, it is not always possible to construct a value of the type that describes middle-end symbols (such as will appear in Clambda) during Cmmgen. I would rather not get into the details here, but in summary, this relates to the fact that during Cmmgen we may be generating code (e.g. for a startup file) that does not stem from an OCaml compilation unit.

There are a couple of other related changes that I thought reasonable to include with this patch, since our workflow management becomes difficult if there are just too many pull requests. One of them passes the list of constants, for the classic Closure mode, through from Asmgen rather than reading the Compilenv global state in Cmmgen; this is consistent with what Flambda does. The other pulls the global state from Cmmgen, much of which is to do with constants, into a separate file. Warning 40 is suppressed to increase code legibility (e.g. with Local and Global) and reduce proliferation of "open".

@lthls Could you please look at this? (Some amount of this is just moving code upwards in cmmgen.ml; you might want to diff that separately.)

@lthls

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

The patch looks fine, except for a few small issues:

  • You're losing sharing of constants generated during Cmmgen. It happens in two places: the compilation of empty arrays ([| |] == [| |] will evaluate to false with this patch, at least without flambda), and in the box_int constant case, which I couldn't find a way to trigger easily (I had to use Sys.opaque_identity because the middle-end would otherwise lift the constant before it reaches Cmmgen).
    I would suggest ensuring that the middle-end correctly shares empty arrays (both in flambda and closure), and then removing these two specific optimisations from Cmmgen.

  • You've removed the variables used for naming the symbols associated to integer operations (caml_nativeint_ops and so on), that you introduced in #2083, whereas I think it would have been better to reuse them systematically, like I proposed in #2260.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Well spotted. I've pushed a patch that should cause Closure to lift empty arrays and then share the resulting constants. As far as I can see Flambda already does this (see Simplify_primitives).

Regarding other constants not shared in Cmmgen, I am minded not to worry about this, as any potential size increase seems likely to be minor (especially given what you say about the relevant optimisation being difficult to trigger)---and in any case, we have a medium-term story for doing unboxing in a more principled manner inside the next version of Flambda.

I've reinstated the variables instead of strings for the boxed integer operations symbols. (All of the symbol strings turn into variables in a later patch, actually...)

@lthls

lthls approved these changes Mar 5, 2019

Copy link
Contributor

left a comment

There are still a few places where the strings "caml_*_ops" could be replaced by a variable, but it's not particularly important. I'm in favour of merging this PR as it stands now.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Right, those references will be sorted out by a later patch anyway. Let's merge this once CI passes. Thanks for the review.

@mshinwell mshinwell merged commit 7cb0da5 into ocaml:trunk Mar 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.