[pushed] c++: allow NRV and non-NRV returns [PR58487]

[pushed] c++: allow NRV and non-NRV returns [PR58487]

[Jason Merrill]

Tested x86_64-pc-linux-gnu, applying to trunk.

-- 8< --

Now that we support NRV from an inner block, we can also support non-NRV
returns from other blocks, since once the NRV is out of scope a later return
expression can't possibly alias it.

This fixes 58487 and half-fixes 53637: now one of the returns is elided, but
not the other.

Fixing the remaining xfails in these testcases will require a very different
approach, probably involving a full tree/block walk from finalize_nrv, and
check_return_expr only adding to a list of potential return variables.

	PR c++/58487
	PR c++/53637

gcc/cp/ChangeLog:

	* cp-tree.h (INIT_EXPR_NRV_P): New.
	* semantics.cc (finalize_nrv_r): Check it.
	* name-lookup.h (decl_in_scope_p): Declare.
	* name-lookup.cc (decl_in_scope_p): New.
	* typeck.cc (check_return_expr): Allow non-NRV
	returns if the NRV is no longer in scope.

gcc/testsuite/ChangeLog:

	* g++.dg/opt/nrv26.C: New test.
	* g++.dg/opt/nrv26a.C: New test.
	* g++.dg/opt/nrv27.C: New test.
---
 gcc/cp/cp-tree.h                  |  5 +++++
 gcc/cp/name-lookup.h              |  1 +
 gcc/cp/name-lookup.cc             | 22 ++++++++++++++++++
 gcc/cp/semantics.cc               |  8 +++----
 gcc/cp/typeck.cc                  | 37 ++++++++++++++++++++++++-------
 gcc/testsuite/g++.dg/opt/nrv26.C  | 19 ++++++++++++++++
 gcc/testsuite/g++.dg/opt/nrv26a.C | 18 +++++++++++++++
 gcc/testsuite/g++.dg/opt/nrv27.C  | 23 +++++++++++++++++++
 8 files changed, 121 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/opt/nrv26.C
 create mode 100644 gcc/testsuite/g++.dg/opt/nrv26a.C
 create mode 100644 gcc/testsuite/g++.dg/opt/nrv27.C

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 87572e3574d..83982233111 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -444,6 +444,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
       REINTERPRET_CAST_P (in NOP_EXPR)
       ALIGNOF_EXPR_STD_P (in ALIGNOF_EXPR)
       OVL_DEDUP_P (in OVERLOAD)
+      INIT_EXPR_NRV_P (in INIT_EXPR)
       ATOMIC_CONSTR_MAP_INSTANTIATED_P (in ATOMIC_CONSTR)
       contract_semantic (in ASSERTION_, PRECONDITION_, POSTCONDITION_STMT)
    1: IDENTIFIER_KIND_BIT_1 (in IDENTIFIER_NODE)
@@ -4078,6 +4079,10 @@ struct GTY(()) lang_decl {
 #define DELETE_EXPR_USE_VEC(NODE) \
   TREE_LANG_FLAG_1 (DELETE_EXPR_CHECK (NODE))
 
+/* True iff this represents returning a potential named return value.  */
+#define INIT_EXPR_NRV_P(NODE) \
+  TREE_LANG_FLAG_0 (INIT_EXPR_CHECK (NODE))
+
 #define CALL_OR_AGGR_INIT_CHECK(NODE) \
   TREE_CHECK2 ((NODE), CALL_EXPR, AGGR_INIT_EXPR)
 
diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index b3e708561d8..613745ba501 100644
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -449,6 +449,7 @@ extern void resort_type_member_vec (void *, void *,
 extern vec<tree, va_gc> *set_class_bindings (tree, int extra = 0);
 extern void insert_late_enum_def_bindings (tree, tree);
 extern tree innermost_non_namespace_value (tree);
+extern bool decl_in_scope_p (tree);
 extern cxx_binding *outer_binding (tree, cxx_binding *, bool);
 extern void cp_emit_debug_info_for_using (tree, tree);
 
diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
index eb5c333b5ea..b8ca7306a28 100644
--- a/gcc/cp/name-lookup.cc
+++ b/gcc/cp/name-lookup.cc
@@ -7451,6 +7451,28 @@ innermost_non_namespace_value (tree name)
   return binding ? binding->value : NULL_TREE;
 }
 
+/* True iff current_binding_level is within the potential scope of local
+   variable DECL. */
+
+bool
+decl_in_scope_p (tree decl)
+{
+  gcc_checking_assert (DECL_FUNCTION_SCOPE_P (decl));
+
+  tree name = DECL_NAME (decl);
+
+  for (cxx_binding *iter = NULL;
+       (iter = outer_binding (name, iter, /*class_p=*/false)); )
+    {
+      if (!LOCAL_BINDING_P (iter))
+	return false;
+      if (iter->value == decl)
+	return true;
+    }
+
+  return false;
+}
+
 /* Look up NAME in the current binding level and its superiors in the
    namespace of variables, functions and typedefs.  Return a ..._DECL
    node of some kind representing its definition if there is only one
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 1d397b6f257..a2e74a5d2c7 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -4956,7 +4956,7 @@ finalize_nrv_r (tree* tp, int* walk_subtrees, void* data)
   /* If there's a label, we might need to destroy the NRV on goto (92407).  */
   else if (TREE_CODE (*tp) == LABEL_EXPR)
     dp->simple = false;
-  /* Change all returns to just refer to the RESULT_DECL; this is a nop,
+  /* Change NRV returns to just refer to the RESULT_DECL; this is a nop,
      but differs from using NULL_TREE in that it indicates that we care
      about the value of the RESULT_DECL.  But preserve anything appended
      by check_return_expr.  */
@@ -4965,9 +4965,9 @@ finalize_nrv_r (tree* tp, int* walk_subtrees, void* data)
       tree *p = &TREE_OPERAND (*tp, 0);
       while (TREE_CODE (*p) == COMPOUND_EXPR)
 	p = &TREE_OPERAND (*p, 0);
-      gcc_checking_assert (TREE_CODE (*p) == INIT_EXPR
-			   && TREE_OPERAND (*p, 0) == dp->result);
-      *p = dp->result;
+      if (TREE_CODE (*p) == INIT_EXPR
+	  && INIT_EXPR_NRV_P (*p))
+	*p = dp->result;
     }
   /* Change all cleanups for the NRV to only run when not returning.  */
   else if (TREE_CODE (*tp) == CLEANUP_STMT
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 6b5705e806d..11927cbdf83 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -11147,11 +11147,15 @@ check_return_expr (tree retval, bool *no_warning)
      So, if this is a value-returning function that always returns the same
      local variable, remember it.
 
-     It might be nice to be more flexible, and choose the first suitable
-     variable even if the function sometimes returns something else, but
-     then we run the risk of clobbering the variable we chose if the other
-     returned expression uses the chosen variable somehow.  And people expect
-     this restriction, anyway.  (jason 2000-11-19)
+     We choose the first suitable variable even if the function sometimes
+     returns something else, but only if the variable is out of scope at the
+     other return sites, or else we run the risk of clobbering the variable we
+     chose if the other returned expression uses the chosen variable somehow.
+
+     We don't currently do this if the first return is a non-variable, as it
+     would be complicated to determine whether an NRV selected later was in
+     scope at the point of the earlier return.  We also don't currently support
+     multiple variables with non-overlapping scopes (53637).
 
      See finish_function and finalize_nrv for the rest of this optimization.  */
   tree bare_retval = NULL_TREE;
@@ -11162,12 +11166,26 @@ check_return_expr (tree retval, bool *no_warning)
     }
 
   bool named_return_value_okay_p = want_nrvo_p (bare_retval, functype);
-  if (fn_returns_value_p && flag_elide_constructors)
+  if (fn_returns_value_p && flag_elide_constructors
+      && current_function_return_value != bare_retval)
     {
       if (named_return_value_okay_p
-          && (current_function_return_value == NULL_TREE
-	      || current_function_return_value == bare_retval))
+	  && current_function_return_value == NULL_TREE)
 	current_function_return_value = bare_retval;
+      else if (current_function_return_value
+	       && VAR_P (current_function_return_value)
+	       && !decl_in_scope_p (current_function_return_value))
+	{
+	  /* The earlier NRV is out of scope at this point, so it's safe to
+	     leave it alone; the current return can't refer to it.  */;
+	  if (named_return_value_okay_p
+	      && !warning_suppressed_p (current_function_decl, OPT_Wnrvo))
+	    {
+	      warning (OPT_Wnrvo, "not eliding copy on return from %qD",
+		       bare_retval);
+	      suppress_warning (current_function_decl, OPT_Wnrvo);
+	    }
+	}
       else
 	{
 	  if ((named_return_value_okay_p
@@ -11281,6 +11299,9 @@ check_return_expr (tree retval, bool *no_warning)
       retval = cp_build_init_expr (result, retval);
     }
 
+  if (current_function_return_value == bare_retval)
+    INIT_EXPR_NRV_P (retval) = true;
+
   if (tree set = maybe_set_retval_sentinel ())
     retval = build2 (COMPOUND_EXPR, void_type_node, retval, set);
 
diff --git a/gcc/testsuite/g++.dg/opt/nrv26.C b/gcc/testsuite/g++.dg/opt/nrv26.C
new file mode 100644
index 00000000000..3d2b12c5f6c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/opt/nrv26.C
@@ -0,0 +1,19 @@
+// PR c++/58487
+// { dg-additional-options -Wnrvo }
+// { dg-do link }
+
+struct A {
+  A() {}
+  A(const A&);
+};
+
+A test() {
+  if (true) {
+    A a;
+    return a;
+  } else {
+    return A();
+  }
+}
+
+int main() { }
diff --git a/gcc/testsuite/g++.dg/opt/nrv26a.C b/gcc/testsuite/g++.dg/opt/nrv26a.C
new file mode 100644
index 00000000000..208014a186a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/opt/nrv26a.C
@@ -0,0 +1,18 @@
+// PR c++/58487
+// { dg-additional-options -Wnrvo }
+
+struct A {
+  A() {}
+  A(const A&);
+};
+
+A test() {
+  if (true) {
+    return A();
+  } else {
+    A a;
+    return a;		       // { dg-bogus "not eliding" "" { xfail *-*-* } }
+  }
+}
+
+int main() { }
diff --git a/gcc/testsuite/g++.dg/opt/nrv27.C b/gcc/testsuite/g++.dg/opt/nrv27.C
new file mode 100644
index 00000000000..f71dd7c3eb2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/opt/nrv27.C
@@ -0,0 +1,23 @@
+// PR c++/53637
+// { dg-additional-options "-Wnrvo -fdump-tree-gimple" }
+
+class Foo {
+public:
+  Foo() {}
+  Foo(const Foo& other);
+};
+
+Foo bar(int i) {
+  if (i > 1) {
+    Foo result;
+    return result;	// We currently elide this copy, but not the second one.
+    // { dg-final { scan-tree-dump {result .value-expr} "gimple" } }
+  } else {
+    Foo result;
+    return result; // { dg-bogus "not eliding copy on return from" "" { xfail *-*-* } }
+  }
+}
+
+int main(int argc, char* argv[]) {
+  Foo f = bar(argc);
+}

base-commit: 5faaabef3819434d13fcbf749bd07bfc98ca7c3c
-- 
2.31.1

Re: [pushed] c++: allow NRV and non-NRV returns [PR58487]

[Hans-Peter Nilsson]

> Date: Wed,  7 Jun 2023 18:06:15 -0400
> From: Jason Merrill via Gcc-patches <gcc-patches@gcc.gnu.org>

> Tested x86_64-pc-linux-gnu, applying to trunk.
> 
> -- 8< --
> 
> Now that we support NRV from an inner block, we can also support non-NRV
> returns from other blocks, since once the NRV is out of scope a later return
> expression can't possibly alias it.
> 
> This fixes 58487 and half-fixes 53637: now one of the returns is elided, but
> not the other.
> 
> Fixing the remaining xfails in these testcases will require a very different
> approach, probably involving a full tree/block walk from finalize_nrv, and
> check_return_expr only adding to a list of potential return variables.
> 
> 	PR c++/58487
> 	PR c++/53637
> 
> gcc/cp/ChangeLog:
> 
> 	* cp-tree.h (INIT_EXPR_NRV_P): New.
> 	* semantics.cc (finalize_nrv_r): Check it.
> 	* name-lookup.h (decl_in_scope_p): Declare.
> 	* name-lookup.cc (decl_in_scope_p): New.
> 	* typeck.cc (check_return_expr): Allow non-NRV
> 	returns if the NRV is no longer in scope.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/opt/nrv26.C: New test.
> 	* g++.dg/opt/nrv26a.C: New test.
> 	* g++.dg/opt/nrv27.C: New test.

This somehow caused 21 regressions for cris-elf in the c++
and libstdc++ testsuites.  I opened PR110185 to hold the
preprocessed g++.dg/cpp2a/spaceship-p1186.C.

brgds, H-P