diff mbox series

[C++] Clear TREE_ADDRESSABLE on non-aggregate thunk arguments (PR c++/89187)

Message ID 20190205090117.GR2135@tucnak
State New
Headers show
Series [C++] Clear TREE_ADDRESSABLE on non-aggregate thunk arguments (PR c++/89187) | expand

Commit Message

Jakub Jelinek Feb. 5, 2019, 9:01 a.m. UTC 
Hi!

As the following testcase shows, for thunks the expansion code sometimes
requires that arguments with gimple reg type aren't addressable,
thunk needs to be simple passing of arguments directly to another call, not
having extra statements in between that.

In particular, for pass_by_reference arguments calls.c has if
call_from_thunk_p:
              /* We may have turned the parameter value into an SSA name.
                 Go back to the original parameter so we can take the
                 address.  */
              if (TREE_CODE (args[i].tree_value) == SSA_NAME)
                {
                  gcc_assert (SSA_NAME_IS_DEFAULT_DEF (args[i].tree_value));
                  args[i].tree_value = SSA_NAME_VAR (args[i].tree_value);
                  gcc_assert (TREE_CODE (args[i].tree_value) == PARM_DECL);
                }
which is not the case if the PARM_DECL is TREE_ADDRESSABLE, then the def
stmt of the SSA_NAME is an assignment which loads the PARM_DECL value from
"memory".

TREE_ADDRESSABLE on the arguments is copied from the ctor from which it is
cloned (and which will be called by the thunk).  In this case the argument
needs to be addressable because vector[idx] folding takes address of vector.
In the thunk itself that doesn't matter though, it just passes the argument
to another function, so the following patch clears the TREE_ADDRESSABLE bit
there.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-02-05  Jakub Jelinek  <jakub@redhat.com>

	PR c++/89187
	* optimize.c (maybe_thunk_body): Clear TREE_ADDRESSABLE on
	non-aggregate PARM_DECLs of the thunk.

	* g++.dg/opt/pr89187.C: New test.


	Jakub

Comments

Jason Merrill Feb. 5, 2019, 4:16 p.m. UTC | #1 
On 2/5/19 4:01 AM, Jakub Jelinek wrote:
> Hi!
> 
> As the following testcase shows, for thunks the expansion code sometimes
> requires that arguments with gimple reg type aren't addressable,
> thunk needs to be simple passing of arguments directly to another call, not
> having extra statements in between that.
> 
> In particular, for pass_by_reference arguments calls.c has if
> call_from_thunk_p:
>                /* We may have turned the parameter value into an SSA name.
>                   Go back to the original parameter so we can take the
>                   address.  */
>                if (TREE_CODE (args[i].tree_value) == SSA_NAME)
>                  {
>                    gcc_assert (SSA_NAME_IS_DEFAULT_DEF (args[i].tree_value));
>                    args[i].tree_value = SSA_NAME_VAR (args[i].tree_value);
>                    gcc_assert (TREE_CODE (args[i].tree_value) == PARM_DECL);
>                  }
> which is not the case if the PARM_DECL is TREE_ADDRESSABLE, then the def
> stmt of the SSA_NAME is an assignment which loads the PARM_DECL value from
> "memory".
> 
> TREE_ADDRESSABLE on the arguments is copied from the ctor from which it is
> cloned (and which will be called by the thunk).  In this case the argument
> needs to be addressable because vector[idx] folding takes address of vector.
> In the thunk itself that doesn't matter though, it just passes the argument
> to another function, so the following patch clears the TREE_ADDRESSABLE bit
> there.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2019-02-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/89187
> 	* optimize.c (maybe_thunk_body): Clear TREE_ADDRESSABLE on
> 	non-aggregate PARM_DECLs of the thunk.
> 
> 	* g++.dg/opt/pr89187.C: New test.
> 
> --- gcc/cp/optimize.c.jj	2019-01-21 23:32:43.000000000 +0100
> +++ gcc/cp/optimize.c	2019-02-04 16:40:21.354179933 +0100
> @@ -417,6 +417,12 @@ maybe_thunk_body (tree fn, bool force)
>   		  gcc_assert (clone_parm);
>   		  DECL_ABSTRACT_ORIGIN (clone_parm) = NULL;
>   		  args[parmno] = clone_parm;
> +		  /* Clear TREE_ADDRESSABLE on arguments with non-aggregate
> +		     types, the thunk will not take addresses of those
> +		     arguments, will just pass them through to another
> +		     call.  */
> +		  if (!AGGREGATE_TYPE_P (TREE_TYPE (clone_parm)))
> +		    TREE_ADDRESSABLE (clone_parm) = 0;

We probably want to do this in maybe_add_lambda_conv_op, as well (in the 
loop over fn_args that sets DECL_CONTEXT).

I notice that use_thunk clears TREE_ADDRESSABLE unconditionally, is it 
important to handle aggregates differently here?

use_thunk also clears DECL_RTL and DECL_HAS_VALUE_EXPR_P, I don't know 
if that's important.

Jason
Jakub Jelinek Feb. 5, 2019, 4:40 p.m. UTC | #2 
On Tue, Feb 05, 2019 at 11:16:04AM -0500, Jason Merrill wrote:
> > --- gcc/cp/optimize.c.jj	2019-01-21 23:32:43.000000000 +0100
> > +++ gcc/cp/optimize.c	2019-02-04 16:40:21.354179933 +0100
> > @@ -417,6 +417,12 @@ maybe_thunk_body (tree fn, bool force)
> >   		  gcc_assert (clone_parm);
> >   		  DECL_ABSTRACT_ORIGIN (clone_parm) = NULL;
> >   		  args[parmno] = clone_parm;
> > +		  /* Clear TREE_ADDRESSABLE on arguments with non-aggregate
> > +		     types, the thunk will not take addresses of those
> > +		     arguments, will just pass them through to another
> > +		     call.  */
> > +		  if (!AGGREGATE_TYPE_P (TREE_TYPE (clone_parm)))
> > +		    TREE_ADDRESSABLE (clone_parm) = 0;
> 
> We probably want to do this in maybe_add_lambda_conv_op, as well (in the
> loop over fn_args that sets DECL_CONTEXT).

Like below?

> I notice that use_thunk clears TREE_ADDRESSABLE unconditionally, is it
> important to handle aggregates differently here?

No, I was just trying to be too careful.  If use_thunk does that
unconditionally, I think we can as well.

> use_thunk also clears DECL_RTL and DECL_HAS_VALUE_EXPR_P, I don't know if
> that's important.

I can't imagine how DECL_RTL could be set (these days) and have no idea why
DECL_VALUE_EXPR would be used either.

I'll bootstrap/regtest following patch then:

2019-02-05  Jakub Jelinek  <jakub@redhat.com>

	PR c++/89187
	* optimize.c (maybe_thunk_body): Clear TREE_ADDRESSABLE on
	PARM_DECLs of the thunk.
	* lambda.c (maybe_add_lambda_conv_op): Likewise.

	* g++.dg/opt/pr89187.C: New test.

--- gcc/cp/optimize.c.jj	2019-02-05 10:04:17.089060672 +0100
+++ gcc/cp/optimize.c	2019-02-05 17:34:37.644762690 +0100
@@ -417,6 +417,8 @@ maybe_thunk_body (tree fn, bool force)
 		  gcc_assert (clone_parm);
 		  DECL_ABSTRACT_ORIGIN (clone_parm) = NULL;
 		  args[parmno] = clone_parm;
+		  /* Clear TREE_ADDRESSABLE on thunk arguments.  */
+		  TREE_ADDRESSABLE (clone_parm) = 0;
 		  clone_parm = TREE_CHAIN (clone_parm);
 		}
 	      if (fn_parm_typelist)
