Updated [[musttail]] patchkit

Updated [[musttail]] patchkit

[Andi Kleen]

Fixed all earlier review comments.

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

[Andi Kleen]

- 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");
   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");
+      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");
+      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");
 		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

Re: [PATCH v3 1/5] Improve must tail in RTL backend

[Marek Polacek]

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

Re: [PATCH v3 1/5] Improve must tail in RTL backend

[Andi Kleen]

> 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

Re: [PATCH v3 1/5] Improve must tail in RTL backend

[Marek Polacek]

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

[PATCH v3 2/5] C++: Support clang compatible [[musttail]] (PR83324)

[Andi Kleen]

This patch implements a clang compatible [[musttail]] attribute for
returns.

musttail is useful as an alternative to computed goto for interpreters.
With computed goto the interpreter function usually ends up very big
which causes problems with register allocation and other per function
optimizations not scaling. With musttail the interpreter can be instead
written as a sequence of smaller functions that call each other. To
avoid unbounded stack growth this requires forcing a sibling call, which
this attribute does. It guarantees an error if the call cannot be tail
called which allows the programmer to fix it instead of risking a stack
overflow. Unlike computed goto it is also type-safe.

It turns out that David Malcolm had already implemented middle/backend
support for a musttail attribute back in 2016, but it wasn't exposed
to any frontend other than a special plugin.

This patch adds a [[gnu::musttail]] attribute for C++ that can be added
to return statements. The return statement must be a direct call
(it does not follow dependencies), which is similar to what clang
implements. It then uses the existing must tail infrastructure.

For compatibility it also detects clang::musttail

One problem is that tree-tailcall usually fails when optimization
is disabled, which implies the attribute only really works with
optimization on. But that seems to be a reasonable limitation.

Passes bootstrap and full test
---
 gcc/cp/cp-tree.h    |  4 ++--
 gcc/cp/parser.cc    | 28 +++++++++++++++++++++++-----
 gcc/cp/semantics.cc |  6 +++---
 gcc/cp/typeck.cc    | 20 ++++++++++++++++++--
 4 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 60e6dafc5494..bed52e860a00 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7763,7 +7763,7 @@ extern void finish_while_stmt			(tree);
 extern tree begin_do_stmt			(void);
 extern void finish_do_body			(tree);
 extern void finish_do_stmt		(tree, tree, bool, tree, bool);
-extern tree finish_return_stmt			(tree);
+extern tree finish_return_stmt			(tree, bool = false);
 extern tree begin_for_scope			(tree *);
 extern tree begin_for_stmt			(tree, tree);
 extern void finish_init_stmt			(tree);
@@ -8275,7 +8275,7 @@ extern tree composite_pointer_type		(const op_location_t &,
 						 tsubst_flags_t);
 extern tree merge_types				(tree, tree);
 extern tree strip_array_domain			(tree);
-extern tree check_return_expr			(tree, bool *, bool *);
+extern tree check_return_expr			(tree, bool *, bool *, bool);
 extern tree spaceship_type			(tree, tsubst_flags_t = tf_warning_or_error);
 extern tree genericize_spaceship		(location_t, tree, tree, tree);
 extern tree cp_build_binary_op                  (const op_location_t &,
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 3748ccd49ff3..5a32804c0201 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -2462,7 +2462,7 @@ static tree cp_parser_perform_range_for_lookup
 static tree cp_parser_range_for_member_function
   (tree, tree);
 static tree cp_parser_jump_statement
-  (cp_parser *);
+  (cp_parser *, bool = false);
 static void cp_parser_declaration_statement
   (cp_parser *);
 
@@ -12719,9 +12719,27 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 						     NULL_TREE, false);
 	  break;
 
+	case RID_RETURN:
+	  {
+	    bool musttail_p = false;
+	    std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
+	    if (lookup_attribute ("", "musttail", std_attrs))
+	      {
+		musttail_p = true;
+		std_attrs = remove_attribute ("", "musttail", std_attrs);
+	      }
+	    // support this for compatibility
+	    if (lookup_attribute ("clang", "musttail", std_attrs))
+	      {
+		musttail_p = true;
+		std_attrs = remove_attribute ("clang", "musttail", std_attrs);
+	      }
+	    statement = cp_parser_jump_statement (parser, musttail_p);
+	  }
+	  break;
+
 	case RID_BREAK:
 	case RID_CONTINUE:
-	case RID_RETURN:
 	case RID_CO_RETURN:
 	case RID_GOTO:
 	  std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
@@ -14767,7 +14785,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
   return false;
 }
 
-/* Parse a jump-statement.
+/* Parse a jump-statement. MUSTTAIL_P indicates a musttail attribute.
 
    jump-statement:
      break ;
@@ -14785,7 +14803,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
    Returns the new BREAK_STMT, CONTINUE_STMT, RETURN_EXPR, or GOTO_EXPR.  */
 
 static tree
-cp_parser_jump_statement (cp_parser* parser)
+cp_parser_jump_statement (cp_parser* parser, bool musttail_p)
 {
   tree statement = error_mark_node;
   cp_token *token;
@@ -14869,7 +14887,7 @@ cp_parser_jump_statement (cp_parser* parser)
 	else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt)
 	  /* Don't deduce from a discarded return statement.  */;
 	else
-	  statement = finish_return_stmt (expr);
+	  statement = finish_return_stmt (expr, musttail_p);
 	/* Look for the final `;'.  */
 	cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
       }
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 3299e2704465..a277f70ea0fd 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -1324,16 +1324,16 @@ finish_do_stmt (tree cond, tree do_stmt, bool ivdep, tree unroll,
 }
 
 /* Finish a return-statement.  The EXPRESSION returned, if any, is as
-   indicated.  */
+   indicated.  MUSTTAIL_P indicates a mustcall attribute.  */
 
 tree
