[v9,07/10] Give better error messages for musttail

When musttail is set, make tree-tailcall give error messages
when it cannot handle a call. This avoids vague "other reasons"
error messages later at expand time when it sees a musttail
function not marked tail call.

In various cases this requires delaying the error until
the call is discovered.

Also print more information on the failure to the dump file.

gcc/ChangeLog:

	PR83324
	* tree-tailcall.cc (maybe_error_musttail): New function.
	(suitable_for_tail_opt_p): Report error reason.
	(suitable_for_tail_call_opt_p): Report error reason.
	(find_tail_calls): Accept basic blocks with abnormal edges.
        Delay reporting of errors until the call is discovered.
	Move top level suitability checks to here.
	(tree_optimize_tail_calls_1): Remove top level checks.
---
 gcc/tree-tailcall.cc | 187 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 154 insertions(+), 33 deletions(-)

On Mon, Jul 8, 2024 at 7:00 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> When musttail is set, make tree-tailcall give error messages
> when it cannot handle a call. This avoids vague "other reasons"
> error messages later at expand time when it sees a musttail
> function not marked tail call.
>
> In various cases this requires delaying the error until
> the call is discovered.
>
> Also print more information on the failure to the dump file.

This looks OK now.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         PR83324
>         * tree-tailcall.cc (maybe_error_musttail): New function.
>         (suitable_for_tail_opt_p): Report error reason.
>         (suitable_for_tail_call_opt_p): Report error reason.
>         (find_tail_calls): Accept basic blocks with abnormal edges.
>         Delay reporting of errors until the call is discovered.
>         Move top level suitability checks to here.
>         (tree_optimize_tail_calls_1): Remove top level checks.
> ---
>  gcc/tree-tailcall.cc | 187 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 154 insertions(+), 33 deletions(-)
>
> diff --git a/gcc/tree-tailcall.cc b/gcc/tree-tailcall.cc
> index 43e8c25215cb..a68079d4f507 100644
> --- a/gcc/tree-tailcall.cc
> +++ b/gcc/tree-tailcall.cc
> @@ -40,9 +40,11 @@ along with GCC; see the file COPYING3.  If not see
>  #include "tree-eh.h"
>  #include "dbgcnt.h"
>  #include "cfgloop.h"
> +#include "intl.h"
>  #include "common/common-target.h"
>  #include "ipa-utils.h"
>  #include "tree-ssa-live.h"
> +#include "diagnostic-core.h"
>
>  /* The file implements the tail recursion elimination.  It is also used to
>     analyze the tail calls in general, passing the results to the rtl level
> @@ -131,14 +133,20 @@ static tree m_acc, a_acc;
>
>  static bitmap tailr_arg_needs_copy;
>
> +static void maybe_error_musttail (gcall *call, const char *err);
> +
>  /* Returns false when the function is not suitable for tail call optimization
> -   from some reason (e.g. if it takes variable number of arguments).  */
> +   from some reason (e.g. if it takes variable number of arguments). CALL
> +   is call to report for.  */
>
>  static bool
> -suitable_for_tail_opt_p (void)
> +suitable_for_tail_opt_p (gcall *call)
>  {
>    if (cfun->stdarg)
> -    return false;
> +    {
> +      maybe_error_musttail (call, _("caller uses stdargs"));
> +      return false;
> +    }
>
>    return true;
>  }
> @@ -146,35 +154,47 @@ suitable_for_tail_opt_p (void)
>  /* Returns false when the function is not suitable for tail call optimization
>     for some reason (e.g. if it takes variable number of arguments).
>     This test must pass in addition to suitable_for_tail_opt_p in order to make
> -   tail call discovery happen.  */
> +   tail call discovery happen. CALL is call to report error for.  */
>
>  static bool
> -suitable_for_tail_call_opt_p (void)
> +suitable_for_tail_call_opt_p (gcall *call)
>  {
>    tree param;
>
>    /* alloca (until we have stack slot life analysis) inhibits
>       sibling call optimizations, but not tail recursion.  */
>    if (cfun->calls_alloca)
> -    return false;
> +    {
> +      maybe_error_musttail (call, _("caller uses alloca"));
> +      return false;
> +    }
>
>    /* If we are using sjlj exceptions, we may need to add a call to
>       _Unwind_SjLj_Unregister at exit of the function.  Which means
>       that we cannot do any sibcall transformations.  */
>    if (targetm_common.except_unwind_info (&global_options) == UI_SJLJ
>        && current_function_has_exception_handlers ())
> -    return false;
> +    {
> +      maybe_error_musttail (call, _("caller uses sjlj exceptions"));
> +      return false;
> +    }
>
>    /* Any function that calls setjmp might have longjmp called from
>       any called function.  ??? We really should represent this
>       properly in the CFG so that this needn't be special cased.  */
>    if (cfun->calls_setjmp)
> -    return false;
> +    {
> +      maybe_error_musttail (call, _("caller uses setjmp"));
> +      return false;
> +    }
>
>    /* Various targets don't handle tail calls correctly in functions
>       that call __builtin_eh_return.  */
>    if (cfun->calls_eh_return)
> -    return false;
> +    {
> +      maybe_error_musttail (call, _("caller uses __builtin_eh_return"));
> +      return false;
> +    }
>
>    /* ??? It is OK if the argument of a function is taken in some cases,
>       but not in all cases.  See PR15387 and PR19616.  Revisit for 4.1.  */
> @@ -182,7 +202,10 @@ suitable_for_tail_call_opt_p (void)
>         param;
>         param = DECL_CHAIN (param))
>      if (TREE_ADDRESSABLE (param))
> -      return false;
> +      {
> +       maybe_error_musttail (call, _("address of caller arguments taken"));
> +       return false;
> +      }
>
>    return true;
>  }
> @@ -402,16 +425,42 @@ propagate_through_phis (tree var, edge e)
>    return var;
>  }
>
> +/* Report an error for failing to tail convert must call CALL
> +   with error message ERR. Also clear the flag to prevent further
> +   errors.  */
> +
> +static void
> +maybe_error_musttail (gcall *call, const char *err)
> +{
> +  if (gimple_call_must_tail_p (call))
> +    {
> +      error_at (call->location, "cannot tail-call: %s", err);
> +      /* Avoid another error. ??? If there are multiple reasons why tail
> +        calls fail it might be useful to report them all to avoid
> +        whack-a-mole for the user. But currently there is too much
> +        redundancy in the reporting, so keep it simple.  */
> +      gimple_call_set_must_tail (call, false); /* Avoid another error.  */
> +      gimple_call_set_tail (call, false);
> +    }
> +  if (dump_file)
> +    {
> +      print_gimple_stmt (dump_file, call, 0, TDF_SLIM);
> +      fprintf (dump_file, "Cannot convert: %s\n", err);
> +    }
> +}
> +
>  /* Argument for compute_live_vars/live_vars_at_stmt and what compute_live_vars
>     returns.  Computed lazily, but just once for the function.  */
>  static live_vars_map *live_vars;
>  static vec<bitmap_head> live_vars_vec;
>
>  /* Finds tailcalls falling into basic block BB. The list of found tailcalls is
> -   added to the start of RET. When ONLY_MUSTTAIL is set only handle musttail.  */
> +   added to the start of RET. When ONLY_MUSTTAIL is set only handle musttail.
> +   Update OPT_TAILCALLS as output parameter.  */
>
>  static void
> -find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
> +find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail,
> +                bool &opt_tailcalls)
>  {
>    tree ass_var = NULL_TREE, ret_var, func, param;
>    gimple *stmt;
> @@ -426,8 +475,23 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>    tree var;
>
>    if (!single_succ_p (bb))
> -    return;
> +    {
> +      /* If there is an abnormal edge assume it's the only extra one.
> +        Tolerate that case so that we can give better error messages
> +        for musttail later.  */
> +      if (!has_abnormal_or_eh_outgoing_edge_p (bb))
> +       {
> +         if (dump_file)
> +           fprintf (dump_file, "Basic block %d has extra exit edges\n",
> +                           bb->index);
> +         return;
> +       }
> +      if (!cfun->has_musttail)
> +       return;
> +    }
>
> +  bool bad_stmt = false;
> +  gimple *last_stmt = nullptr;
>    for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
>      {
>        stmt = gsi_stmt (gsi);
> @@ -441,6 +505,8 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>           || is_gimple_debug (stmt))
>         continue;
>
> +      if (!last_stmt)
> +       last_stmt = stmt;
>        /* Check for a call.  */
>        if (is_gimple_call (stmt))
>         {
> @@ -448,6 +514,12 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>           /* Handle only musttail calls when not optimizing.  */
>           if (only_musttail && !gimple_call_must_tail_p (call))
>             return;
> +         if (bad_stmt)
> +           {
> +             maybe_error_musttail (call,
> +                             _("memory reference or volatile after call"));
> +             return;
> +           }
>           ass_var = gimple_call_lhs (call);
>           break;
>         }
> @@ -462,19 +534,37 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>        /* If the statement references memory or volatile operands, fail.  */
>        if (gimple_references_memory_p (stmt)
>           || gimple_has_volatile_ops (stmt))
> -       return;
> +       {
> +         if (dump_file)
> +           {
> +             fprintf (dump_file, "Cannot handle ");
> +             print_gimple_stmt (dump_file, stmt, 0);
> +           }
> +         bad_stmt = true;
> +         if (!cfun->has_musttail)
> +           break;
> +       }
>      }
>
> +  if (bad_stmt)
> +    return;
> +
>    if (gsi_end_p (gsi))
>      {
>        edge_iterator ei;
>        /* Recurse to the predecessors.  */
>        FOR_EACH_EDGE (e, ei, bb->preds)
> -       find_tail_calls (e->src, ret, only_musttail);
> +       find_tail_calls (e->src, ret, only_musttail, opt_tailcalls);
>
>        return;
>      }
>
> +  if (!suitable_for_tail_opt_p (call))
> +    return;
> +
> +  if (!suitable_for_tail_call_opt_p (call))
> +    opt_tailcalls = false;
> +
>    /* If the LHS of our call is not just a simple register or local
>       variable, we can't transform this into a tail or sibling call.
>       This situation happens, in (e.g.) "*p = foo()" where foo returns a
> @@ -489,13 +579,30 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>    if (ass_var
>        && !is_gimple_reg (ass_var)
>        && !auto_var_in_fn_p (ass_var, cfun->decl))
> -    return;
> +    {
> +      maybe_error_musttail (call, _("return value in memory"));
> +      return;
> +    }
> +
> +  if (cfun->calls_setjmp)
> +    {
> +      maybe_error_musttail (call, _("caller uses setjmp"));
> +      return;
> +    }
>
>    /* If the call might throw an exception that wouldn't propagate out of
>       cfun, we can't transform to a tail or sibling call (82081).  */
> -  if (stmt_could_throw_p (cfun, stmt)
> -      && !stmt_can_throw_external (cfun, stmt))
> +  if ((stmt_could_throw_p (cfun, stmt)
> +       && !stmt_can_throw_external (cfun, stmt)) || !single_succ_p (bb))
> +  {
> +    if (stmt == last_stmt)
> +      maybe_error_musttail (call,
> +                         _("call may throw exception that does not propagate"));
> +    else
> +      maybe_error_musttail (call,
> +                         _("code between call and return"));
>      return;
> +  }
>
>    /* If the function returns a value, then at present, the tail call
>       must return the same type of value.  There is conceptually a copy
> @@ -524,7 +631,10 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>    if (result_decl
>        && may_be_aliased (result_decl)
>        && ref_maybe_used_by_stmt_p (call, result_decl, false))
> -    return;
> +    {
> +      maybe_error_musttail (call, _("tail call must be same type"));
> +      return;
> +    }
>
>    /* We found the call, check whether it is suitable.  */
>    tail_recursion = false;
> @@ -605,6 +715,7 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>             {
>               if (local_live_vars)
>                 BITMAP_FREE (local_live_vars);
> +             maybe_error_musttail (call, _("call invocation refers to locals"));
>               return;
>             }
>           else
> @@ -613,6 +724,7 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>               if (bitmap_bit_p (local_live_vars, *v))
>                 {
>                   BITMAP_FREE (local_live_vars);
> +                 maybe_error_musttail (call, _("call invocation refers to locals"));
>                   return;
>                 }
>             }
> @@ -658,17 +770,21 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>         continue;
>
>        if (gimple_code (stmt) != GIMPLE_ASSIGN)
> -       return;
> +       {
> +         maybe_error_musttail (call, _("unhandled code after call"));
> +         return;
> +       }
>
>        /* This is a gimple assign. */
>        par ret = process_assignment (as_a <gassign *> (stmt), gsi,
>                                     &tmp_m, &tmp_a, &ass_var, to_move_defs);
> -      if (ret == FAIL)
> -       return;
> +      if (ret == FAIL || (ret == TRY_MOVE && !tail_recursion))
> +       {
> +         maybe_error_musttail (call, _("return value changed after call"));
> +         return;
> +       }
>        else if (ret == TRY_MOVE)
>         {
> -         if (! tail_recursion)
> -           return;
>           /* Do not deal with checking dominance, the real fix is to
>              do path isolation for the transform phase anyway, removing
>              the need to compute the accumulators with new stmts.  */
> @@ -716,16 +832,26 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
>    if (ret_var
>        && (ret_var != ass_var
>           && !(is_empty_type (TREE_TYPE (ret_var)) && !ass_var)))
> -    return;
> +    {
> +      maybe_error_musttail (call, _("call uses return slot"));
> +      return;
> +    }
>
>    /* If this is not a tail recursive call, we cannot handle addends or
>       multiplicands.  */
>    if (!tail_recursion && (m || a))
> -    return;
> +    {
> +      maybe_error_musttail (call, _("operations after non tail recursive call"));
> +      return;
> +    }
>
>    /* For pointers only allow additions.  */
>    if (m && POINTER_TYPE_P (TREE_TYPE (DECL_RESULT (current_function_decl))))
> -    return;
> +    {
> +      maybe_error_musttail (call,
> +                     _("tail recursion with pointers can only use additions"));
> +      return;
> +    }
>
>    /* Move queued defs.  */
>    if (tail_recursion)
> @@ -1112,17 +1238,12 @@ tree_optimize_tail_calls_1 (bool opt_tailcalls, bool only_mustcall)
>    tree param;
>    edge_iterator ei;
>
> -  if (!suitable_for_tail_opt_p ())
> -    return 0;
> -  if (opt_tailcalls)
> -    opt_tailcalls = suitable_for_tail_call_opt_p ();
> -
>    FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
>      {
>        /* Only traverse the normal exits, i.e. those that end with return
>          statement.  */
>        if (safe_is_a <greturn *> (*gsi_last_bb (e->src)))
> -       find_tail_calls (e->src, &tailcalls, only_mustcall);
> +       find_tail_calls (e->src, &tailcalls, only_mustcall, opt_tailcalls);
>      }
>
>    if (live_vars)
> --
> 2.45.2
>