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

Use a different type for primitives in clambda and lambda #1579

Merged
merged 17 commits into from Feb 14, 2019

Conversation

@chambart
Copy link
Contributor

commented Jan 22, 2018

This patch defines a new Clambda_primitive module containing the primitive for Flambda and Clambda. The aim of this patch is to allow to refactor the types to remove the parts that are meaningless in the backend.

This does not yet contain a lot of the refactoring itself.

Note that this will be required by the future developments of flambda which includes a specific type for primitives in flambda that can't really be represented into the current primitive type of lambda without adding a lot of new cases useless for the bytecode implementation.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2018

The CI failure are tests that does not compile to the exact same binary with ocamlopt.opt and ocamlopt.byte.

@chambart chambart closed this Jan 22, 2018
@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

I must have misdone something. I never intended to close this...

@chambart chambart reopened this Jan 30, 2018
@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2018

I'm going to rebase this onto trunk since I've already had to do that once on the dev Flambda branch.

@chambart chambart force-pushed the chambart:split_backend_primitives branch from 22ae9d7 to bb412a4 May 4, 2018
@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

Rebased

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

@alainfrisch @xavierleroy would you object to this ? Otherwise this is ready to merge.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

Well, I tried to read the diff. It's so big that I gave up. I'm fine with the idea; it's just that someone else will have to check all those changes and vouch that they are correct and not excessively intrusive.

BTW: I see a .ml file containing type definitions only, and an identical .ml file. What's the policy du jour concerning those? It's no longer OK to have only the .mli? or only the .ml?

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2018

It's problematic to have only the mli. It's ok to have only the ml, but the Makefiles do not seem to handle that well. Maybe a symlink is the right way to go.

@chambart chambart force-pushed the chambart:split_backend_primitives branch from bb412a4 to a217b22 May 5, 2018
@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2018

There was some rebasing error. Pushed a cleaned version.

@chambart chambart force-pushed the chambart:split_backend_primitives branch from c1379ba to e67c2e2 May 30, 2018
@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

Rebased

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented May 30, 2018

I will review this once the CI passes.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

Notice that it might be easier to review the patches separately rather than the complete diff.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

On 64bits the build failure is a binary that is different in bytecode and native.
But on 32bits there is a real error with the string_access test. I'll look into this.

@chambart chambart force-pushed the chambart:split_backend_primitives branch from 2dfbe28 to 8c47eb0 Feb 11, 2019
@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Rebased on trunk. (thanks @lthls)

@lthls
lthls approved these changes Feb 12, 2019
Copy link
Contributor

left a comment

I've reviewed the implementation, and I believe it is correct.

The actual changes in the rest of the code are minimal: the primitives for Lambda are left untouched, and the new type introduced for Clambda primitives is the same as the Lambda one except for the following changes:

  • The Pstring_load_*, Pbytes_load_*, Pbytes_set_* primitives and their Bigstring equivalents have been merged into primitives taking a size argument (and a variant argument for safety instead of a boolean).
  • The primitives Pidentity, Pbytes_to_string, Pbytes_of_string, Pctconst, Pignore, Prevapply, Pdirapply, Pgetglobal and Psetglobal have been removed. A new primitive Pread_symbol is added to replace Pgetglobal, the others are expected to be dealt with when translating from Lambda (most were already dealt with anyway).

The rest are mostly mechanical changes induced by the change in types.

This pull request is rather important for simplifying other pull requests on the backend, so it would be helpful to get it merged early in the 4.09 cycle.

@chambart chambart force-pushed the chambart:split_backend_primitives branch from 36a4d05 to 80ad15d Feb 12, 2019
@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Moved Changes entry to trunk section.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

@alainfrisch Do you have any objection ? Otherwise I think this is good to go.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

I haven't reviewed the PR, but I have no objection in principle.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

This has already been reviewed, that was the kind of non objection I expected.

@chambart chambart merged commit 1e6c739 into ocaml:trunk Feb 14, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2019

Currently the Change entry for this GPR says:

Add a separate types for clambda primitives

Is there onlyone type of are there several of them?

Could the change entry be fixed accordingly, please?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

ping @chambart

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