Musttail patchkit v6

Musttail patchkit v6

[Andi Kleen]

I reworked the musttail patchkit to make it work without optimization
and also give better error messages and fixed some bugs and increased
the test coverage.

The no optimization support is not perfect: it cannot handle returning
small structures which would need to be allocated to registers first.

There are also still cases when musttail cannot give an accurate
error message (mainly because of complex control flow making
tree-tailcall giving up). But overall it works significantly better.

Jason's feedback on the C++ parser is also addressed.

[PATCH v6 1/8] Improve must tail in RTL backend

[Andi Kleen]

- 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.

	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.  */
-- 
2.44.0

[PATCH v6 2/8] Add a musttail generic attribute to the c-attribs table

[Andi Kleen]

It does nothing currently since statement attributes are handled
directly in the parser.

gcc/c-family/ChangeLog:

	* c-attribs.cc (handle_musttail_attribute): Add.
	* c-common.h (handle_musttail_attribute): Add.
---
 gcc/c-family/c-attribs.cc | 15 +++++++++++++++
 gcc/c-family/c-common.h   |  1 +
 2 files changed, 16 insertions(+)

diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
index 04e39b41bdf3..7110d56c8ca0 100644
--- a/gcc/c-family/c-attribs.cc
+++ b/gcc/c-family/c-attribs.cc
@@ -338,6 +338,8 @@ const struct attribute_spec c_common_gnu_attributes[] =
   { "common",                 0, 0, true,  false, false, false,
 			      handle_common_attribute,
 	                      attr_common_exclusions },
+  { "musttail",		      0, 0, false, false, false,
+			      false, handle_musttail_attribute, NULL },
   /* FIXME: logically, noreturn attributes should be listed as
      "false, true, true" and apply to function types.  But implementing this
      would require all the places in the compiler that use TREE_THIS_VOLATILE
@@ -1216,6 +1218,19 @@ handle_common_attribute (tree *node, tree name, tree ARG_UNUSED (args),
   return NULL_TREE;
 }
 
+/* Handle a "musttail" attribute; arguments as in
+   struct attribute_spec.handler.  */
+
+tree
+handle_musttail_attribute (tree ARG_UNUSED (*node), tree name, tree ARG_UNUSED (args),
+			   int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  /* Currently only a statement attribute, handled directly in parser.  */
+  warning (OPT_Wattributes, "%qE attribute ignored", name);
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
 /* Handle a "noreturn" attribute; arguments as in
    struct attribute_spec.handler.  */
 
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 2d5f53998855..2707405e8def 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1637,6 +1637,7 @@ extern tree find_tm_attribute (tree);
 extern const struct attribute_spec::exclusions attr_cold_hot_exclusions[];
 extern const struct attribute_spec::exclusions attr_noreturn_exclusions[];
 extern tree handle_noreturn_attribute (tree *, tree, tree, int, bool *);
+extern tree handle_musttail_attribute (tree *, tree, tree, int, bool *);
 extern bool has_attribute (location_t, tree, tree, tree (*)(tree));
 extern tree build_attr_access_from_parms (tree, bool);
 
-- 
2.44.0

[PATCH v6 3/8] 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

	PR83324

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_statement): Handle musttail.
	(cp_parser_jump_statement): Dito.
	(cp_parser_std_attribute): Dito.
---
 gcc/cp/parser.cc | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 66ce161252c7..d25c1a1ac6a5 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -2467,7 +2467,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 *, tree &);
 static void cp_parser_declaration_statement
   (cp_parser *);
 
@@ -12734,13 +12734,17 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 						     NULL_TREE, false);
 	  break;
 
+	case RID_RETURN:
+	  std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
+	  statement = cp_parser_jump_statement (parser, std_attrs);
+	  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);
-	  statement = cp_parser_jump_statement (parser);
+	  statement = cp_parser_jump_statement (parser, std_attrs);
 	  break;
 
 	  /* Objective-C++ exception-handling constructs.  */
@@ -14797,10 +14801,11 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
    jump-statement:
      goto * expression ;
 
+   STD_ATTRS are the statement attributes. They can be modified.
    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, tree &std_attrs)
 {
   tree statement = error_mark_node;
   cp_token *token;
@@ -14877,6 +14882,33 @@ cp_parser_jump_statement (cp_parser* parser)
 	  /* If the next token is a `;', then there is no
 	     expression.  */
 	  expr = NULL_TREE;
+
+	if (keyword == RID_RETURN && expr)
+	  {
+	    bool musttail_p = false;
+	    if (lookup_attribute ("gnu", "musttail", std_attrs))
+	      {
+		musttail_p = true;
+		std_attrs = remove_attribute ("gnu", "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);
+	      }
+	    if (musttail_p)
+	      {
+		tree t = expr;
+		if (t && TREE_CODE (t) == TARGET_EXPR)
+		  t = TARGET_EXPR_INITIAL (t);
+		if (t && TREE_CODE (t) != CALL_EXPR)
+		  error_at (token->location, "cannot tail-call: return value must be a call");
+		else
+		  CALL_EXPR_MUST_TAIL_CALL (t) = 1;
+	      }
+	  }
+
 	/* Build the return-statement, check co-return first, since type
 	   deduction is not valid there.  */
 	if (keyword == RID_CO_RETURN)
@@ -30189,7 +30221,7 @@ cp_parser_std_attribute (cp_parser *parser, tree attr_ns)
     /* Maybe we don't expect to see any arguments for this attribute.  */
     const attribute_spec *as
       = lookup_attribute_spec (TREE_PURPOSE (attribute));
-    if (as && as->max_length == 0)
+    if ((as && as->max_length == 0) || is_attribute_p ("musttail", attr_id))
       {
 	error_at (token->location, "%qE attribute does not take any arguments",
 		  attr_id);
-- 
2.44.0

[PATCH v6 4/8] 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.

	PR83324

gcc/c/ChangeLog:

	* c-parser.cc (struct attr_state): Define with musttail_p.
	(c_parser_statement_after_labels): Handle [[musttail]]
	(c_parser_std_attribute): Dito.
	(c_parser_handle_musttail): Dito.
	(c_parser_compound_statement_nostart): Dito.
	(c_parser_all_labels): Dito.
	(c_parser_statement): Dito.
	* c-tree.h (c_finish_return): Add musttail_p flag.
	* c-typeck.cc (c_finish_return): Handle musttail_p flag.
---
 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 00f8bf4376e5..9edadb0fee96 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> *);
@@ -5756,6 +5761,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;
@@ -6941,6 +6948,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).  */
@@ -6957,6 +6986,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))
     {
@@ -7095,7 +7125,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)
@@ -7243,7 +7276,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
@@ -7285,15 +7318,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)
@@ -7315,6 +7351,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).
@@ -7558,11 +7595,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
@@ -7571,11 +7608,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;
@@ -7643,7 +7680,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
@@ -7652,7 +7689,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 22b0009874b5..002351bb9934 100644
--- a/gcc/c/c-tree.h
+++ b/gcc/c/c-tree.h
@@ -826,7 +826,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 4567b114734b..174348e96d7f 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -11445,10 +11445,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;
@@ -11462,6 +11462,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)
+    {
+      tree t = retval;
+      if (TREE_CODE (t) == TARGET_EXPR)
+	t = TARGET_EXPR_INITIAL (t);
+      if (TREE_CODE (t) != CALL_EXPR)
+	error_at (xloc, "cannot tail-call: return value must be call");
+      else
+	CALL_EXPR_MUST_TAIL_CALL (t) = 1;
+    }
+
   if (retval)
     {
       tree semantic_type = NULL_TREE;
-- 
2.44.0

[PATCH v6 5/8] Add tests for C/C++ musttail attributes

[Andi Kleen]

Mostly adopted from the existing C musttail plugin tests.

gcc/testsuite/ChangeLog:

	* c-c++-common/musttail1.c: New test.
	* c-c++-common/musttail2.c: New test.
	* c-c++-common/musttail3.c: New test.
	* c-c++-common/musttail4.c: New test.
	* c-c++-common/musttail7.c: New test.
	* c-c++-common/musttail8.c: New test.
	* g++.dg/musttail6.C: New test.
	* g++.dg/musttail9.C: New test.
---
 gcc/testsuite/c-c++-common/musttail1.c | 14 +++++++
 gcc/testsuite/c-c++-common/musttail2.c | 33 +++++++++++++++
 gcc/testsuite/c-c++-common/musttail3.c | 29 +++++++++++++
 gcc/testsuite/c-c++-common/musttail4.c | 17 ++++++++
 gcc/testsuite/c-c++-common/musttail5.c | 28 +++++++++++++
 gcc/testsuite/c-c++-common/musttail7.c | 14 +++++++
 gcc/testsuite/c-c++-common/musttail8.c | 17 ++++++++
 gcc/testsuite/g++.dg/musttail6.C       | 58 ++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/musttail9.C       | 10 +++++
 9 files changed, 220 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/c-c++-common/musttail5.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail7.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail8.c
 create mode 100644 gcc/testsuite/g++.dg/musttail6.C
 create mode 100644 gcc/testsuite/g++.dg/musttail9.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..74efcc2a0bc6
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail1.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { tail_call && { c || c++11 } } } } */
+/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
+
+int __attribute__((noinline,noclone,noipa))
+callee (int i)
+{
+  return i * i;
+}
+
+int __attribute__((noinline,noclone,noipa))
+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..86f2c3d77404
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail2.c
@@ -0,0 +1,33 @@
+/* { dg-do compile { target { tail_call && { c || c++11 } } } } */
+
+struct box { char field[256]; int i; };
+
+int __attribute__((noinline,noclone,noipa))
+test_2_callee (int i, struct box b)
+{
+  if (b.field[0])
+    return 5;
+  return i * i;
+}
+
+int __attribute__((noinline,noclone,noipa))
+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: " } */
+}
+
+extern float f7(void);
+
+int
+test_6 (void)
+{
+  [[gnu::musttail]] return f7(); /* { 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..ea9589c59ef2
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail3.c
@@ -0,0 +1,29 @@
+/* { dg-do compile { target { tail_call && { c || c++11 } } } } */
+
+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..23f4b5e1cd68
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail4.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target { tail_call && { c || c++11 } } } } */
+
+struct box { char field[64]; int i; };
+
+struct box __attribute__((noinline,noclone,noipa))
+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/c-c++-common/musttail5.c b/gcc/testsuite/c-c++-common/musttail5.c
new file mode 100644
index 000000000000..234da0d3f2a9
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail5.c
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-options "-std=c23" { target c } } */
+/* { dg-options "-std=gnu++11" { target c++ } } */
+
+[[musttail]] int j; /* { dg-warning "attribute" } */
+__attribute__((musttail)) int k; /* { dg-warning "attribute" } */
+
+void foo(void)
+{
+  [[gnu::musttail]] j++; /* { dg-warning "attribute" } */
+  [[gnu::musttail]] if (k > 0) /* { dg-warning "attribute" } */
+    [[gnu::musttail]] k++; /* { dg-warning "attribute" } */
+}
+
+int foo2(int p)
+{
+  [[gnu::musttail(1)]] return foo2(p + 1); /* { dg-error "\(before numeric constant|attribute\)" } */
+}
+
+int i;
+
+int foo3(void)
+{
+  [[musttail]] i++; /* { dg-warning "attribute" } */
+  [[musttail]] if (i > 10) /* { dg-warning "attribute" } */
+    [[musttail]] return foo2(i); /* { dg-warning "attribute" } */
+  return 0;
+}
diff --git a/gcc/testsuite/c-c++-common/musttail7.c b/gcc/testsuite/c-c++-common/musttail7.c
new file mode 100644
index 000000000000..5e4eb1bfbacc
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail7.c
@@ -0,0 +1,14 @@
+/* { dg-do compile { target { tail_call && { c || c++11 } } } } */
+/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
+
+extern void f();
+
+void f2()
+{
+  [[gnu::musttail]] return f2();
+}
+
+void f3()
+{
+  [[gnu::musttail]] return f();
+}
diff --git a/gcc/testsuite/c-c++-common/musttail8.c b/gcc/testsuite/c-c++-common/musttail8.c
new file mode 100644
index 000000000000..676d0e8f381c
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail8.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target { tail_call && { c || c++11 } } } } */
+
+float f1(void);
+
+int f2(void)
+{
+  [[gnu::musttail]] return f1 (); /* { dg-error "\(must be the same type|other reasons|target is not\)" } */
+}
+
+
+int f3(int *);
+
+int f4(void)
+{
+  int x;
+  [[gnu::musttail]] return f3(&x); /* { dg-error "\(refers to locals|other reasons\)" } */
+}
diff --git a/gcc/testsuite/g++.dg/musttail6.C b/gcc/testsuite/g++.dg/musttail6.C
new file mode 100644
index 000000000000..e0e478e08d58
--- /dev/null
+++ b/gcc/testsuite/g++.dg/musttail6.C
@@ -0,0 +1,58 @@
+/* { dg-do compile { target { tail_call } } } */
+/* { dg-options "-std=gnu++11" } */
+/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
+
+class Foo {
+public:
+  int a, b;
+  Foo(int a, int b) : a(a), b(b) {}
+};
+
+Foo __attribute__((noinline,noclone,noipa))
+callee (int i)
+{
+  return Foo(i, i+1);
+}
+
+Foo __attribute__((noinline,noclone,noipa))
+caller (int i)
+{
+  [[gnu::musttail]] return callee (i + 1);
+}
+
+template<typename T>
+T __attribute__((noinline,noclone,noipa)) foo (T i)
+{
+  return i + 1;
+}
+
+int
+caller2 (int k)
+{
+  [[gnu::musttail]] return foo<int>(1);
+}
+
+template<typename T>
+T caller3 (T v)
+{
+  [[gnu::musttail]] return foo<T>(v);
+}
+
+int call3(int i)
+{
+  [[gnu::musttail]] return caller3<int>(i + 1);
+}
+
+struct Bar {
+  int a;
+  Bar(int a) : a(a) {}
+  Bar operator+(Bar o) { return Bar(a + o.a); } 
+};
+
+#if __OPTIMIZE__ >= 1
+Bar
+caller4 (Bar k)
+{
+  [[gnu::musttail]] return caller3<Bar>(Bar(99));
+}
+#endif
diff --git a/gcc/testsuite/g++.dg/musttail9.C b/gcc/testsuite/g++.dg/musttail9.C
new file mode 100644
index 000000000000..78e0c50db6e0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/musttail9.C
@@ -0,0 +1,10 @@
+/* { dg-do compile { target { tail_call } } } */
+/* { dg-options "-std=gnu++11 -Wno-deprecated" } */
+/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
+
+extern void foo() throw(const char *);
+
+void f() throw()
+{
+  [[gnu::musttail]] return foo(); /* { dg-error "cannot tail-call" } */
+}
-- 
2.44.0

