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

Remove loop constructors in Cmm and Mach #1965

Merged
merged 3 commits into from Feb 21, 2019

Conversation

@lthls
Copy link
Contributor

commented Aug 3, 2018

This is extracted from #1660 , where it was included as part of an unrelated bugfix.

The feature got some negative feedback from @xavierleroy , but @mshinwell and I would like the opportunity to defend it (and hear arguments against the feature).

The arguments I see for the removal:

  • It removes a bit of code, simplifying maintenance and future work.
  • The loop construction doesn't bring any advantage over the use of recursive catch handlers, which are already used anyway.

On the performance side, the limited testing I have done on the compiler's files show that the generated object files are identical in most cases (even restricting to files containing for/while loops), which is slightly unexpected but in line with my expectations that the produced code would be equivalent.

@SimonJF

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

I like the idea; in my experience of targeting CMM directly I've never had to use Cloop and have been a little surprised at its inclusion given that it can be macro-expressed using Ccatch.

Just a few questions that came to mind:

  1. Perhaps it would be nice to add a cloop helper function like the ccatch function in cmm.ml to abstract over the change.

  2. You currently allow while loops in CMM asmgen test files by treating while as syntactic sugar. I think this is fine, but I wonder about whether it might be more uniform to go a step further and remove the construct from the CMM parser altogether (although this would admittedly make the tests less readable).

  3. On the performance side -- have you done any testing with, say, deeply-nested loops, and a mixture of for and while loops? It might be worth looking at specially-constructed test cases as opposed to files from the compiler. You say that the generated object files are "identical in most cases" -- what about the ones that aren't?

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2018

@SimonJF The Ccatch (and Icatch) constructs didn't used to handle recursion at all, hence the inclusion of Cloop.

@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2018

Answering @SimonJF 's points:

  1. Ccatch before the change of type was used in a lot of places, and a simpler way to write non-recursive Ccatches with exactly one handler is still useful. Cloop was only used three times (four if you count the Cmm parser), and always directly below a Ccatch construct. I actually considered removing the create_loop helper function and transforming the outer Ccatches into recursive ones with two handlers, but the code paths dealing with more than one handler haven't been exercised much and I was afraid of finding bugs unrelated to the change.

  2. In the asmgen tests, the while construct was already treated as syntactic sugar and there is no loop keyword equivalent to the Cloop construction, which comforts me in my opinion that Cloop is not particularly important, but while loops are (in the Cmm language used to write the tests).

  3. After some deeper investigation, the only differences I saw in ocamlc's object files were in Format.pp_print_text, which contains a while loop, where register allocation swapped %rax and %rbx for the right and len variables.
    I wasn't sure that the order of the code blocks would be preserved, but it seems it is. It also seems that it didn't affect the numbering of the assembly labels, which means diffing .s files only showed the register swap.
    If you have examples that you think would be interesting to test, I can try them, but I'm not expecting differences other than the ones mentioned above.

@SimonJF

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2018

Thanks for the answers, all of which sound perfectly reasonable!

@lthls

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2018

I'd like to move forward with this PR.
We see it as some harmless simplification of the backend, and it should not be particularly hard to review.
From the comments in #1660 , we know that @xavierleroy didn't like the change, but we feel the proposition deserves at least a discussion.

@chambart chambart self-requested a review Feb 19, 2019
Copy link
Contributor

left a comment

This simplifies the code. I think this is good and the code is correct. I'm obviously linked to this so we might want a 'non-object' by @xavierleroy.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

No objections.

@chambart

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

@lthls Could you fix the Change file conflict by moving the line to the trunk section ?

@lthls lthls force-pushed the lthls:remove_iloop branch from 9034fd2 to 4126ca2 Feb 19, 2019
@chambart chambart merged commit 74182ae into ocaml:trunk Feb 21, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@chambart

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Thanks all.

gretay-js added a commit to gretay-js/ocaml that referenced this pull request Aug 26, 2019
Remove loop constructors in Cmm and Mach
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.