[PATCH RFA] gimplify: prevent some C++ temporary elision

[PATCH RFA] gimplify: prevent some C++ temporary elision

[Jason Merrill]

Tested x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

In this testcase, we were optimizing away the temporary for f(), but
C++17 and above are clear that there is a temporary, and because its
destructor has visible side-effects we can't optimize it away under the
as-if rule.  So disable this optimization for TREE_ADDRESSABLE type.

I moved the declaration of volatile_p after the call to
gimple_fold_indirect_ref_rhs to minimize indentation changes; I don't see
any way the value of that flag could be affected by the call.

gcc/ChangeLog:

	* gimplify.cc (gimplify_modify_expr_rhs): Don't optimize
	x = *(A*)&<expr> to x = <expr> for a TREE_ADDRESSABLE type.

gcc/testsuite/ChangeLog:

	* g++.dg/init/elide9.C: New test.
---
 gcc/gimplify.cc                    | 15 ++++++++-------
 gcc/testsuite/g++.dg/init/elide9.C | 25 +++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/init/elide9.C

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 9e0e3429958..07e63538e5b 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -5601,7 +5601,7 @@ gimplify_modify_expr_rhs (tree *expr_p, tree *from_p, tree *to_p,
 	    }
 	  break;
 	case INDIRECT_REF:
-	  {
+	  if (!TREE_ADDRESSABLE (TREE_TYPE (*from_p)))
 	    /* If we have code like
 
 	     *(const A*)(A*)&x
@@ -5610,11 +5610,13 @@ gimplify_modify_expr_rhs (tree *expr_p, tree *from_p, tree *to_p,
 	     of "A"), treat the entire expression as identical to "x".
 	     This kind of code arises in C++ when an object is bound
 	     to a const reference, and if "x" is a TARGET_EXPR we want
-	     to take advantage of the optimization below.  */
-	    bool volatile_p = TREE_THIS_VOLATILE (*from_p);
-	    tree t = gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p, 0));
-	    if (t)
+	     to take advantage of the optimization below.  But not if
+	     the type is TREE_ADDRESSABLE; then C++17 says that the
+	     TARGET_EXPR needs to be a temporary.  */
+	    if (tree t
+		= gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p, 0)))
 	      {
+		bool volatile_p = TREE_THIS_VOLATILE (*from_p);
 		if (TREE_THIS_VOLATILE (t) != volatile_p)
 		  {
 		    if (DECL_P (t))
@@ -5627,8 +5629,7 @@ gimplify_modify_expr_rhs (tree *expr_p, tree *from_p, tree *to_p,
 		ret = GS_OK;
 		changed = true;
 	      }
-	    break;
-	  }
+	  break;
 
 	case TARGET_EXPR:
 	  {
diff --git a/gcc/testsuite/g++.dg/init/elide9.C b/gcc/testsuite/g++.dg/init/elide9.C
new file mode 100644
index 00000000000..810d60a94a4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/init/elide9.C
@@ -0,0 +1,25 @@
+// The static_cast should prevent temporary elision.
+// { dg-do run { target c++11 } }
+
+int d;
+struct A
+{
+  int i;
+  A() { }
+  ~A() { ++d; }
+};
+
+A f() { return A(); }
+
+struct B
+{
+  A a;
+  B(): a(static_cast<A&&>(f())) {}
+};
+
+int main()
+{
+  { B b; }
+  if (d != 2)
+    return -1;
+}

base-commit: 966010b2eb4a4c52f139b63548533e7bbd74ec9c
-- 
2.31.1

Re: [PATCH RFA] gimplify: prevent some C++ temporary elision

[Richard Biener]

On Thu, Oct 6, 2022 at 8:23 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Tested x86_64-pc-linux-gnu, OK for trunk?

OK.

Thanks,
Richard.

> -- >8 --
>
> In this testcase, we were optimizing away the temporary for f(), but
> C++17 and above are clear that there is a temporary, and because its
> destructor has visible side-effects we can't optimize it away under the
> as-if rule.  So disable this optimization for TREE_ADDRESSABLE type.
>
> I moved the declaration of volatile_p after the call to
> gimple_fold_indirect_ref_rhs to minimize indentation changes; I don't see
> any way the value of that flag could be affected by the call.
>
> gcc/ChangeLog:
>
>         * gimplify.cc (gimplify_modify_expr_rhs): Don't optimize
>         x = *(A*)&<expr> to x = <expr> for a TREE_ADDRESSABLE type.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/init/elide9.C: New test.
> ---
>  gcc/gimplify.cc                    | 15 ++++++++-------
>  gcc/testsuite/g++.dg/init/elide9.C | 25 +++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 7 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/init/elide9.C
>
> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index 9e0e3429958..07e63538e5b 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -5601,7 +5601,7 @@ gimplify_modify_expr_rhs (tree *expr_p, tree *from_p, tree *to_p,
>             }
>           break;
>         case INDIRECT_REF:
> -         {
> +         if (!TREE_ADDRESSABLE (TREE_TYPE (*from_p)))
>             /* If we have code like
>
>              *(const A*)(A*)&x
> @@ -5610,11 +5610,13 @@ gimplify_modify_expr_rhs (tree *expr_p, tree *from_p, tree *to_p,
>              of "A"), treat the entire expression as identical to "x".
>              This kind of code arises in C++ when an object is bound
>              to a const reference, and if "x" is a TARGET_EXPR we want
> -            to take advantage of the optimization below.  */
> -           bool volatile_p = TREE_THIS_VOLATILE (*from_p);
> -           tree t = gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p, 0));
> -           if (t)
> +            to take advantage of the optimization below.  But not if
> +            the type is TREE_ADDRESSABLE; then C++17 says that the
> +            TARGET_EXPR needs to be a temporary.  */
> +           if (tree t
> +               = gimple_fold_indirect_ref_rhs (TREE_OPERAND (*from_p, 0)))
>               {
> +               bool volatile_p = TREE_THIS_VOLATILE (*from_p);
>                 if (TREE_THIS_VOLATILE (t) != volatile_p)
>                   {
>                     if (DECL_P (t))
> @@ -5627,8 +5629,7 @@ gimplify_modify_expr_rhs (tree *expr_p, tree *from_p, tree *to_p,
>                 ret = GS_OK;
>                 changed = true;
>               }
> -           break;
> -         }
> +         break;
>
>         case TARGET_EXPR:
>           {
> diff --git a/gcc/testsuite/g++.dg/init/elide9.C b/gcc/testsuite/g++.dg/init/elide9.C
> new file mode 100644
> index 00000000000..810d60a94a4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/init/elide9.C
> @@ -0,0 +1,25 @@
> +// The static_cast should prevent temporary elision.
> +// { dg-do run { target c++11 } }
> +
> +int d;
> +struct A
> +{
> +  int i;
> +  A() { }
> +  ~A() { ++d; }
> +};
> +
> +A f() { return A(); }
> +
> +struct B
> +{
> +  A a;
> +  B(): a(static_cast<A&&>(f())) {}
> +};
> +
> +int main()
> +{
> +  { B b; }
> +  if (d != 2)
> +    return -1;
> +}
>
> base-commit: 966010b2eb4a4c52f139b63548533e7bbd74ec9c
> --
> 2.31.1
>