[PATCH v6 6/8] Enable musttail tail conversion even when not optimizing

[Andi Kleen]

Enable the tailcall optimization for non optimizing builds,
but in this case only checks calls that have the musttail attribute set.
This makes musttail work without optimization.

This is done with a new late musttail pass that is only active when
not optimizing. The pass must be after ehcleanup.

gcc/ChangeLog:

	* passes.def (pass_musttail): Add.
	* tree-pass.h (make_pass_musttail): Add.
	* tree-tailcall.cc (find_tail_calls): Handle only_musttail
	  argument.
	(tree_optimize_tail_calls_1): Pass on only_musttail.
	(execute_tail_calls): Pass only_musttail as false.
	(class pass_musttail): Add.
	(make_pass_musttail): Add.
---
 gcc/passes.def       |  1 +
 gcc/tree-pass.h      |  1 +
 gcc/tree-tailcall.cc | 64 +++++++++++++++++++++++++++++++++++++-------
 3 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/gcc/passes.def b/gcc/passes.def
index 1cbbd4130970..3e83cc327fd2 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -443,6 +443,7 @@ along with GCC; see the file COPYING3.  If not see
   NEXT_PASS (pass_tsan_O0);
   NEXT_PASS (pass_sanopt);
   NEXT_PASS (pass_cleanup_eh);
+  NEXT_PASS (pass_musttail);
   NEXT_PASS (pass_lower_resx);
   NEXT_PASS (pass_nrv);
   NEXT_PASS (pass_gimple_isel);
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 29267589eeb3..0668cea0a48e 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -368,6 +368,7 @@ extern gimple_opt_pass *make_pass_sra (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_sra_early (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_tail_recursion (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_tail_calls (gcc::context *ctxt);
+extern gimple_opt_pass *make_pass_musttail (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_fix_loops (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_tree_loop (gcc::context *ctxt);
 extern gimple_opt_pass *make_pass_tree_no_loop (gcc::context *ctxt);
diff --git a/gcc/tree-tailcall.cc b/gcc/tree-tailcall.cc
index e9f7f8a12b3a..094856de22ef 100644
--- a/gcc/tree-tailcall.cc
+++ b/gcc/tree-tailcall.cc
@@ -408,10 +408,10 @@ 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.  */
+   added to the start of RET. When ONLY_MUSTTAIL is set only handle musttail.  */
 
 static void
-find_tail_calls (basic_block bb, struct tailcall **ret)
+find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
 {
   tree ass_var = NULL_TREE, ret_var, func, param;
   gimple *stmt;
@@ -445,6 +445,9 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
       if (is_gimple_call (stmt))
 	{
 	  call = as_a <gcall *> (stmt);
+	  /* Handle only musttail calls when not optimizing.  */
+	  if (only_musttail && !gimple_call_must_tail_p (call))
+	    return;
 	  ass_var = gimple_call_lhs (call);
 	  break;
 	}
@@ -467,7 +470,7 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
       edge_iterator ei;
       /* Recurse to the predecessors.  */
       FOR_EACH_EDGE (e, ei, bb->preds)
-	find_tail_calls (e->src, ret);
+	find_tail_calls (e->src, ret, only_musttail);
 
       return;
     }
@@ -528,7 +531,8 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
   func = gimple_call_fndecl (call);
   if (func
       && !fndecl_built_in_p (func)
-      && recursive_call_p (current_function_decl, func))
+      && recursive_call_p (current_function_decl, func)
+      && !only_musttail)
     {
       tree arg;
 
@@ -1094,10 +1098,11 @@ create_tailcall_accumulator (const char *label, basic_block bb, tree init)
 }
 
 /* Optimizes tail calls in the function, turning the tail recursion
-   into iteration.  */
+   into iteration. When ONLY_MUSTCALL is true only optimize mustcall
+   marked calls.  */
 
 static unsigned int
-tree_optimize_tail_calls_1 (bool opt_tailcalls)
+tree_optimize_tail_calls_1 (bool opt_tailcalls, bool only_mustcall)
 {
   edge e;
   bool phis_constructed = false;
@@ -1117,7 +1122,7 @@ tree_optimize_tail_calls_1 (bool opt_tailcalls)
       /* 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);
+	find_tail_calls (e->src, &tailcalls, only_mustcall);
     }
 
   if (live_vars)
@@ -1228,7 +1233,7 @@ gate_tail_calls (void)
 static unsigned int
 execute_tail_calls (void)
 {
-  return tree_optimize_tail_calls_1 (true);
+  return tree_optimize_tail_calls_1 (true, false);
 }
 
 namespace {
@@ -1261,7 +1266,7 @@ public:
   bool gate (function *) final override { return gate_tail_calls (); }
   unsigned int execute (function *) final override
     {
-      return tree_optimize_tail_calls_1 (false);
+      return tree_optimize_tail_calls_1 (false, false);
     }
 
 }; // class pass_tail_recursion
@@ -1312,3 +1317,44 @@ make_pass_tail_calls (gcc::context *ctxt)
 {
   return new pass_tail_calls (ctxt);
 }
+
+namespace {
+
+const pass_data pass_data_musttail =
+{
+  GIMPLE_PASS, /* type */
+  "musttail", /* name */
+  OPTGROUP_NONE, /* optinfo_flags */
+  TV_NONE, /* tv_id */
+  ( PROP_cfg | PROP_ssa ), /* properties_required */
+  0, /* properties_provided */
+  0, /* properties_destroyed */
+  0, /* todo_flags_start */
+  0, /* todo_flags_finish */
+};
+
+class pass_musttail : public gimple_opt_pass
+{
+public:
+  pass_musttail (gcc::context *ctxt)
+    : gimple_opt_pass (pass_data_musttail, ctxt)
+  {}
+
+  /* opt_pass methods: */
+  /* This pass is only used when not optimizing to make [[musttail]] still
+     work.  */
+  bool gate (function *) final override { return !flag_optimize_sibling_calls; }
+  unsigned int execute (function *) final override
+  {
+    return tree_optimize_tail_calls_1 (true, true);
+  }
+
+}; // class pass_musttail
+
+} // anon namespace
+
+gimple_opt_pass *
+make_pass_musttail (gcc::context *ctxt)
+{
+  return new pass_musttail (ctxt);
+}
-- 
2.44.0

[PATCH v6 7/8] Give better error messages for musttail

[Andi Kleen]

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

This doesn't always work, for example when find_tail_call
walking gives up because the control flow is too complicated
then it won't find the tail call and can't give a suitable
error message.

gcc/ChangeLog:

	* tree-tailcall.cc (maybe_error_musttail): Add.
	(find_tail_calls): Add error messages.
---
 gcc/tree-tailcall.cc | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/gcc/tree-tailcall.cc b/gcc/tree-tailcall.cc
index 094856de22ef..1217435c3f90 100644
--- a/gcc/tree-tailcall.cc
+++ b/gcc/tree-tailcall.cc
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3.  If not see
 #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
@@ -402,6 +403,20 @@ propagate_through_phis (tree var, edge e)
   return var;
 }
 
+/* Report an error for failing to tail convert must call CALL
+   with error message ERR.  */
+
+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);
+      gimple_call_set_must_tail (call, false); /* Avoid another error.  */
+      gimple_call_set_tail (call, false);
+    }
+}
+
 /* 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;
@@ -489,8 +504,14 @@ 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, "complex return value");
+      return;
+    }
 
+  /* This doesn't really work because the BB would have multiple succ edges
+     and is rejected above. Instead just gives a "other reasons" error
+     at expand time.  */
   /* 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)
@@ -524,7 +545,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 +629,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 +638,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;
 		}
 	    }
@@ -631,6 +657,8 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
   auto_bitmap to_move_defs;
   auto_vec<gimple *> to_move_stmts;
 
+  /* Does not give musttail errors because the frontend won't generate
+     such musttails. */
   abb = bb;
   agsi = gsi;
   while (1)
@@ -716,7 +744,10 @@ 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 must be the same type");
+      return;
+    }
 
   /* If this is not a tail recursive call, we cannot handle addends or
      multiplicands.  */
-- 
2.44.0

[PATCH v6 8/8] Add documentation for musttail attribute

[Andi Kleen]

gcc/ChangeLog:

	* doc/extend.texi: Document [[musttail]]
---
 gcc/doc/extend.texi | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e290265d68d3..f3df7688edf1 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -9839,7 +9839,7 @@ same manner as the @code{deprecated} attribute.
 @section Statement Attributes
 @cindex Statement Attributes
 
-GCC allows attributes to be set on null statements.  @xref{Attribute Syntax},
+GCC allows attributes to be set on statements.  @xref{Attribute Syntax},
 for details of the exact syntax for using attributes.  Other attributes are
 available for functions (@pxref{Function Attributes}), variables
 (@pxref{Variable Attributes}), labels (@pxref{Label Attributes}), enumerators
@@ -9896,6 +9896,23 @@ 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 @code{return} statement with a return-value expression
+that is a function call.  It asserts 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 generates
+an error. On some targets they may not be supported.
+Tail calls cannot reference locals in memory, which may affect
+builds without optimization when passing structures that
+would fit into registers.
 @end table
 
 @node Attribute Syntax
@@ -10019,7 +10036,9 @@ the constant expression, if present.
 
 @subsubheading Statement Attributes
 In GNU C, an attribute specifier list may appear as part of a null
-statement.  The attribute goes before the semicolon.
+statement. The attribute goes before the semicolon.
+Some attributes in new style syntax are also supported
+on non-null statements.
 
 @subsubheading Type Attributes
 
-- 
2.44.0

Re: [PATCH v6 1/8] Improve must tail in RTL backend

[Michael Matz]

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.  */
> 

Re: [PATCH v6 1/8] Improve must tail in RTL backend

[Andi Kleen]

> 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's not the ultimate truth, but some of the checks it does are not
duplicated at expand time nor the backend. So it's one necessary pre condition
with the current code base.

Yes maybe the checks could be all moved, but that's a much larger
project.

-Andi

Re: [PATCH v6 1/8] Improve must tail in RTL backend

[Michael Matz]

Hello,

On Fri, 31 May 2024, Andi Kleen wrote:

> > 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's not the ultimate truth, but some of the checks it does are not 
> duplicated at expand time nor the backend. So it's one necessary pre 
> condition with the current code base.
> 
> Yes maybe the checks could be all moved, but that's a much larger 
> project.

Hmm.  I count six tests in about 25 lines of code in 
tree-tailcall.cc:suitable_for_tail_opt_p and suitable_for_tail_call_opt_p.

