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

Phantom let support in Cmm #2070

Merged
merged 3 commits into from Oct 16, 2018

Conversation

Projects
None yet
3 participants
@mshinwell
Copy link
Contributor

commented Sep 26, 2018

This patch adds the "phantom let" construct to the Cmm language and propagates it through Cmmgen.

@lthls Perhaps you could review this, it follows on from GPR#2060.

@mshinwell mshinwell added this to the 4.08 milestone Sep 26, 2018

@mshinwell mshinwell changed the title Phantom let support for Cmm Phantom let support in Cmm Sep 26, 2018

@lthls

lthls approved these changes Sep 26, 2018

Copy link
Contributor

left a comment

A straight-forward propagation of the phantom-let constructions to Cmm. These constructions are dropped at Selectgen for now, but I assume a subsequent PR will build on this one and propagate them further.

I'm wondering about reusing the exact same type as Clambda for defining expressions; this is certainly convenient, but it might be cleaner in this case to define a specific type for the constants allowed to appear in Uphantom_const and Uphantom_read_symbol_field.
Do you think it would simplify the code that later deconstructs these expressions ?
It would also allow you to put these type definitions into their own file, and simplify dependencies a bit.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

I'm happy to use a separate type; indeed I was wondering about that myself. I think in general it's probably a mistake to share types across intermediate languages, even though it does require rather more typing to establish a situation where they are not shared.

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

@lthls Please look at 8913f42. If that looks reasonable then I will adapt GPR#2071 to make use of this. I don't think we need to use a separate file for that time at this juncture.

@lthls

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

This looks better indeed.
I still have two questions:

  • Is there a reason why the Clambda version of Uphantom_read_symbol_field takes its symbol in the form of a uconstant instead of a bare string ? I assumed the associated defining expr could be useful later, but if you don't use it when translating to Cmm, you might as well forbid it as early as Clambda.
  • Could you document that the constant integers used in the Cmm Cphantom_const_int represent OCaml integers, untagged ? This is obvious in earlier representations, but Cmm mostly manipulates machine integers, so I think a comment on the meaning/encoding here wouldn't hurt (of course, a full conversion of the backend to use the Targetint types would help, too, but that's outside the scope of this PR).
@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2018

I don't remember why Uphantom_read_symbol_field didn't just take a symbol name, so I've changed it to that for the moment. We can always alter it later.

Good point about the tagging---in fact, Cphantom_const_int should just be like Cconst_int. Previously, the DWARF emitting code was doing the tagging, which is wrong. Now it's done in Cmmgen. I've also changed this constructor to use Targetint.t. (It should really be "target-width integers of the same size as [int]", like Targetint.OCaml in the new Flambda branch, but we don't have that type merged yet.)

@mshinwell mshinwell force-pushed the mshinwell:cmm_phantom_let branch from 6ae60ce to 8db8b07 Oct 1, 2018

@chambart

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

I trust @lthls review on this, and approve. This is good to merge as soon as the conflict on Changes is fixed.

@chambart chambart added the approved label Oct 4, 2018

@mshinwell mshinwell force-pushed the mshinwell:cmm_phantom_let branch from 8db8b07 to 1a1a737 Oct 15, 2018

@mshinwell mshinwell force-pushed the mshinwell:cmm_phantom_let branch from 1a1a737 to 4204f63 Oct 15, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Going to wait for CI on this one before merging.

@mshinwell mshinwell merged commit 72378c0 into ocaml:trunk Oct 16, 2018

2 checks passed

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

damiendoligez added a commit to damiendoligez/ocaml that referenced this pull request Nov 5, 2018

@nojb nojb referenced this pull request May 21, 2019

Merged

Remove duplicated entry #8686

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.