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

Allow non-val payload types in CMM Ccatch #1833

Merged
merged 1 commit into from Jun 28, 2018

Conversation

Projects
None yet
4 participants
@SimonJF
Copy link
Contributor

commented Jun 11, 2018

Summary

This patch adds explicit type annotations for Ccatch payloads in the CMM
IR, thus supporting payloads which are not of type val.

(Now with the x86 build problem fixed, and rebased on top of the most recent commit...)

Rationale

While CMM generated from OCaml source will always generate payloads of type
val, this is not the case when targeting CMM to compile from a different
language. As a concrete example, I am currently targeting the CMM backend for
compilation of WebAssembly, and require float payloads. Additionally, @mshinwell
has noted that such an extension will be useful for flambda2. As a result, this
patch will increase the applicability of the CMM IR as a compilation target.

Design

The design of the patch is as follows:

  1. Add explicit type annotations to Ccatch handlers. Specifically,
    Ccatch (int * (Ident.t list) * expression)
    becomes
    Ccatch (int * (Ident.t * machtype) list * expression)

  2. By default, in cmmgen, select typ_val as the machtype

  3. Select an appropriate register using the type annotation in selectgen,
    instead of defaulting to typ_val

  4. Test updates:

    • Update the CMM parser and pretty-printer to require annotations on Ccatch
      handlers
    • Update the existing CMM tests to add the required annotations
    • Add new tests which require the use of floating-point registers, for
      which the compiler would generate invalid ASM prior to this patch

Since all OCaml code will use typ_val as before in cmmgen and therefore
selectgen, compilation for OCaml programs will be identical.

@gasche

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

Note: you don't need to close and reopen a new pull request if you want to update your patchset, just use git push --force to update the branch on your personal github fork, and the PR will automatically get updated with the new HEAD.

@SimonJF

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

Thanks. I tried that originally but had some problems with the Travis scripts not running properly (perhaps because they've been updated recently). Sorry for the extra emails!

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

I did not review the PR yet, but I fully support the approach. I did a similar experiment ( trunk...alainfrisch:unbox_across_jumps ) also including some logic to actually unbox floats (and boxed integers) across static jumps, but it makes sense to include the infrastructure first before such heuristics.

@SimonJF

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2018

Great! I'm glad you agree with the approach. I think we're coming at this from different angles, which is promising since it seems to suggest the infrastructure would be useful for other purposes too.

The Travis seems to have failed on the changelog modification test which I'll modify when appropriate, but passed everything else.

@SimonJF SimonJF force-pushed the SimonJF:ccatch-types-pr-3 branch from 3f300c3 to d415191 Jun 15, 2018

@SimonJF

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

Would anyone be willing / available to review this?

@@ -185,7 +185,8 @@ let rec expr ppf = function
i
(fun ppf ids ->
List.iter
(fun id -> fprintf ppf " %a" Ident.print id)
(fun (id, ty) ->
fprintf ppf " %a: %a" Ident.print id machtype ty)

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 25, 2018

Contributor

For consistency, I would write "@ %a: %a" rather than " %a: %a".

(* CR-someday mshinwell: consider how we can do better than
[typ_val] when appropriate. *)
(fun id -> let r = self#regs_for typ_val in name_regs id r; r)
(fun (id, typ) -> let r = self#regs_for typ in name_regs id r; r)

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 25, 2018

Contributor

I am not completely sure the CR should be deleted: you are
adding support for non-val parameters, but all parameters are
still passed as values. Of course, the CR make little sense here,
maybe it should just be moved e.g. to your addition in Cmmgen.

This comment has been minimized.

Copy link
@SimonJF

SimonJF Jun 25, 2018

Author Contributor

I've now moved the CR to Cmmgen.

@@ -53,6 +53,17 @@ G(caml_c_call):
.comm G(young_ptr), 4
.comm G(young_start), 4

.data

This comment has been minimized.

Copy link
@xclerc

xclerc Jun 25, 2018

Contributor

This change might deserve a comment.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

The change looks good, I have only made some minor remarks.
I am a bit nervous regarding the float comparison in the tests, but
am not sure it would be worthwhile to spend more time on it.

@SimonJF SimonJF force-pushed the SimonJF:ccatch-types-pr-3 branch from d415191 to 8f1bcbc Jun 25, 2018

Allow non-val payload types in CMM Ccatch
Summary
-------
This patch adds explicit type annotations for Ccatch payloads in the CMM
IR, thus supporting payloads which are not of type `val`.

Rationale
---------
While CMM generated from OCaml source will always generate payloads of type
`val`, this is not the case when targeting CMM to compile from a different
language. As a concrete example, I am currently targeting the CMM backend for
compilation of WebAssembly, and require `float` payloads. Additionally, @mshinwell
has noted that such an extension will be useful for flambda2. As a result, this
patch will increase the applicability of the CMM IR as a compilation target.

Updates
-------

25/06/2018: Incorporate review comments by @xclerc

Design
------
The design of the patch is as follows:

  1. Add explicit type annotations to Ccatch handlers. Specifically,
        ```Ccatch (int * (Ident.t list) * expression)```
     becomes
        ```Ccatch (int * (Ident.t * machtype) list * expression)```

  2. By default, in `cmmgen`, select `typ_val` as the `machtype`

  3. Select an appropriate register using the type annotation in `selectgen`,
     instead of defaulting to `typ_val`

  4. Test updates:
     - Update the CMM parser and pretty-printer to require annotations on Ccatch
       handlers
     - Update the existing CMM tests to add the required annotations
     - Add new tests which require the use of floating-point registers, for
       which the compiler would generate invalid ASM prior to this patch

Since all OCaml code will use `typ_val` as before in `cmmgen` and therefore
`selectgen`, compilation for OCaml programs will be identical.

@SimonJF SimonJF force-pushed the SimonJF:ccatch-types-pr-3 branch from 8f1bcbc to efedf2e Jun 25, 2018

@SimonJF

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2018

Thanks for the review! I've made the requested changes.

@alainfrisch
Copy link
Contributor

left a comment

LGTM

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

Will merge soon if nobody else complains.

@alainfrisch alainfrisch merged commit ace6af8 into ocaml:trunk Jun 28, 2018

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
You can’t perform that action at this time.