Skip to content

Flambda. No, really.#415

Merged
chambart merged 1 commit intoocaml:trunkfrom
chambart:flambda_merge
Jan 28, 2016
Merged

Flambda. No, really.#415
chambart merged 1 commit intoocaml:trunkfrom
chambart:flambda_merge

Conversation

@mshinwell
Copy link
Copy Markdown
Contributor

This pull request contains the outstanding merge items for Flambda. We are continuing to split out a few more bits, but the end is nigh.

We are aware that some copyright headers need fixing in this diff.

There are also testsuite changes specific for Flambda which will come later. We need to arrange things so that certain tests are only run with Flambda or have different results with Flambda (or Closure).

@mshinwell mshinwell added this to the 4.03.0 milestone Jan 12, 2016
@mshinwell
Copy link
Copy Markdown
Contributor Author

Some existing warning numbers have been accidentally changed, we need to fix this.
(now fixed)

@mshinwell
Copy link
Copy Markdown
Contributor Author

If you would like to read this diff, please instead pull the following branches from github.com/chambart:

flambda_prereqs (= ocaml/trunk + outstanding GPRs)
flambda_merge (desired final state)

Then do e.g.:
git diff -r flambda_prereqs -r flambda_merge
and just ignore the ".depend" hunk at the top.

@chambart
Copy link
Copy Markdown
Contributor

I will squash this branch when the other PR will be merged. This will make the diff obvious.
And we probably don't want to see those commits in trunk history.

@mshinwell
Copy link
Copy Markdown
Contributor Author

Well, the diff should be obvious automatically as soon as the other PRs are merged.

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Jan 15, 2016

Yes, but a clean commit history would be really nice to have.

2016-01-15 14:08 GMT+00:00 Mark Shinwell notifications@github.com:

Well, the diff should be obvious automatically as soon as the other PRs
are merged.


Reply to this email directly or view it on GitHub
#415 (comment).

@mshinwell
Copy link
Copy Markdown
Contributor Author

The commit history is an orthogonal issue, but yes, we can squash that.

@chambart
Copy link
Copy Markdown
Contributor

I've just squashed the patch, the history is still availableas in the branch https://github.com/chambart/ocaml-1/tree/flambda_merge_before_squash

@mshinwell
Copy link
Copy Markdown
Contributor Author

Any comments on this patch are welcome.
Please note that we intend to tie [Backend] properly through to [Proc] before the release, but maybe not in this patch.

@mshinwell
Copy link
Copy Markdown
Contributor Author

Also, we will probably adjust some of the naming of command line arguments for inlining in flambda mode before the release.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems to be the only call to this function which is not protected by a check on flambda mode. Instead of doing the check inside the function, one could do it here to make it that nothing changes in non flambda mode.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is this information recorded? I cannot find such a place in this diff.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm currently cleaning the diff and that was part of some changes that were removed.

@chambart
Copy link
Copy Markdown
Contributor

The native toplevel will keep getting broken by some other patch if we don't add it to the testsuite. Should I add that to this patch ?
Note that some minimal testing is not commiting to support it, but this would avoid continuous unnoticed breakage by changing any somewhat related function.

@trefis
Copy link
Copy Markdown
Contributor

trefis commented Jan 27, 2016

The native toplevel will keep getting broken by some other patch if we don't add it to the testsuite. Should I add that to this patch ?

I agree with that assertion. But I think adding it to the testsuite is a separate concern than the one of this PR, so I would do another one.

@chambart
Copy link
Copy Markdown
Contributor

@damiendoligez, do you think that this is ready ?
If you approve, I think I will squash the recent changes in a single patch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason for the change from the previous symbol = Compilenv.make_symbol None to this one? (and current_unit_linkage_name is marked as flambda only)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not really, this is apparently what remained from removing closure, reintroducing it later while keeping the change on cmmgen interface, then going back on cmmgen interface...

@chambart
Copy link
Copy Markdown
Contributor

rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants