From: Michael Matz <matz@suse.de>
To: Andi Kleen <ak@linux.intel.com>,
gcc-patches@gcc.gnu.org,
Richard Biener <richard.guenther@gmail.com>
Subject: Re: [PATCH v6 1/8] Improve must tail in RTL backend
Date: Wed, 29 May 2024 15:39:58 +0200 (CEST) [thread overview]
Message-ID: <357aa546-91d8-8d98-f941-ac2bdafab656@suse.de> (raw)
In-Reply-To: <20240521143203.2893096-2-ak@linux.intel.com>
On Tue, 21 May 2024, Andi Kleen wrote:
> - Give error messages for all causes of non sibling call generation
> - When giving error messages clear the musttail flag to avoid ICEs
> - Error out when tree-tailcall failed to mark a must-tail call
> sibcall. In this case it doesn't know the true reason and only gives
> a vague message.
Sorry for jumping in late, Richi triggered me :) But some general
remarks:
I think the ultimate knowledge if a call can or cannot be implemented as
tail-call lies within calls.cc/expand_call: It is inherently
target and ABI specific how arguments and returns are layed out, how the
stack frame is generated, if arguments are or aren't removed by callers
or callees and so on; all of that being knowledge that tree-tailcall
doesn't have and doesn't want to have. As such tree-tailcall should
not be regarded as ultimate truth, and failures of tree-tailcall to
recognize something as tail-callable shouldn't matter.
It then follows that tree-tailcall needn't be run at -O0 merely for
setting the flag. Instead calls.cc simply should try expanding a
tail-call when it sees the must-tail flag (as it right now would do), i.e.
trust the user. If that fails for some reasons then that means that the
checks within calls.cc aren't complete enough (and that tree-tailcall
papered over that problem). That would be (IMHO) an independend bug to be
solved. But _when_ those bugs are fixed then what you merely need to do
for the musttail attribute is to set that flag on the gimple_call,
possibly make sure that nothing (tree-tailcall!) removes the flag, and be
done.
(For avoidance of doubt: with tree-tailcall I mean the tree sibcall call
pass, "tailc", not the tail-recursion pass).
IOW: I don't see why the tree pass needs to be run at -O0 for musttail.
If something doesn't work currently then that points to other
deficiencies.
Ciao,
Michael.
>
> PR83324
>
> gcc/ChangeLog:
>
> * calls.cc (expand_call): Fix mustcall implementation.
> (maybe_complain_about_tail_call): Clear must tail flag on error.
> ---
> gcc/calls.cc | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/calls.cc b/gcc/calls.cc
> index 21d78f9779fe..161e36839654 100644
> --- a/gcc/calls.cc
> +++ b/gcc/calls.cc
> @@ -1249,6 +1249,7 @@ maybe_complain_about_tail_call (tree call_expr, const char *reason)
> return;
>
> error_at (EXPR_LOCATION (call_expr), "cannot tail-call: %s", reason);
> + CALL_EXPR_MUST_TAIL_CALL (call_expr) = 0;
> }
>
> /* Fill in ARGS_SIZE and ARGS array based on the parameters found in
> @@ -2650,7 +2651,11 @@ expand_call (tree exp, rtx target, int ignore)
> /* The type of the function being called. */
> tree fntype;
> bool try_tail_call = CALL_EXPR_TAILCALL (exp);
> - bool must_tail_call = CALL_EXPR_MUST_TAIL_CALL (exp);
> + /* tree-tailcall decided not to do tail calls. Error for the musttail case,
> + unfortunately we don't know the reason so it's fairly vague.
> + When tree-tailcall reported an error it already cleared the flag. */
> + if (!try_tail_call)
> + maybe_complain_about_tail_call (exp, "other reasons");
> int pass;
>
> /* Register in which non-BLKmode value will be returned,
> @@ -3022,10 +3027,21 @@ expand_call (tree exp, rtx target, int ignore)
> pushed these optimizations into -O2. Don't try if we're already
> expanding a call, as that means we're an argument. Don't try if
> there's cleanups, as we know there's code to follow the call. */
> - if (currently_expanding_call++ != 0
> - || (!flag_optimize_sibling_calls && !CALL_FROM_THUNK_P (exp))
> - || args_size.var
> - || dbg_cnt (tail_call) == false)
> + if (currently_expanding_call++ != 0)
> + {
> + maybe_complain_about_tail_call (exp, "inside another call");
> + try_tail_call = 0;
> + }
> + if (!flag_optimize_sibling_calls
> + && !CALL_FROM_THUNK_P (exp)
> + && !CALL_EXPR_MUST_TAIL_CALL (exp))
> + try_tail_call = 0;
> + if (args_size.var)
> + {
> + maybe_complain_about_tail_call (exp, "variable size arguments");
> + try_tail_call = 0;
> + }
> + if (dbg_cnt (tail_call) == false)
> try_tail_call = 0;
>
> /* Workaround buggy C/C++ wrappers around Fortran routines with
> @@ -3046,13 +3062,15 @@ expand_call (tree exp, rtx target, int ignore)
> if (MEM_P (*iter))
> {
> try_tail_call = 0;
> + maybe_complain_about_tail_call (exp,
> + "hidden string length argument passed on stack");
> break;
> }
> }
>
> /* If the user has marked the function as requiring tail-call
> optimization, attempt it. */
> - if (must_tail_call)
> + if (CALL_EXPR_MUST_TAIL_CALL (exp))
> try_tail_call = 1;
>
> /* Rest of purposes for tail call optimizations to fail. */
>
next prev parent reply other threads:[~2024-05-29 13:40 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-21 14:28 Musttail patchkit v6 Andi Kleen
2024-05-21 14:28 ` [PATCH v6 1/8] Improve must tail in RTL backend Andi Kleen
2024-05-29 13:39 ` Michael Matz [this message]
2024-05-31 18:00 ` Andi Kleen
2024-06-03 17:02 ` Michael Matz
2024-06-03 17:17 ` Jakub Jelinek
2024-06-04 13:49 ` Michael Matz
2024-06-03 17:31 ` Andi Kleen
2024-05-21 14:28 ` [PATCH v6 2/8] Add a musttail generic attribute to the c-attribs table Andi Kleen
2024-05-21 14:28 ` [PATCH v6 3/8] C++: Support clang compatible [[musttail]] (PR83324) Andi Kleen
2024-05-21 14:28 ` [PATCH v6 4/8] C: Implement musttail attribute for returns Andi Kleen
2024-05-21 14:28 ` [PATCH v6 5/8] Add tests for C/C++ musttail attributes Andi Kleen
2024-05-21 14:28 ` [PATCH v6 6/8] Enable musttail tail conversion even when not optimizing Andi Kleen
2024-05-21 14:28 ` [PATCH v6 7/8] Give better error messages for musttail Andi Kleen
2024-06-05 4:52 ` Andi Kleen
2024-05-21 14:28 ` [PATCH v6 8/8] Add documentation for musttail attribute Andi Kleen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=357aa546-91d8-8d98-f941-ac2bdafab656@suse.de \
--to=matz@suse.de \
--cc=ak@linux.intel.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).