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
base-commit: 5faaabef3819434d13fcbf749bd07bfc98ca7c3c
> 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
@@ -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)
@@ -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);
@@ -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
@@ -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
@@ -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);
new file mode 100644
@@ -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() { }
new file mode 100644
@@ -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() { }
new file mode 100644
@@ -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);
+}