[v3,1/5] Improve must tail in RTL backend

- Give error messages for all causes of non sibling call generation
- Don't override choices of other non sibling call checks with
must tail. This causes ICEs. The must tail attribute now only
overrides flag_optimize_sibling_calls locally.
- 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 (this could be improved, but it's already useful without
that) tree-tailcall usually fails without optimization, so must
adjust the existing must-tail plugin test to specify -O2.
---
 gcc/calls.cc                                  | 31 +++++++++++++------
 .../gcc.dg/plugin/must-tail-call-1.c          |  1 +
 2 files changed, 22 insertions(+), 10 deletions(-)

On Tue, Jan 30, 2024 at 06:17:14PM -0800, Andi Kleen wrote:
> - Give error messages for all causes of non sibling call generation
> - Don't override choices of other non sibling call checks with
> must tail. This causes ICEs. The must tail attribute now only
> overrides flag_optimize_sibling_calls locally.
> - 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 (this could be improved, but it's already useful without
> that) tree-tailcall usually fails without optimization, so must
> adjust the existing must-tail plugin test to specify -O2.
> ---
>  gcc/calls.cc                                  | 31 +++++++++++++------
>  .../gcc.dg/plugin/must-tail-call-1.c          |  1 +
>  2 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/gcc/calls.cc b/gcc/calls.cc
> index 01f447347437..3115807b7788 100644
> --- a/gcc/calls.cc
> +++ b/gcc/calls.cc
> @@ -2650,7 +2650,9 @@ 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.  */
> +  if (!try_tail_call)
> +      maybe_complain_about_tail_call (exp, "cannot tail-call: other reasons");

This results in "error: cannot tail-call: cannot tail-call: other reasons".
So the second argument should be "other reasons" only.

I notice that if I don't use -O2 I also get "other reasons".  But it should be
easy-ish to say "cannot tail-call: optimizations not enabled" or so.

>    int pass;
>  
>    /* Register in which non-BLKmode value will be returned,
> @@ -3021,10 +3023,22 @@ 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, "cannot tail-call: inside another call");

Duplicate "cannot tail-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)
> +    {
> +      /* ??? correct message?  */
> +      maybe_complain_about_tail_call (exp, "cannot tail-call: stack space needed");

Duplicate "cannot tail-call:".

> +      try_tail_call = 0;
> +    }
> +  if (dbg_cnt (tail_call) == false)
>      try_tail_call = 0;
>  
>    /* Workaround buggy C/C++ wrappers around Fortran routines with
> @@ -3045,15 +3059,12 @@ expand_call (tree exp, rtx target, int ignore)
>  	    if (MEM_P (*iter))
>  	      {
>  		try_tail_call = 0;
> +		maybe_complain_about_tail_call (exp,
> +				"cannot tail-call: hidden string length argument");

Duplicate "cannot tail-call:".  Formatting seems off.

>  		break;
>  	      }
>  	}
>  
> -  /* If the user has marked the function as requiring tail-call
> -     optimization, attempt it.  */
> -  if (must_tail_call)
> -    try_tail_call = 1;
> -
>    /*  Rest of purposes for tail call optimizations to fail.  */
>    if (try_tail_call)
>      try_tail_call = can_implement_as_sibling_call_p (exp,
> diff --git a/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c b/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c
> index 3a6d4cceaba7..44af361e2925 100644
> --- a/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c
> +++ b/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c
> @@ -1,4 +1,5 @@
>  /* { dg-do compile { target tail_call } } */
> +/* { dg-options "-O2" } */
>  /* { dg-options "-fdelayed-branch" { target sparc*-*-* } } */
>  
>  extern void abort (void);
> -- 
> 2.43.0
> 

Marek

> This results in "error: cannot tail-call: cannot tail-call: other reasons".
> So the second argument should be "other reasons" only.

Yes will fix those. Thanks.

> 
> I notice that if I don't use -O2 I also get "other reasons".  But it should be
> easy-ish to say "cannot tail-call: optimizations not enabled" or so.

It's unfortunately not easy to distinguish. It's not just -O2, but
various missing transformations make tree-tailcall not do it its job,
and they could depend on other flags. But there might be also other
reasons not related to the optimization that makes the tail call fall.
I would be uncomfortable reporting the problem is -O2 when it might 
be something else.

The right fix would be to make tree-tailcall not fail with optimization,
and for the remaining cases add errors there.  But that would
make the patch a lot bigger and it's not clear it would improve usability
that much. So I opted to just mention the problem in the documentation.

-Andi

On Wed, Jan 31, 2024 at 12:16:59PM -0800, Andi Kleen wrote:
> > This results in "error: cannot tail-call: cannot tail-call: other reasons".
> > So the second argument should be "other reasons" only.
> 
> Yes will fix those. Thanks.
> 
> > 
> > I notice that if I don't use -O2 I also get "other reasons".  But it should be
> > easy-ish to say "cannot tail-call: optimizations not enabled" or so.
> 
> It's unfortunately not easy to distinguish. It's not just -O2, but
> various missing transformations make tree-tailcall not do it its job,
> and they could depend on other flags. But there might be also other
> reasons not related to the optimization that makes the tail call fall.
> I would be uncomfortable reporting the problem is -O2 when it might 
> be something else.
> 
> The right fix would be to make tree-tailcall not fail with optimization,
> and for the remaining cases add errors there.  But that would
> make the patch a lot bigger and it's not clear it would improve usability
> that much. So I opted to just mention the problem in the documentation.

Ah, we don't want that.  I meant to check the simplest case:
  if (optimize < 2)
     ...
but if it's more complicated than that then let's let it be.

Thanks,
Marek