--- gcc/cp/lambda.c.jj	2019-02-02 11:07:01.217345765 +0100
+++ gcc/cp/lambda.c	2019-02-05 17:37:00.573387272 +0100
@@ -1130,6 +1130,9 @@ maybe_add_lambda_conv_op (tree type)
       {
 	tree new_node = copy_node (src);
 
+	/* Clear TREE_ADDRESSABLE on thunk arguments.  */
+	TREE_ADDRESSABLE (new_node) = 0;
+
 	if (!fn_args)
 	  fn_args = tgt = new_node;
 	else
--- gcc/testsuite/g++.dg/opt/pr89187.C.jj	2019-02-05 17:33:44.230650417 +0100
+++ gcc/testsuite/g++.dg/opt/pr89187.C	2019-02-05 17:33:44.230650417 +0100
@@ -0,0 +1,23 @@
+// PR c++/89187
+// { dg-do compile { target c++11 } }
+// { dg-options "-Os -fno-tree-ccp -fno-tree-sra -fno-inline" }
+
+template <typename T, int N> struct A {
+  typedef T __attribute__((vector_size (N))) type;
+};
+template <typename T, int N> using B = typename A<T, N>::type;
+template <typename T> using C = B<T, 4>;
+struct D {
+  D (C<int> x) : d{x[3]} {}
+  D foo () { return d; }
+  C<int> d;
+};
+extern D d;
+struct { D bar () { return d; } } l;
+struct E { void baz () const; };
+
+void
+E::baz () const
+{
+  l.bar ().foo ();
+}


	Jakub
Jason Merrill Feb. 5, 2019, 6:25 p.m. UTC | #3 
On 2/5/19 11:40 AM, Jakub Jelinek wrote:
> On Tue, Feb 05, 2019 at 11:16:04AM -0500, Jason Merrill wrote:
>>> --- gcc/cp/optimize.c.jj	2019-01-21 23:32:43.000000000 +0100
>>> +++ gcc/cp/optimize.c	2019-02-04 16:40:21.354179933 +0100
>>> @@ -417,6 +417,12 @@ maybe_thunk_body (tree fn, bool force)
>>>    		  gcc_assert (clone_parm);
>>>    		  DECL_ABSTRACT_ORIGIN (clone_parm) = NULL;
>>>    		  args[parmno] = clone_parm;
>>> +		  /* Clear TREE_ADDRESSABLE on arguments with non-aggregate
>>> +		     types, the thunk will not take addresses of those
>>> +		     arguments, will just pass them through to another
>>> +		     call.  */
>>> +		  if (!AGGREGATE_TYPE_P (TREE_TYPE (clone_parm)))
>>> +		    TREE_ADDRESSABLE (clone_parm) = 0;
>>
>> We probably want to do this in maybe_add_lambda_conv_op, as well (in the
>> loop over fn_args that sets DECL_CONTEXT).
> 
> Like below?
> 
>> I notice that use_thunk clears TREE_ADDRESSABLE unconditionally, is it
>> important to handle aggregates differently here?
> 
> No, I was just trying to be too careful.  If use_thunk does that
> unconditionally, I think we can as well.
> 
>> use_thunk also clears DECL_RTL and DECL_HAS_VALUE_EXPR_P, I don't know if
>> that's important.
> 
> I can't imagine how DECL_RTL could be set (these days) and have no idea why
> DECL_VALUE_EXPR would be used either.
> 
> I'll bootstrap/regtest following patch then:
> 
> 2019-02-05  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/89187
> 	* optimize.c (maybe_thunk_body): Clear TREE_ADDRESSABLE on
> 	PARM_DECLs of the thunk.
> 	* lambda.c (maybe_add_lambda_conv_op): Likewise.

OK.

Jason
diff mbox series

Patch

--- gcc/cp/optimize.c.jj	2019-01-21 23:32:43.000000000 +0100
+++ gcc/cp/optimize.c	2019-02-04 16:40:21.354179933 +0100
@@ -417,6 +417,12 @@  maybe_thunk_body (tree fn, bool force)
 		  gcc_assert (clone_parm);
 		  DECL_ABSTRACT_ORIGIN (clone_parm) = NULL;
 		  args[parmno] = clone_parm;
+		  /* Clear TREE_ADDRESSABLE on arguments with non-aggregate
+		     types, the thunk will not take addresses of those
+		     arguments, will just pass them through to another
+		     call.  */
+		  if (!AGGREGATE_TYPE_P (TREE_TYPE (clone_parm)))
+		    TREE_ADDRESSABLE (clone_parm) = 0;
 		  clone_parm = TREE_CHAIN (clone_parm);
 		}
 	      if (fn_parm_typelist)
--- gcc/testsuite/g++.dg/opt/pr89187.C.jj	2019-02-04 17:34:13.973847193 +0100
+++ gcc/testsuite/g++.dg/opt/pr89187.C	2019-02-04 17:34:41.191396316 +0100
@@ -0,0 +1,23 @@ 
+// PR c++/89187
+// { dg-do compile { target c++11 } }
+// { dg-options "-Os -fno-tree-ccp -fno-tree-sra -fno-inline" }
+
+template <typename T, int N> struct A {
+  typedef T __attribute__((vector_size (N))) type;
+};
+template <typename T, int N> using B = typename A<T, N>::type;
+template <typename T> using C = B<T, 4>;
+struct D {
+  D (C<int> x) : d{x[3]} {}
+  D foo () { return d; }
+  C<int> d;
+};
+extern D d;
+struct { D bar () { return d; } } l;
+struct E { void baz () const; };
+
+void
+E::baz () const
+{
+  l.bar ().foo ();
+}