-finish_return_stmt (tree expr)
+finish_return_stmt (tree expr, bool musttail_p)
 {
   tree r;
   bool no_warning;
   bool dangling;
 
-  expr = check_return_expr (expr, &no_warning, &dangling);
+  expr = check_return_expr (expr, &no_warning, &dangling, musttail_p);
 
   if (error_operand_p (expr)
       || (flag_openmp && !check_omp_return ()))
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index a15eda3f5f8c..8c116e3b4f4c 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -11028,10 +11028,12 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
    the DECL_RESULT for the function.  Set *NO_WARNING to true if
    code reaches end of non-void function warning shouldn't be issued
    on this RETURN_EXPR.  Set *DANGLING to true if code returns the
-   address of a local variable.  */
+   address of a local variable.  MUSTTAIL_P indicates a musttail
+   return.  */
 
 tree
-check_return_expr (tree retval, bool *no_warning, bool *dangling)
+check_return_expr (tree retval, bool *no_warning, bool *dangling,
+		   bool musttail_p)
 {
   tree result;
   /* The type actually returned by the function.  */
@@ -11045,6 +11047,20 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling)
   *no_warning = false;
   *dangling = false;
 
+  if (musttail_p)
+    {
+      if (TREE_CODE (retval) == TARGET_EXPR
+	  && TREE_CODE (TARGET_EXPR_INITIAL (retval)) == CALL_EXPR)
+	CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_INITIAL (retval)) = 1;
+      else if (TREE_CODE (retval) != CALL_EXPR)
+	{
+	  error_at (loc, "cannot tail-call: return value must be a call");
+	  return error_mark_node;
+	}
+      else
+	CALL_EXPR_MUST_TAIL_CALL (retval) = 1;
+    }
+
   /* A `volatile' function is one that isn't supposed to return, ever.
      (This is a G++ extension, used to get better code for functions
      that call the `volatile' function.)  */
-- 
2.43.0

Re: [PATCH v3 2/5] C++: Support clang compatible [[musttail]] (PR83324)

[Marek Polacek]

On Tue, Jan 30, 2024 at 06:17:15PM -0800, Andi Kleen wrote:
> This patch implements a clang compatible [[musttail]] attribute for
> returns.
> 
> musttail is useful as an alternative to computed goto for interpreters.
> With computed goto the interpreter function usually ends up very big
> which causes problems with register allocation and other per function
> optimizations not scaling. With musttail the interpreter can be instead
> written as a sequence of smaller functions that call each other. To
> avoid unbounded stack growth this requires forcing a sibling call, which
> this attribute does. It guarantees an error if the call cannot be tail
> called which allows the programmer to fix it instead of risking a stack
> overflow. Unlike computed goto it is also type-safe.
> 
> It turns out that David Malcolm had already implemented middle/backend
> support for a musttail attribute back in 2016, but it wasn't exposed
> to any frontend other than a special plugin.
> 
> This patch adds a [[gnu::musttail]] attribute for C++ that can be added
> to return statements. The return statement must be a direct call
> (it does not follow dependencies), which is similar to what clang
> implements. It then uses the existing must tail infrastructure.
>
> For compatibility it also detects clang::musttail

FWIW, it's not clear to me we should do this.  I don't see a precedent.

> One problem is that tree-tailcall usually fails when optimization
> is disabled, which implies the attribute only really works with
> optimization on. But that seems to be a reasonable limitation.
> 
> Passes bootstrap and full test

I don't see a ChangeLog entry.

