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

Extended static exceptions for 4.05 #524

Merged
merged 6 commits into from Oct 28, 2016

Conversation

chambart
Copy link
Contributor

This branch is an update of #98 for 4.04

This only contains the part about extending the Cmm Ccatch construction to allow recursive cases and the handling in further backend passes.

As noted by @xavierleroy in the other pull request, the name of this construction should change, but for patch maintanability, I'd prefer to do that in a separate patch if this one is accepted.

I didn't change it yet, but I'd like to require a rec annotation for recursive cases. Mainly for being more explicit and allow to be a bit faster in the passes that needs to reach a fixpoint in the recursive case.

Notice that now, registers are a bit more typed. Currently this patch only accept registers of type val as arguments (as before), but we might need to put anything in there (in particular values built with Ctuple). There are multiple possibilities to do that, but I don't want to finish implementing any before having received some feedback on what would be prefered.

  • Do the typing on Cmm. The drawback being that it almost requires to duplicates large parts of selectgen to propagate the same types everywhere.
  • Do the typing on Mach. I find that quite inelegant as this would require to either duplicate the mach to have a special version for 'typing' containing type variables, or change the Cmm.machtype to allow some type variable register (and pollute every pass with that).
  • Requite a type annotation on Ccatch like for function argument and application. (This is the solution I prefer) The drawback would be obviously that this requires a bit more work for the pass generating it (currently the transformation patch would obviously be just adding the Val annotation everywhere).

Note that for testing purpose I extended a bit the cmm parser to accept actual output of -dcmm and be a bit more convenient.

Nothing except the backend change is provided here, in particular, the generated code won't benefit for any optimisation yet. There is a small difference in the output of Selectgen as recursive cases might require an additionnal register move (as shown in the following swap example). When this is not required, the register allocator should coalesce the intermediate temporary registers and the generated code shouldn't differ.

(function swap (n : int a : val b : val)
  (catch (exit swap n a b)
   with (swap n a b)
     (if (<= n 0)
         a
         (exit swap (- n 1) b a))))
swap:
.L102:
    movq    %rax, %rsi
    movq    %rbx, %rax
.L100:
    cmpq    $0, %rsi
    jg          .L101
    ret
.L101:
    decq    %rsi
    movq    %rax, %rbx
    movq    %rdi, %rax
    movq    %rbx, %rdi
    jmp     .L100

The first thing to do after a potential merge of this is a cleanup: rename the constructors, remove the Cloop construction, then propagate those constructions up to flambda.

@chambart chambart force-pushed the recursive_static_exceptions branch from e24b932 to 7d2e76a Compare March 25, 2016 14:37
@chambart
Copy link
Contributor Author

Rebased on latest trunk.

@chambart chambart force-pushed the recursive_static_exceptions branch from 7d2e76a to f3fd41d Compare March 25, 2016 14:58
@damiendoligez damiendoligez added this to the 4.04 milestone Mar 31, 2016
@lpw25
Copy link
Contributor

lpw25 commented Apr 5, 2016

the name of this construction should change

This is pure bikeshedding, but I would suggest something like let_cont (for let continuation). Or possibly if the order is kept so that the continuation definition comes after its scope where_cont like the where construct in Haskell.

@mshinwell
Copy link
Contributor

(I am going to review this GPR in detail)

@mshinwell
Copy link
Contributor

We decided to leave the name change until later.

I've reviewed this carefully and discussed the details with Pierre. I have pushed code review comments to the recursive_static_exceptions branch on my Github account. Once Pierre attends to these, which are generally fairly minor things, I think this is OK to merge.

@xavierleroy Are you happy with this?

@chambart chambart force-pushed the recursive_static_exceptions branch from f3fd41d to b406daa Compare July 6, 2016 17:01
@chambart
Copy link
Contributor Author

chambart commented Jul 6, 2016

The changed discussed have been made.

Also rebased.

@chambart
Copy link
Contributor Author

chambart commented Jul 6, 2016

I forgot to push an update of the testsuite parser, here it is.

By the way, the travis CI rightfully failed, but not appveyor. Is it not running the testsuite ?
the build was: https://ci.appveyor.com/project/avsm/ocaml/build/1.0.685

Apparently it is skipping the asmcomp part of the testsuite. Is there any reason for that ?

@damiendoligez damiendoligez removed this from the 4.04 milestone Aug 2, 2016
@mshinwell
Copy link
Contributor

@chambart Can we get this into trunk?

@lefessan
Copy link
Contributor

No, it's not mergeable, there are conflicts that must be resolved...

@damiendoligez damiendoligez added this to the 4.05-or-later milestone Sep 29, 2016
@chambart chambart force-pushed the recursive_static_exceptions branch 2 times, most recently from 343ca8f to 02526fe Compare October 12, 2016 16:45
@chambart
Copy link
Contributor Author

This is now rebased on current trunk. The main conflics where comming from spacetime integration.

@chambart chambart changed the title Extended static exceptions for 4.04 Extended static exceptions for 4.05 Oct 12, 2016
@chambart
Copy link
Contributor Author

Rebased again against lastest trunk (cmm debuginfo conflicts mainly).

This branch clearly wins the title of my most rebased branche.

@mshinwell
Copy link
Contributor

I believe this is ok for 4.05 following previous discussions and code review.
I have also tested it recently by compiling "for" and "while" using recursive static exception handlers with no problems apparent.

@chambart chambart merged commit 008101e into ocaml:trunk Oct 28, 2016
mshinwell added a commit to mshinwell/ocaml that referenced this pull request Jul 1, 2021
stedolan pushed a commit to stedolan/ocaml that referenced this pull request May 24, 2022
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: Thibaut Mattio <thibaut.mattio@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants