Allow [@tailcall true] and [@tailcall false]
#9754
Conversation
|
Remark: a first cleanup commit in this PR, 66aba00, removes the One could wonder whether we should try harder to give backend-accurate tailcall information; this is (of course ?) unchanged by the present PR. (On the other hand, having code that raises a warning or not depending on the backend configuration is also bad for usability.) If we decide to do something in this area (no hurry!), I feel somewhat more motivated by the idea of trying harder to preserve tailcalls in presence of spilling, although that may also not be workable at all. |
|
This PR is a dependency of the new TRMC PR #9760, and it is short and easy to review independently. Having it merged would help reduce the diff of TRMC, and made that (larger) PR easier to review. Volunteers welcome! |
utils/warnings.ml
Outdated
| @@ -565,8 +565,9 @@ let message = function | |||
| | Bad_docstring unattached -> | |||
| if unattached then "unattached documentation comment (ignored)" | |||
| else "ambiguous documentation comment" | |||
| | Expect_tailcall -> | |||
| Printf.sprintf "expected tailcall" | |||
| | Expect_tailcall b -> | |||
There was a problem hiding this comment.
A better name for this constructor could be Expected_tailcall, and it would match up with the other *-ed_xxx warnings.
There was a problem hiding this comment.
I think your PR already renamed this constructor to Tailcall_expected. Maybe I should change it to Tailcall_expectation, now that there are both an "expected" and "not expected" case?
There was a problem hiding this comment.
Oops, you are right :) ! Tailcall_expected of bool seems fine... But if we are changing it, I find that Expected_tailcall reads a bit better.
There was a problem hiding this comment.
I tried with expected_tailcall and now warnings on @tailcall false read very confusing:
Line 3, characters 9-56:
3 | | n -> (fact_tail [@tailcall false]) (n * acc) (n - 1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning 51 [expected-tailcall]: expected non-tailcall
I will try again with Tailcall_expectation, which works better with both true and false.
lambda/lambda.ml
Outdated
| @@ -212,7 +212,8 @@ type structured_constant = | |||
| | Const_immstring of string | |||
|
|
|||
| type tailcall_attribute = | |||
| | Should_be_tailcall (* [@tailcall] *) | |||
| | Should_be_tailcall (* [@tailcall] or [@tailcall true] *) | |||
| | Should_not_be_tailcall (* [@tailcall false] *) | |||
There was a problem hiding this comment.
Perhaps these two constructors could be merged into a single one Expect_tailcall of bool? It would match up with the warning constructor and the call to maybe_warn below...
There was a problem hiding this comment.
Initially I didn't want to do this because I think that bool is not clear enough on what it means. (Does false mean that the implementation should try to disable tail-call optimization when we are in tailcall position?) But with Expect in the name of some form, it is sort of clear that the attribute semantics is (currently) only to report an expectation, so it is in fact equivalent to the current names, but more factorized. Will change.
|
This PR looks almost ready. What should we do with it? |
|
Ping. |
gasche
force-pushed
the
tailcall-attribute-boolean-trunk
branch
from
a201d95
to
77a307a
Compare
|
I just rebased it to take into account the comments by Nicolás; this could be good to go if the CI is happy, but I'm in the middle of a deadline (I pushed this to free my "trunk" repository for some other testing) and I'd like to double-check it before merging. |
|
I checked the PR and did a last commit, adding a testcase for the "invalid payload" case as suggested by @nojb. This should now be good to go if the CI is happy. |
gasche
force-pushed
the
tailcall-attribute-boolean-trunk
branch
from
0c15279
to
a1650d7
Compare
|
The dependencies need to be updated. |
This forward-reference from Lambda to Asmcomp was used to generate machine-specific tailcall information in -annot output; this only use was removed in 57d329e, so we can now remove it to simplify the codebase. The logic was non-trivial and might be useful again in the future.
(+ constructor renaming suggested by Nicolás during review)
(suggested by Nicolás during review)
gasche
force-pushed
the
tailcall-attribute-boolean-trunk
branch
from
a1650d7
to
e1aeb42
Compare
[@tailcall true]is equivalent to[@tailcall], and[@tailcall false]warns if the call is in tail-call position.This is build-up work for the upcoming "TRMC Reborn" PR, which will use these attributes to drive some parts of the TRMC transformation.