Are you perhaps worrying about the sibcall discovery itself (i.e. much of 
find_tail_calls)?  Why would that be needed for musttail?  Is that 
attribute sometimes applied to calls that aren't in fact sibcall-able?

One thing I'm worried about is the need for a new sibcall pass at O0 just 
for sibcall discovery.  find_tail_calls isn't cheap, because it computes 
live local variables for the whole function, potentially being quadratic.


Ciao,
Michael.

Re: [PATCH v6 1/8] Improve must tail in RTL backend

[Jakub Jelinek]

On Mon, Jun 03, 2024 at 07:02:00PM +0200, Michael Matz wrote:
> Hello,
> 
> On Fri, 31 May 2024, Andi Kleen wrote:
> 
> > > 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's not the ultimate truth, but some of the checks it does are not 
> > duplicated at expand time nor the backend. So it's one necessary pre 
> > condition with the current code base.
> > 
> > Yes maybe the checks could be all moved, but that's a much larger 
> > project.
> 
> Hmm.  I count six tests in about 25 lines of code in 
> tree-tailcall.cc:suitable_for_tail_opt_p and suitable_for_tail_call_opt_p.
> 
> Are you perhaps worrying about the sibcall discovery itself (i.e. much of 
> find_tail_calls)?  Why would that be needed for musttail?  Is that 
> attribute sometimes applied to calls that aren't in fact sibcall-able?
> 
> One thing I'm worried about is the need for a new sibcall pass at O0 just 
> for sibcall discovery.  find_tail_calls isn't cheap, because it computes 
> live local variables for the whole function, potentially being quadratic.

But the pass could be done only if there is at least one musttail call
in a function (remembered in some cfun flag).  If people use that attribute,
guess they are willing to pay for it.

	Jakub

Re: [PATCH v6 1/8] Improve must tail in RTL backend

[Andi Kleen]

> > Yes maybe the checks could be all moved, but that's a much larger 
> > project.
> 
> Hmm.  I count six tests in about 25 lines of code in 
> tree-tailcall.cc:suitable_for_tail_opt_p and suitable_for_tail_call_opt_p.

There are more checks in find_tail_calls. The logic is fairly spread
out. Some of it is needed to determine if it is valid.

> 
> Are you perhaps worrying about the sibcall discovery itself (i.e. much of 
> find_tail_calls)? Why would that be needed for musttail?  Is that 
> attribute sometimes applied to calls that aren't in fact sibcall-able?

The rules the compilers use for this are hard to understand for
programmers.  So that's the whole point of the attribute. If they miss some subtle 
requirement they get a compile time error instead of a stack overflow at
runtime.

So yes it has to do all the checks.

> 
> One thing I'm worried about is the need for a new sibcall pass at O0 just 
> for sibcall discovery.  find_tail_calls isn't cheap, because it computes 
> live local variables for the whole function, potentially being quadratic.

The live local variables computation is only done when there are actual
suitable tail calls. And the new -O0 variant only does it for musttail, nothing
else. So by default it is just a BB backwards walk until it sees a BB with 
enough edges to give up.


-Andi

Re: [PATCH v6 1/8] Improve must tail in RTL backend

[Michael Matz]

Hello,

On Mon, 3 Jun 2024, Jakub Jelinek wrote:

> > Hmm.  I count six tests in about 25 lines of code in 
> > tree-tailcall.cc:suitable_for_tail_opt_p and suitable_for_tail_call_opt_p.
> > 
> > Are you perhaps worrying about the sibcall discovery itself (i.e. much of 
> > find_tail_calls)?  Why would that be needed for musttail?  Is that 
> > attribute sometimes applied to calls that aren't in fact sibcall-able?
> > 
> > One thing I'm worried about is the need for a new sibcall pass at O0 just 
> > for sibcall discovery.  find_tail_calls isn't cheap, because it computes 
> > live local variables for the whole function, potentially being quadratic.
> 
> But the pass could be done only if there is at least one musttail call 
> in a function (remembered in some cfun flag).  If people use that 
> attribute, guess they are willing to pay for it.