> ---
>  gcc/cp/cp-tree.h    |  4 ++--
>  gcc/cp/parser.cc    | 28 +++++++++++++++++++++++-----
>  gcc/cp/semantics.cc |  6 +++---
>  gcc/cp/typeck.cc    | 20 ++++++++++++++++++--
>  4 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 60e6dafc5494..bed52e860a00 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -7763,7 +7763,7 @@ extern void finish_while_stmt			(tree);
>  extern tree begin_do_stmt			(void);
>  extern void finish_do_body			(tree);
>  extern void finish_do_stmt		(tree, tree, bool, tree, bool);
> -extern tree finish_return_stmt			(tree);
> +extern tree finish_return_stmt			(tree, bool = false);
>  extern tree begin_for_scope			(tree *);
>  extern tree begin_for_stmt			(tree, tree);
>  extern void finish_init_stmt			(tree);
> @@ -8275,7 +8275,7 @@ extern tree composite_pointer_type		(const op_location_t &,
>  						 tsubst_flags_t);
>  extern tree merge_types				(tree, tree);
>  extern tree strip_array_domain			(tree);
> -extern tree check_return_expr			(tree, bool *, bool *);
> +extern tree check_return_expr			(tree, bool *, bool *, bool);
>  extern tree spaceship_type			(tree, tsubst_flags_t = tf_warning_or_error);
>  extern tree genericize_spaceship		(location_t, tree, tree, tree);
>  extern tree cp_build_binary_op                  (const op_location_t &,
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index 3748ccd49ff3..5a32804c0201 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -2462,7 +2462,7 @@ static tree cp_parser_perform_range_for_lookup
>  static tree cp_parser_range_for_member_function
>    (tree, tree);
>  static tree cp_parser_jump_statement
> -  (cp_parser *);
> +  (cp_parser *, bool = false);
>  static void cp_parser_declaration_statement
>    (cp_parser *);
>  
> @@ -12719,9 +12719,27 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
>  						     NULL_TREE, false);
>  	  break;
>  
> +	case RID_RETURN:
> +	  {
> +	    bool musttail_p = false;
> +	    std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
> +	    if (lookup_attribute ("", "musttail", std_attrs))
> +	      {
> +		musttail_p = true;
> +		std_attrs = remove_attribute ("", "musttail", std_attrs);
> +	      }
> +	    // support this for compatibility
> +	    if (lookup_attribute ("clang", "musttail", std_attrs))
> +	      {
> +		musttail_p = true;
> +		std_attrs = remove_attribute ("clang", "musttail", std_attrs);
> +	      }

Doing lookup_attribute unconditionally twice seems like a lot.
You could do just lookup_attribute ("musttail", std_attrs) and then
check get_attribute_namespace() == nullptr/gnu_identifier?

It's not pretty that you have to remove_attribute but I guess we emit
warnings otherwise?

> +	    statement = cp_parser_jump_statement (parser, musttail_p);
> +	  }
> +	  break;
> +
>  	case RID_BREAK:
>  	case RID_CONTINUE:
> -	case RID_RETURN:
>  	case RID_CO_RETURN:
>  	case RID_GOTO:
>  	  std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
> @@ -14767,7 +14785,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
>    return false;
>  }
>  
> -/* Parse a jump-statement.
> +/* Parse a jump-statement. MUSTTAIL_P indicates a musttail attribute.

Two spaces after a '.'.

>  
>     jump-statement:
>       break ;
> @@ -14785,7 +14803,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
>     Returns the new BREAK_STMT, CONTINUE_STMT, RETURN_EXPR, or GOTO_EXPR.  */
>  
>  static tree
> -cp_parser_jump_statement (cp_parser* parser)
> +cp_parser_jump_statement (cp_parser* parser, bool musttail_p)
>  {
>    tree statement = error_mark_node;
>    cp_token *token;
> @@ -14869,7 +14887,7 @@ cp_parser_jump_statement (cp_parser* parser)
>  	else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt)
>  	  /* Don't deduce from a discarded return statement.  */;
>  	else
> -	  statement = finish_return_stmt (expr);
> +	  statement = finish_return_stmt (expr, musttail_p);
>  	/* Look for the final `;'.  */
>  	cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
>        }
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 3299e2704465..a277f70ea0fd 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -1324,16 +1324,16 @@ finish_do_stmt (tree cond, tree do_stmt, bool ivdep, tree unroll,
>  }
>  
>  /* Finish a return-statement.  The EXPRESSION returned, if any, is as
> -   indicated.  */
> +   indicated.  MUSTTAIL_P indicates a mustcall attribute.  */
>  
>  tree
> -finish_return_stmt (tree expr)
> +finish_return_stmt (tree expr, bool musttail_p)
>  {
>    tree r;
>    bool no_warning;
>    bool dangling;
>  
> -  expr = check_return_expr (expr, &no_warning, &dangling);
> +  expr = check_return_expr (expr, &no_warning, &dangling, musttail_p);
>  
>    if (error_operand_p (expr)
>        || (flag_openmp && !check_omp_return ()))
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index a15eda3f5f8c..8c116e3b4f4c 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -11028,10 +11028,12 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>     the DECL_RESULT for the function.  Set *NO_WARNING to true if
>     code reaches end of non-void function warning shouldn't be issued
>     on this RETURN_EXPR.  Set *DANGLING to true if code returns the
> -   address of a local variable.  */
> +   address of a local variable.  MUSTTAIL_P indicates a musttail
> +   return.  */
>  
>  tree
> -check_return_expr (tree retval, bool *no_warning, bool *dangling)
> +check_return_expr (tree retval, bool *no_warning, bool *dangling,
> +		   bool musttail_p)
>  {
>    tree result;
>    /* The type actually returned by the function.  */
> @@ -11045,6 +11047,20 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling)
>    *no_warning = false;
>    *dangling = false;
>  
> +  if (musttail_p)
> +    {
> +      if (TREE_CODE (retval) == TARGET_EXPR
> +	  && TREE_CODE (TARGET_EXPR_INITIAL (retval)) == CALL_EXPR)
> +	CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_INITIAL (retval)) = 1;
> +      else if (TREE_CODE (retval) != CALL_EXPR)
> +	{

This won't handle [[gnu::musttail]] return (1, foo ()); which I guess is fine
since clang++ doesn't accept that either, but musttail-invalid.c doesn't check
the case (and it's C only).

> +	  error_at (loc, "cannot tail-call: return value must be a call");
> +	  return error_mark_node;
> +	}
> +      else
> +	CALL_EXPR_MUST_TAIL_CALL (retval) = 1;
> +    }

IMHO nicer it would be to do

      tree t = retval;
      if (TREE_CODE (t) == TARGET_EXPR)
        t = TARGET_EXPR_INITIAL (t);
      if (TREE_CODE (t) == CALL_EXPR)
        CALL_EXPR_MUST_TAIL_CALL (t) = true;
      else
        {
          error_at (loc, "cannot tail-call: return value must be a call");
          return error_mark_node;
        }

so that you can set CALL_EXPR_MUST_TAIL_CALL in one place.

Marek

Re: [PATCH v3 2/5] C++: Support clang compatible [[musttail]] (PR83324)

[Andi Kleen]

> > For compatibility it also detects clang::musttail
> 
> FWIW, it's not clear to me we should do this.  I don't see a precedent.

It would make existing code just work (as long as they don't use ifdef)

> 
> > One problem is that tree-tailcall usually fails when optimization
> > is disabled, which implies the attribute only really works with
> > optimization on. But that seems to be a reasonable limitation.
> > 
> > Passes bootstrap and full test
> 
> I don't see a ChangeLog entry.

I have them, but will add them to the next post.
> >  static void cp_parser_declaration_statement
> >    (cp_parser *);
> >  
> > @@ -12719,9 +12719,27 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
> >  						     NULL_TREE, false);
> >  	  break;
> >  
> > +	case RID_RETURN:
> > +	  {
> > +	    bool musttail_p = false;
> > +	    std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
> > +	    if (lookup_attribute ("", "musttail", std_attrs))
> > +	      {
> > +		musttail_p = true;
> > +		std_attrs = remove_attribute ("", "musttail", std_attrs);
> > +	      }
> > +	    // support this for compatibility
> > +	    if (lookup_attribute ("clang", "musttail", std_attrs))
> > +	      {
> > +		musttail_p = true;
> > +		std_attrs = remove_attribute ("clang", "musttail", std_attrs);
> > +	      }
> 
> Doing lookup_attribute unconditionally twice seems like a lot.
> You could do just lookup_attribute ("musttail", std_attrs) and then
> check get_attribute_namespace() == nullptr/gnu_identifier?

Actually the common case is 0 and very rarely 1 attribute, and in that it is both 
very cheap. If people ever write code with lots of attributes
per line we can worry about optimizations, but at this point it would
see premature.


> 
> It's not pretty that you have to remove_attribute but I guess we emit
> warnings otherwise?

Yes. 


-Andi

Re: [PATCH v3 2/5] C++: Support clang compatible [[musttail]] (PR83324)

[Jakub Jelinek]

On Wed, Jan 31, 2024 at 12:21:38PM -0800, Andi Kleen wrote:
> > > +	case RID_RETURN:
> > > +	  {
> > > +	    bool musttail_p = false;
> > > +	    std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
> > > +	    if (lookup_attribute ("", "musttail", std_attrs))
> > > +	      {
> > > +		musttail_p = true;
> > > +		std_attrs = remove_attribute ("", "musttail", std_attrs);
> > > +	      }

Using "" looks wrong to me, that is for standard attributes which
are also gnu attributes, say [[noreturn]]/[[gnu::noreturn]].
That is not the case here.  Even the __attribute__((musttail)) form will have
gnu namespace.

> > > +	    // support this for compatibility
> > > +	    if (lookup_attribute ("clang", "musttail", std_attrs))
> > > +	      {
> > > +		musttail_p = true;
> > > +		std_attrs = remove_attribute ("clang", "musttail", std_attrs);
> > > +	      }
> > 
> > Doing lookup_attribute unconditionally twice seems like a lot.
> > You could do just lookup_attribute ("musttail", std_attrs) and then
> > check get_attribute_namespace() == nullptr/gnu_identifier?

I agree with Marek here.  The fact that it is most often NULL std_attrs is
indeed already optimized by lookup_attribute, but people write all kinds of
code.  The remove_attribute can be done separately of course.

Though, I'd also prefer not to add clang attributes, just add gnu ones.

	Jakub

Re: [PATCH v3 2/5] C++: Support clang compatible [[musttail]] (PR83324)

[Joseph Myers]

On Wed, 31 Jan 2024, Jakub Jelinek wrote:

> On Wed, Jan 31, 2024 at 12:21:38PM -0800, Andi Kleen wrote:
> > > > +	case RID_RETURN:
> > > > +	  {
> > > > +	    bool musttail_p = false;
> > > > +	    std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
> > > > +	    if (lookup_attribute ("", "musttail", std_attrs))
> > > > +	      {
> > > > +		musttail_p = true;
> > > > +		std_attrs = remove_attribute ("", "musttail", std_attrs);
> > > > +	      }
> 
> Using "" looks wrong to me, that is for standard attributes which
> are also gnu attributes, say [[noreturn]]/[[gnu::noreturn]].
> That is not the case here.  Even the __attribute__((musttail)) form will have
> gnu namespace.

And it's incorrect to use [[musttail]] (C23 syntax, no namespace) in any 
circumstances, at least for C, as it's not a standard attribute - so tests 
should verify that [[musttail]] is diagnosed as ignored even in contexts 
where [[gnu::musttail]] is valid.  (It can't be standardized as 
[[musttail]] because of the rule that standard attributes must be 
ignorable; the proposed syntax for a TS and possible future 
standardization after that is "return goto".)

-- 
Joseph S. Myers
josmyers@redhat.com

[PATCH v3 3/5] C: Implement musttail attribute for returns

[Andi Kleen]

Implement a C23 clang compatible musttail attribute similar to the earlier
C++ implementation in the C parser.
---
 gcc/c/c-parser.cc | 61 +++++++++++++++++++++++++++++++++++++----------
 gcc/c/c-tree.h    |  2 +-
 gcc/c/c-typeck.cc | 15 ++++++++++--
 3 files changed, 63 insertions(+), 15 deletions(-)

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index c31349dae2ff..76931931e270 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -1616,6 +1616,11 @@ struct omp_for_parse_data {
   bool fail : 1;
 };
 
+struct attr_state
+{
+  bool musttail_p; // parsed a musttail for return
+};
+
 static bool c_parser_nth_token_starts_std_attributes (c_parser *,
 						      unsigned int);
 static tree c_parser_std_attribute_specifier_sequence (c_parser *);
@@ -1660,7 +1665,7 @@ static location_t c_parser_compound_statement_nostart (c_parser *);
 static void c_parser_label (c_parser *, tree);
 static void c_parser_statement (c_parser *, bool *, location_t * = NULL);
 static void c_parser_statement_after_labels (c_parser *, bool *,
-					     vec<tree> * = NULL);
+					     vec<tree> * = NULL, attr_state = {});
 static tree c_parser_c99_block_statement (c_parser *, bool *,
 					  location_t * = NULL);
 static void c_parser_if_statement (c_parser *, bool *, vec<tree> *);
@@ -5757,6 +5762,8 @@ c_parser_std_attribute (c_parser *parser, bool for_tm,
 	}
       goto out;
     }
+  else if (is_attribute_p ("musttail", name))
+    error ("%<musttail%> attribute has arguments");
   {
     location_t open_loc = c_parser_peek_token (parser)->location;
     matching_parens parens;
@@ -6943,6 +6950,28 @@ c_parser_handle_directive_omp_attributes (tree &attrs,
     }
 }
 
+/* Check if STD_ATTR contains a musttail attribute and handle it
+   PARSER is the parser and A is the output attr_state.  */
+
+static tree
+c_parser_handle_musttail (c_parser *parser, tree std_attrs, attr_state &a)
+{
+  if (c_parser_next_token_is_keyword (parser, RID_RETURN))
+    {
+      if (lookup_attribute ("gnu", "musttail", std_attrs))
+	{
+	  std_attrs = remove_attribute ("gnu", "musttail", std_attrs);
+	  a.musttail_p = true;
+	}
+      if (lookup_attribute ("clang", "musttail", std_attrs))
+	{
+	  std_attrs = remove_attribute ("clang", "musttail", std_attrs);
+	  a.musttail_p = true;
+	}
+    }
+  return std_attrs;
+}
+
 /* Parse a compound statement except for the opening brace.  This is
    used for parsing both compound statements and statement expressions
    (which follow different paths to handling the opening).  */
@@ -6959,6 +6988,7 @@ c_parser_compound_statement_nostart (c_parser *parser)
   bool in_omp_loop_block
     = omp_for_parse_state ? omp_for_parse_state->want_nested_loop : false;
   tree sl = NULL_TREE;
+  attr_state a = {};
 
   if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE))
     {
@@ -7097,7 +7127,10 @@ c_parser_compound_statement_nostart (c_parser *parser)
 	= c_parser_nth_token_starts_std_attributes (parser, 1);
       tree std_attrs = NULL_TREE;
       if (have_std_attrs)
-	std_attrs = c_parser_std_attribute_specifier_sequence (parser);
+	{
+	  std_attrs = c_parser_std_attribute_specifier_sequence (parser);
+	  std_attrs = c_parser_handle_musttail (parser, std_attrs, a);
+	}
       if (c_parser_next_token_is_keyword (parser, RID_CASE)
 	  || c_parser_next_token_is_keyword (parser, RID_DEFAULT)
 	  || (c_parser_next_token_is (parser, CPP_NAME)
@@ -7245,7 +7278,7 @@ c_parser_compound_statement_nostart (c_parser *parser)
 	  last_stmt = true;
 	  mark_valid_location_for_stdc_pragma (false);
 	  if (!omp_for_parse_state)
-	    c_parser_statement_after_labels (parser, NULL);
+	    c_parser_statement_after_labels (parser, NULL, NULL, a);
 	  else
 	    {
 	      /* In canonical loop nest form, nested loops can only appear
@@ -7287,15 +7320,18 @@ c_parser_compound_statement_nostart (c_parser *parser)
 /* Parse all consecutive labels, possibly preceded by standard
    attributes.  In this context, a statement is required, not a
    declaration, so attributes must be followed by a statement that is
-   not just a semicolon.  */
+   not just a semicolon.  Returns an attr_state.  */
 
-static void
+static attr_state
 c_parser_all_labels (c_parser *parser)
 {
+  attr_state a = {};
   bool have_std_attrs;
   tree std_attrs = NULL;
   if ((have_std_attrs = c_parser_nth_token_starts_std_attributes (parser, 1)))
-    std_attrs = c_parser_std_attribute_specifier_sequence (parser);
+    std_attrs = c_parser_handle_musttail (parser,
+		    c_parser_std_attribute_specifier_sequence (parser), a);
+
   while (c_parser_next_token_is_keyword (parser, RID_CASE)
 	 || c_parser_next_token_is_keyword (parser, RID_DEFAULT)
 	 || (c_parser_next_token_is (parser, CPP_NAME)
@@ -7317,6 +7353,7 @@ c_parser_all_labels (c_parser *parser)
     }
   else if (have_std_attrs && c_parser_next_token_is (parser, CPP_SEMICOLON))
     c_parser_error (parser, "expected statement");
+  return a;
 }
 
 /* Parse a label (C90 6.6.1, C99 6.8.1, C11 6.8.1).
@@ -7560,11 +7597,11 @@ c_parser_label (c_parser *parser, tree std_attrs)
 static void
 c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels)
 {
-  c_parser_all_labels (parser);
+  attr_state a = c_parser_all_labels (parser);
   if (loc_after_labels)
     *loc_after_labels = c_parser_peek_token (parser)->location;
   parser->omp_attrs_forbidden_p = false;
-  c_parser_statement_after_labels (parser, if_p, NULL);
+  c_parser_statement_after_labels (parser, if_p, NULL, a);
 }
 
 /* Parse a statement, other than a labeled statement.  CHAIN is a vector
@@ -7573,11 +7610,11 @@ c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels)
 
    IF_P is used to track whether there's a (possibly labeled) if statement
    which is not enclosed in braces and has an else clause.  This is used to
-   implement -Wparentheses.  */
+   implement -Wparentheses. A has an earlier parsed attribute state.  */
 
 static void
 c_parser_statement_after_labels (c_parser *parser, bool *if_p,
-				 vec<tree> *chain)
+				 vec<tree> *chain, attr_state a)
 {
   location_t loc = c_parser_peek_token (parser)->location;
   tree stmt = NULL_TREE;
@@ -7645,7 +7682,7 @@ c_parser_statement_after_labels (c_parser *parser, bool *if_p,
 	  c_parser_consume_token (parser);
 	  if (c_parser_next_token_is (parser, CPP_SEMICOLON))
 	    {
-	      stmt = c_finish_return (loc, NULL_TREE, NULL_TREE);
+	      stmt = c_finish_return (loc, NULL_TREE, NULL_TREE, a.musttail_p);
 	      c_parser_consume_token (parser);
 	    }
 	  else
@@ -7654,7 +7691,7 @@ c_parser_statement_after_labels (c_parser *parser, bool *if_p,
 	      struct c_expr expr = c_parser_expression_conv (parser);
 	      mark_exp_read (expr.value);
 	      stmt = c_finish_return (EXPR_LOC_OR_LOC (expr.value, xloc),
-				      expr.value, expr.original_type);
+				      expr.value, expr.original_type, a.musttail_p);
 	      goto expect_semicolon;
 	    }
 	  break;
diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h
index cf29534c0915..902cc8f6aa49 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -824,7 +824,7 @@ extern tree c_begin_stmt_expr (void);
 extern tree c_finish_stmt_expr (location_t, tree);
 extern tree c_process_expr_stmt (location_t, tree);
 extern tree c_finish_expr_stmt (location_t, tree);
-extern tree c_finish_return (location_t, tree, tree);
+extern tree c_finish_return (location_t, tree, tree, bool = false);
 extern tree c_finish_bc_stmt (location_t, tree, bool);
 extern tree c_finish_goto_label (location_t, tree);
 extern tree c_finish_goto_ptr (location_t, c_expr val);
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 66c6abc9f076..144b001e3a6f 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -11422,10 +11422,10 @@ c_finish_goto_ptr (location_t loc, c_expr val)
    to return, or a null pointer for `return;' with no value.  LOC is
    the location of the return statement, or the location of the expression,
    if the statement has any.  If ORIGTYPE is not NULL_TREE, it
-   is the original type of RETVAL.  */
+   is the original type of RETVAL.  MUSTTAIL_P indicates a musttail attribute.  */
 
 tree
-c_finish_return (location_t loc, tree retval, tree origtype)
+c_finish_return (location_t loc, tree retval, tree origtype, bool musttail_p)
 {
   tree valtype = TREE_TYPE (TREE_TYPE (current_function_decl)), ret_stmt;
   bool no_warning = false;
@@ -11439,6 +11439,17 @@ c_finish_return (location_t loc, tree retval, tree origtype)
     warning_at (xloc, 0,
 		"function declared %<noreturn%> has a %<return%> statement");
 
+  if (retval && musttail_p)
+    {
+      if (TREE_CODE (retval) == CALL_EXPR)
+	CALL_EXPR_MUST_TAIL_CALL (retval) = 1;
+      else if (TREE_CODE (retval) == TARGET_EXPR
+	       && TREE_CODE (TARGET_EXPR_INITIAL (retval)) == CALL_EXPR)
+	CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_INITIAL (retval)) = 1;
+      else
+	error_at (xloc, "cannot tail-call: return value must be call");
+    }
+
   if (retval)
     {
       tree semantic_type = NULL_TREE;
-- 
2.43.0

[PATCH v3 4/5] Add tests for C/C++ musttail attributes

[Andi Kleen]

Mostly adopted from the existing C musttail plugin tests.
---
 gcc/testsuite/c-c++-common/musttail1.c  | 17 ++++++++++++
 gcc/testsuite/c-c++-common/musttail2.c  | 36 +++++++++++++++++++++++++
 gcc/testsuite/c-c++-common/musttail3.c  | 31 +++++++++++++++++++++
 gcc/testsuite/c-c++-common/musttail4.c  | 19 +++++++++++++
 gcc/testsuite/gcc.dg/musttail-invalid.c | 17 ++++++++++++
 5 files changed, 120 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/musttail1.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail2.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail3.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail4.c
 create mode 100644 gcc/testsuite/gcc.dg/musttail-invalid.c

diff --git a/gcc/testsuite/c-c++-common/musttail1.c b/gcc/testsuite/c-c++-common/musttail1.c
new file mode 100644
index 000000000000..476185e3ed4b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-options "-O2" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
+
+int __attribute__((noinline,noclone))
+callee (int i)
+{
+  return i * i;
+}
+
+int __attribute__((noinline,noclone))
+caller (int i)
+{
+  [[gnu::musttail]] return callee (i + 1);
+}
diff --git a/gcc/testsuite/c-c++-common/musttail2.c b/gcc/testsuite/c-c++-common/musttail2.c
new file mode 100644
index 000000000000..28f2f68ef13d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail2.c
@@ -0,0 +1,36 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+
+struct box { char field[256]; int i; };
+
+int __attribute__((noinline,noclone))
+test_2_callee (int i, struct box b)
+{
+  if (b.field[0])
+    return 5;
+  return i * i;
+}
+
+int __attribute__((noinline,noclone))
+test_2_caller (int i)
+{
+  struct box b;
+  [[gnu::musttail]] return test_2_callee (i + 1, b); /* { dg-error "cannot tail-call: " } */
+}
+
+extern void setjmp (void);
+void
+test_3 (void)
+{
+  [[gnu::musttail]] return setjmp (); /* { dg-error "cannot tail-call: " } */
+}
+
+typedef void (fn_ptr_t) (void);
+volatile fn_ptr_t fn_ptr;
+
+void
+test_5 (void)
+{
+  [[gnu::musttail]] return fn_ptr (); /* { dg-error "cannot tail-call: " } */
+}
diff --git a/gcc/testsuite/c-c++-common/musttail3.c b/gcc/testsuite/c-c++-common/musttail3.c
new file mode 100644
index 000000000000..fdbb292944ad
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail3.c
@@ -0,0 +1,31 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+
+extern int foo2 (int x, ...);
+
+struct str
+{
+  int a, b;
+};
+
+struct str
+cstruct (int x)
+{
+  if (x < 10)
+    [[clang::musttail]] return cstruct (x + 1);
+  return ((struct str){ x, 0 });
+}
+
+int
+foo (int x)
+{
+  if (x < 10)
+    [[clang::musttail]] return foo2 (x, 29);
+  if (x < 100)
+    {
+      int k = foo (x + 1);
+      [[clang::musttail]] return k;	/* { dg-error "cannot tail-call: " } */
+    }
+  return x;
+}
diff --git a/gcc/testsuite/c-c++-common/musttail4.c b/gcc/testsuite/c-c++-common/musttail4.c
new file mode 100644
index 000000000000..7bf44816f14a
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail4.c
@@ -0,0 +1,19 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+
+struct box { char field[64]; int i; };
+
+struct box __attribute__((noinline,noclone))
+returns_struct (int i)
+{
+  struct box b;
+  b.i = i * i;
+  return b;
+}
+
+int __attribute__((noinline,noclone))
+test_1 (int i)
+{
+  [[gnu::musttail]] return returns_struct (i * 5).i; /* { dg-error "cannot tail-call: " } */
+}
diff --git a/gcc/testsuite/gcc.dg/musttail-invalid.c b/gcc/testsuite/gcc.dg/musttail-invalid.c
new file mode 100644
index 000000000000..c4725b4b8226
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/musttail-invalid.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c23" } */
+
+[[musttail]] int j; /* { dg-warning "attribute ignored" } */
+__attribute__((musttail)) int k; /* { dg-warning "attribute directive ignored" } */
+
+void foo(void)
+{
+	[[musttail]] j++; /* { dg-warning "attribute ignored" } */
+	[[musttail]] if (k > 0) /* { dg-warning "attribute ignored" } */
+		[[musttail]] k++; /* { dg-warning "attribute ignored" } */
+}
+
+int foo2(int p)
+{
+	[[gnu::musttail(1)]] return foo2(p + 1); /* { dg-error "has arguments" } */
+}
-- 
2.43.0

Re: [PATCH v3 4/5] Add tests for C/C++ musttail attributes

[Prathamesh Kulkarni]

On Wed, 31 Jan 2024 at 07:49, Andi Kleen <ak@linux.intel.com> wrote:
>
> Mostly adopted from the existing C musttail plugin tests.
> ---
>  gcc/testsuite/c-c++-common/musttail1.c  | 17 ++++++++++++
>  gcc/testsuite/c-c++-common/musttail2.c  | 36 +++++++++++++++++++++++++
>  gcc/testsuite/c-c++-common/musttail3.c  | 31 +++++++++++++++++++++
>  gcc/testsuite/c-c++-common/musttail4.c  | 19 +++++++++++++
>  gcc/testsuite/gcc.dg/musttail-invalid.c | 17 ++++++++++++
>  5 files changed, 120 insertions(+)
>  create mode 100644 gcc/testsuite/c-c++-common/musttail1.c
>  create mode 100644 gcc/testsuite/c-c++-common/musttail2.c
>  create mode 100644 gcc/testsuite/c-c++-common/musttail3.c
>  create mode 100644 gcc/testsuite/c-c++-common/musttail4.c
>  create mode 100644 gcc/testsuite/gcc.dg/musttail-invalid.c
>
> diff --git a/gcc/testsuite/c-c++-common/musttail1.c b/gcc/testsuite/c-c++-common/musttail1.c
> new file mode 100644
> index 000000000000..476185e3ed4b
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail1.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-options "-O2" } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
> +
> +int __attribute__((noinline,noclone))
Hi,
Sorry to nitpick -- Just wondering if it'd be slightly better to use
noipa attribute instead, assuming the intent is to disable IPA opts ?

Thanks,
Prathamesh


> +callee (int i)
> +{
> +  return i * i;
> +}
> +
> +int __attribute__((noinline,noclone))
> +caller (int i)
> +{
> +  [[gnu::musttail]] return callee (i + 1);
> +}
> diff --git a/gcc/testsuite/c-c++-common/musttail2.c b/gcc/testsuite/c-c++-common/musttail2.c
> new file mode 100644
> index 000000000000..28f2f68ef13d
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail2.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +
> +struct box { char field[256]; int i; };
> +
> +int __attribute__((noinline,noclone))
> +test_2_callee (int i, struct box b)
> +{
> +  if (b.field[0])
> +    return 5;
> +  return i * i;
> +}
> +
> +int __attribute__((noinline,noclone))
> +test_2_caller (int i)
> +{
> +  struct box b;
> +  [[gnu::musttail]] return test_2_callee (i + 1, b); /* { dg-error "cannot tail-call: " } */
> +}
> +
> +extern void setjmp (void);
> +void
> +test_3 (void)
> +{
> +  [[gnu::musttail]] return setjmp (); /* { dg-error "cannot tail-call: " } */
> +}
> +
> +typedef void (fn_ptr_t) (void);
> +volatile fn_ptr_t fn_ptr;
> +
> +void
> +test_5 (void)
> +{
> +  [[gnu::musttail]] return fn_ptr (); /* { dg-error "cannot tail-call: " } */
> +}
> diff --git a/gcc/testsuite/c-c++-common/musttail3.c b/gcc/testsuite/c-c++-common/musttail3.c
> new file mode 100644
> index 000000000000..fdbb292944ad
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail3.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +
> +extern int foo2 (int x, ...);
> +
> +struct str
> +{
> +  int a, b;
> +};
> +
> +struct str
> +cstruct (int x)
> +{
> +  if (x < 10)
> +    [[clang::musttail]] return cstruct (x + 1);
> +  return ((struct str){ x, 0 });
> +}
> +
> +int
> +foo (int x)
> +{
> +  if (x < 10)
> +    [[clang::musttail]] return foo2 (x, 29);
> +  if (x < 100)
> +    {
> +      int k = foo (x + 1);
> +      [[clang::musttail]] return k;    /* { dg-error "cannot tail-call: " } */
> +    }
> +  return x;
> +}
> diff --git a/gcc/testsuite/c-c++-common/musttail4.c b/gcc/testsuite/c-c++-common/musttail4.c
> new file mode 100644
> index 000000000000..7bf44816f14a
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail4.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +
> +struct box { char field[64]; int i; };
> +
> +struct box __attribute__((noinline,noclone))
> +returns_struct (int i)
> +{
> +  struct box b;
> +  b.i = i * i;
> +  return b;
> +}
> +
> +int __attribute__((noinline,noclone))
> +test_1 (int i)
> +{
> +  [[gnu::musttail]] return returns_struct (i * 5).i; /* { dg-error "cannot tail-call: " } */
> +}
> diff --git a/gcc/testsuite/gcc.dg/musttail-invalid.c b/gcc/testsuite/gcc.dg/musttail-invalid.c
> new file mode 100644
> index 000000000000..c4725b4b8226
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/musttail-invalid.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c23" } */
> +
> +[[musttail]] int j; /* { dg-warning "attribute ignored" } */
> +__attribute__((musttail)) int k; /* { dg-warning "attribute directive ignored" } */
> +
> +void foo(void)
> +{
> +       [[musttail]] j++; /* { dg-warning "attribute ignored" } */
> +       [[musttail]] if (k > 0) /* { dg-warning "attribute ignored" } */
> +               [[musttail]] k++; /* { dg-warning "attribute ignored" } */
> +}
> +
> +int foo2(int p)
> +{
> +       [[gnu::musttail(1)]] return foo2(p + 1); /* { dg-error "has arguments" } */
> +}
> --
> 2.43.0
>

Re: [PATCH v3 4/5] Add tests for C/C++ musttail attributes

[Marek Polacek]

On Tue, Jan 30, 2024 at 06:17:17PM -0800, Andi Kleen wrote:
> Mostly adopted from the existing C musttail plugin tests.

Please add a ChangeLog entry.

> ---
>  gcc/testsuite/c-c++-common/musttail1.c  | 17 ++++++++++++
>  gcc/testsuite/c-c++-common/musttail2.c  | 36 +++++++++++++++++++++++++
>  gcc/testsuite/c-c++-common/musttail3.c  | 31 +++++++++++++++++++++
>  gcc/testsuite/c-c++-common/musttail4.c  | 19 +++++++++++++
>  gcc/testsuite/gcc.dg/musttail-invalid.c | 17 ++++++++++++
>  5 files changed, 120 insertions(+)
>  create mode 100644 gcc/testsuite/c-c++-common/musttail1.c
>  create mode 100644 gcc/testsuite/c-c++-common/musttail2.c
>  create mode 100644 gcc/testsuite/c-c++-common/musttail3.c
>  create mode 100644 gcc/testsuite/c-c++-common/musttail4.c
>  create mode 100644 gcc/testsuite/gcc.dg/musttail-invalid.c
> 
> diff --git a/gcc/testsuite/c-c++-common/musttail1.c b/gcc/testsuite/c-c++-common/musttail1.c
> new file mode 100644
> index 000000000000..476185e3ed4b
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail1.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-options "-O2" } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */

This will run the test only once with -std=c++11.  We'll get better coverage
with dropping the line above and using

/* { dg-do compile { target { tail_call && { c || c++11 } } } } */

but here it may not matter.

> +/* { dg-additional-options "-std=c23" { target c } } */
> +/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
> +
> +int __attribute__((noinline,noclone))
> +callee (int i)
> +{
> +  return i * i;
> +}
> +
> +int __attribute__((noinline,noclone))
> +caller (int i)
> +{
> +  [[gnu::musttail]] return callee (i + 1);
> +}
> diff --git a/gcc/testsuite/c-c++-common/musttail2.c b/gcc/testsuite/c-c++-common/musttail2.c
> new file mode 100644
> index 000000000000..28f2f68ef13d
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail2.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +
> +struct box { char field[256]; int i; };
> +
> +int __attribute__((noinline,noclone))
> +test_2_callee (int i, struct box b)
> +{
> +  if (b.field[0])
> +    return 5;
> +  return i * i;
> +}
> +
> +int __attribute__((noinline,noclone))
> +test_2_caller (int i)
> +{
> +  struct box b;
> +  [[gnu::musttail]] return test_2_callee (i + 1, b); /* { dg-error "cannot tail-call: " } */
> +}
> +
> +extern void setjmp (void);
> +void
> +test_3 (void)
> +{
> +  [[gnu::musttail]] return setjmp (); /* { dg-error "cannot tail-call: " } */
> +}
> +
> +typedef void (fn_ptr_t) (void);
> +volatile fn_ptr_t fn_ptr;
> +
> +void
> +test_5 (void)
> +{
> +  [[gnu::musttail]] return fn_ptr (); /* { dg-error "cannot tail-call: " } */
> +}
> diff --git a/gcc/testsuite/c-c++-common/musttail3.c b/gcc/testsuite/c-c++-common/musttail3.c
> new file mode 100644
> index 000000000000..fdbb292944ad
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail3.c
> @@ -0,0 +1,31 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +
> +extern int foo2 (int x, ...);
> +
> +struct str
> +{
> +  int a, b;
> +};
> +
> +struct str
> +cstruct (int x)
> +{
> +  if (x < 10)
> +    [[clang::musttail]] return cstruct (x + 1);
> +  return ((struct str){ x, 0 });
> +}
> +
> +int
> +foo (int x)
> +{
> +  if (x < 10)
> +    [[clang::musttail]] return foo2 (x, 29);
> +  if (x < 100)
> +    {
> +      int k = foo (x + 1);
> +      [[clang::musttail]] return k;	/* { dg-error "cannot tail-call: " } */
> +    }
> +  return x;
> +}
> diff --git a/gcc/testsuite/c-c++-common/musttail4.c b/gcc/testsuite/c-c++-common/musttail4.c
> new file mode 100644
> index 000000000000..7bf44816f14a
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/musttail4.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile { target tail_call } } */
> +/* { dg-additional-options "-std=c++11" { target c++ } } */
> +/* { dg-additional-options "-std=c23" { target c } } */
> +
> +struct box { char field[64]; int i; };
> +
> +struct box __attribute__((noinline,noclone))
> +returns_struct (int i)
> +{
> +  struct box b;
> +  b.i = i * i;
> +  return b;
> +}
> +
> +int __attribute__((noinline,noclone))
> +test_1 (int i)
> +{
> +  [[gnu::musttail]] return returns_struct (i * 5).i; /* { dg-error "cannot tail-call: " } */
> +}
> diff --git a/gcc/testsuite/gcc.dg/musttail-invalid.c b/gcc/testsuite/gcc.dg/musttail-invalid.c
> new file mode 100644
> index 000000000000..c4725b4b8226
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/musttail-invalid.c

Is there a C++ test for the invalid cases?

> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-std=c23" } */
> +
> +[[musttail]] int j; /* { dg-warning "attribute ignored" } */
> +__attribute__((musttail)) int k; /* { dg-warning "attribute directive ignored" } */
> +
> +void foo(void)
> +{
> +	[[musttail]] j++; /* { dg-warning "attribute ignored" } */
> +	[[musttail]] if (k > 0) /* { dg-warning "attribute ignored" } */
> +		[[musttail]] k++; /* { dg-warning "attribute ignored" } */
> +}
> +
> +int foo2(int p)
> +{
> +	[[gnu::musttail(1)]] return foo2(p + 1); /* { dg-error "has arguments" } */
> +}
> -- 
> 2.43.0
> 

Marek

Re: [PATCH v3 4/5] Add tests for C/C++ musttail attributes

[Andi Kleen]

> This will run the test only once with -std=c++11.  We'll get better coverage
> with dropping the line above and using
> 
> /* { dg-do compile { target { tail_call && { c || c++11 } } } } */
> 
> but here it may not matter.

The problem is that older C/C++ standards don't support [[]] attributes.
It would make sense to say >= gnu++11 || >= c23 but I don't know how to
express that.

Re: [PATCH v3 4/5] Add tests for C/C++ musttail attributes

[Marek Polacek]

On Wed, Jan 31, 2024 at 11:39:56PM -0800, Andi Kleen wrote:
> > This will run the test only once with -std=c++11.  We'll get better coverage
> > with dropping the line above and using
> > 
> > /* { dg-do compile { target { tail_call && { c || c++11 } } } } */
> > 
> > but here it may not matter.
> 
> The problem is that older C/C++ standards don't support [[]] attributes.
> It would make sense to say >= gnu++11 || >= c23 but I don't know how to
> express that.
 
That's what the c++11 target does.  Unfortunately for C we don't have
anything like that, hence the additional-options adding c23.

Marek

Re: [PATCH v3 4/5] Add tests for C/C++ musttail attributes

[Joseph Myers]

On Thu, 1 Feb 2024, Marek Polacek wrote:

> On Wed, Jan 31, 2024 at 11:39:56PM -0800, Andi Kleen wrote:
> > > This will run the test only once with -std=c++11.  We'll get better coverage
> > > with dropping the line above and using
> > > 
> > > /* { dg-do compile { target { tail_call && { c || c++11 } } } } */
> > > 
> > > but here it may not matter.
> > 
> > The problem is that older C/C++ standards don't support [[]] attributes.
> > It would make sense to say >= gnu++11 || >= c23 but I don't know how to
> > express that.
>  
> That's what the c++11 target does.  Unfortunately for C we don't have
> anything like that, hence the additional-options adding c23.

For C we don't loop over the testsuite building with different standard 
options - such a conditional only makes sense for C++ in the context of 
building the same tests several times for different standards, when some 
tests need to be conditional on the standard for which they are built.

-- 
Joseph S. Myers
josmyers@redhat.com

[PATCH v3 5/5] Add documentation for musttail attribute

[Andi Kleen]

---
 gcc/doc/extend.texi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 0bc586d120e7..1d413b0ef6d1 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -9867,6 +9867,22 @@ foo (int x, int y)
 @code{y} is not actually incremented and the compiler can but does not
 have to optimize it to just @code{return 42 + 42;}.
 
+@cindex @code{musttail} statement attribute
+@item musttail
+
+The @code{gnu::musttail} or @code{clang::musttail} attribute
+can be applied to a return statement that returns the value
+of a call to indicate that the call must be a tail call
+that does not allocate extra stack space.
+
+@smallexample
+[[gnu::musttail]] return foo();
+@end smallexample
+
+If the compiler cannot generate a tail call it will generate
+an error. Tail calls generally require enabling optimization.
+On some targets they may not be supported.
+
 @end table
 
 @node Attribute Syntax
-- 
2.43.0

Updated musttail patchkit

[Andi Kleen]

This version fixes C++ template propagation of musttail, avoids ICEs for
ARM (and probably some other targets) generation of -O0 tail calls,
and improves the error messages in tree-musttail again, as well as
the documentation.

I bootstrapped/tested it on x86_64-linux, and checked the musttail tests
work on arm and riscv targets.

-O0 is still not as good as clang (e.g. it doesn't handle struct
returns), but I believe it's good enough for now to be usable.

Updated musttail patchkit

[Andi Kleen]

- Fix problems with encoding musttail in tree structure (Thanks Jakub and Jason)
- Fixes a miscompilation that would break bootstrap with --enable-checking=release
- Avoids a 0.8% compile time penalty at -O0 for the new musttail pass by using a cfun flag
that is discovered by tree-cfg
- Enables translation of musttail error messages
- Further improves error reporting, avoiding "other reasons" error messages
for various cases and reporting the correct error in others.
- Adjusted the test suite to powerpc sibcall limitations
- Addressed C++ review feedback
- Improves dump file output
- Improves the documentation
- Some random cleanups
- Rebased on trunk

Tested full bootstrap on x86_64-linux and powerpc64le-linux, as well
as a x86_64 LTO profiled bootstrap and some x86_64 testing with
--enable-release=checking.