Yeah, but I think the way the current proposal is doing it is mostly 
equivalent and fine enough, as Andi mentioned (in my worry I haven't 
considered that overall the backward walk stops fairly soon and then only 
does something when a musttail is there).

I still think that the tree pass being necessary for correctness is bad 
design, in the grand scheme of things, especially for those tests that are 
done for the call statement in isolation (i.e. tests about arguments like 
address-taken and suchlike, and return value, flags on the callee, and 
facts about the current function).  Those should all move to calls.cc or 
cfgexpand IMHO.

But I will yield on the discovery part that tree-tailcall is doing (i.e. 
those pieces that need to look at multiple statements, e.g. how the call 
result is used later); those are a bit harder to do in expand and how it's 
structured, and without getting rid of that part in tree-tailcall we have 
to run it at O0 anyway for musttail.  And moving only parts of the tests 
to calls.cc doesn't seem so worthwhile to hold up the patch.

So, I have no objections on the patch design anymore.


Ciao,
Michael.

Re: [PATCH v6 7/8] Give better error messages for musttail

[Andi Kleen]

[I slightly improve the patch covering a few more cases where
tree-tailcall gives up, especially with -O1 and -Os.
Here's the updated version.]

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.
    
gcc/ChangeLog:
    
      * tree-tailcall.cc (maybe_error_musttail): Add.
        (bb_get_succ_edge_count): Add.
        (find_tail_calls): Add error messages. Keep searching
        for basic blocks with multiple BBs if all but one is EH
        only.

diff --git a/gcc/tree-tailcall.cc b/gcc/tree-tailcall.cc
index 094856de22ef..7268e8138529 100644
--- a/gcc/tree-tailcall.cc
+++ b/gcc/tree-tailcall.cc
@@ -43,6 +43,7 @@ along with GCC; see the file COPYING3.  If not see
 #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
@@ -402,6 +403,36 @@ propagate_through_phis (tree var, edge e)
   return var;
 }
 
+/* Report an error for failing to tail convert must call CALL
+   with error message ERR.  */
+
+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);
+      gimple_call_set_must_tail (call, false); /* Avoid another error.  */
+      gimple_call_set_tail (call, false);
+    }
+}
+
+/* Count succ edges for BB and return in NUM_OTHER and NUM_EH.  */
+
+static void
+bb_get_succ_edge_count (basic_block bb, int &num_other, int &num_eh)
+{
+  edge e;
+  edge_iterator ei;
+  num_eh = 0;
+  num_other = 0;
+  FOR_EACH_EDGE (e, ei, bb->succs)
+    if (e->flags & EDGE_EH)
+      num_eh++;
+    else
+      num_other++;
+}
+
 /* 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;
@@ -426,8 +457,16 @@ find_tail_calls (basic_block bb, struct tailcall **ret, bool only_musttail)
   tree var;
 
   if (!single_succ_p (bb))
-    return;
+    {
+      int num_eh, num_other;
+      bb_get_succ_edge_count (bb, num_eh, num_other);
+      /* Allow a single EH edge so that we can give a better
+	 error message later.  */
+      if (!(num_eh == 1 && num_other == 1))
+	return;
+    }
 
+  bool bad_stmt = false;
   for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
     {
       stmt = gsi_stmt (gsi);
@@ -448,6 +487,11 @@ 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,7 +506,9 @@ 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;
+	{
+	  bad_stmt = true;
+	}
     }
 
   if (gsi_end_p (gsi))
@@ -489,13 +535,21 @@ 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, "complex return value");
+      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)
+  if ((stmt_could_throw_p (cfun, stmt)
       && !stmt_can_throw_external (cfun, stmt))
+	|| !single_succ_p (bb))
+  {
+    maybe_error_musttail (call,
+			  "call may throw exception that does not propagate");
     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 +578,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 +662,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 +671,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 +717,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 +779,25 @@ 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 must be the same type");
+      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)