NRV with address taken

NRV with address taken

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 781 bytes --]

Hello,

the attached one-liner passed bootstrap+testsuite (really all languages) 
on x86_64-linux-gnu (I got an extra pass of unix/-m32: os but I assume 
that the failure with trunk was random).

The current code is a bit weird: we bail out if either result or found is 
TREE_ADDRESSABLE, but then the variable replacement includes:

   TREE_ADDRESSABLE (result) |= TREE_ADDRESSABLE (found);

(modified "recently", it was a plain assignment before)

I mostly ran the testsuite to find a testcase showing why found should not 
have its address taken, so if someone wants to add one (or at least a 
comment in tree-nrv.c), that would be good.


2014-10-16  Marc Glisse  <marc.glisse@inria.fr>

 	* tree-nrv.c (pass_nrv::execute): Don't disable when address is taken.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 928 bytes --]

Index: gcc/tree-nrv.c
===================================================================
--- gcc/tree-nrv.c	(revision 216286)
+++ gcc/tree-nrv.c	(working copy)
@@ -210,21 +210,20 @@ pass_nrv::execute (function *fun)
 		    return 0;
 		}
 	      else
 		found = rhs;
 
 	      /* The returned value must be a local automatic variable of the
 		 same type and alignment as the function's result.  */
 	      if (TREE_CODE (found) != VAR_DECL
 		  || TREE_THIS_VOLATILE (found)
 		  || !auto_var_in_fn_p (found, current_function_decl)
-		  || TREE_ADDRESSABLE (found)
 		  || DECL_ALIGN (found) > DECL_ALIGN (result)
 		  || !useless_type_conversion_p (result_type,
 						 TREE_TYPE (found)))
 		return 0;
 	    }
 	  else if (gimple_has_lhs (stmt))
 	    {
 	      tree addr = get_base_address (gimple_get_lhs (stmt));
 	       /* If there's any MODIFY of component of RESULT,
 		  then bail out.  */

Re: NRV with address taken

[Jakub Jelinek]

On Thu, Oct 16, 2014 at 07:37:18AM +0200, Marc Glisse wrote:
> Hello,
> 
> the attached one-liner passed bootstrap+testsuite (really all languages) on
> x86_64-linux-gnu (I got an extra pass of unix/-m32: os but I assume that the
> failure with trunk was random).
> 
> The current code is a bit weird: we bail out if either result or found is
> TREE_ADDRESSABLE, but then the variable replacement includes:
> 
>   TREE_ADDRESSABLE (result) |= TREE_ADDRESSABLE (found);
> 
> (modified "recently", it was a plain assignment before)
> 
> I mostly ran the testsuite to find a testcase showing why found should not
> have its address taken, so if someone wants to add one (or at least a
> comment in tree-nrv.c), that would be good.

I'd worry if both result and found are address taken before the pass, then
trying to merge them together might mean something meant to have different
addresses collapses into the same object.

> 2014-10-16  Marc Glisse  <marc.glisse@inria.fr>
> 
> 	* tree-nrv.c (pass_nrv::execute): Don't disable when address is taken.
> 
> -- 
> Marc Glisse

> Index: gcc/tree-nrv.c
> ===================================================================
> --- gcc/tree-nrv.c	(revision 216286)
> +++ gcc/tree-nrv.c	(working copy)
> @@ -210,21 +210,20 @@ pass_nrv::execute (function *fun)
>  		    return 0;
>  		}
>  	      else
>  		found = rhs;
>  
>  	      /* The returned value must be a local automatic variable of the
>  		 same type and alignment as the function's result.  */
>  	      if (TREE_CODE (found) != VAR_DECL
>  		  || TREE_THIS_VOLATILE (found)
>  		  || !auto_var_in_fn_p (found, current_function_decl)
> -		  || TREE_ADDRESSABLE (found)
>  		  || DECL_ALIGN (found) > DECL_ALIGN (result)
>  		  || !useless_type_conversion_p (result_type,
>  						 TREE_TYPE (found)))
>  		return 0;
>  	    }
>  	  else if (gimple_has_lhs (stmt))
>  	    {
>  	      tree addr = get_base_address (gimple_get_lhs (stmt));
>  	       /* If there's any MODIFY of component of RESULT,
>  		  then bail out.  */


	Jakub

Re: NRV with address taken

[Richard Biener]

On Thu, Oct 16, 2014 at 9:31 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Oct 16, 2014 at 07:37:18AM +0200, Marc Glisse wrote:
>> Hello,
>>
>> the attached one-liner passed bootstrap+testsuite (really all languages) on
>> x86_64-linux-gnu (I got an extra pass of unix/-m32: os but I assume that the
>> failure with trunk was random).
>>
>> The current code is a bit weird: we bail out if either result or found is
>> TREE_ADDRESSABLE, but then the variable replacement includes:
>>
>>   TREE_ADDRESSABLE (result) |= TREE_ADDRESSABLE (found);
>>
>> (modified "recently", it was a plain assignment before)
>>
>> I mostly ran the testsuite to find a testcase showing why found should not
>> have its address taken, so if someone wants to add one (or at least a
>> comment in tree-nrv.c), that would be good.

Does this fix PR63537?

> I'd worry if both result and found are address taken before the pass, then
> trying to merge them together might mean something meant to have different
> addresses collapses into the same object.

I'd not worry about that.  But I think what the code tries to avoid is failing
to adjust a use.  But I can't think of a case that isn't handled if it properly
replaces uses in address-taking operations (and asms).

For example it fails to walk PHI nodes where &var can appear as argument.

Otherwise it relies on walk_gimple_op and walk_tree which should work.

The other thing is aliasing though - if 'found' is TREE_ADDRESSABLE
then points-to sets may contain 'found' but they are not adjusted to
contain '<result>' afterwards.  Thus consider

 X a;
 X *p = &a;
 a.x = 1;
 p->x = ...;
 ... = a.x;
 return a;

where after replacing 'a' with '<result>' p->x will no longer alias the
store that now looks like <result>.x and thus we'd happily CSE
<result>.x across the pointer store.  Now NRV runs quite late
but we do preserve points-to information to RTL (and RTL expansion
handles stack slot sharing fine with points-to sets - but we'd need to
handle NRV the same here).

So ... unfortunately the patch is not safe as-is.

Richard.

>> 2014-10-16  Marc Glisse  <marc.glisse@inria.fr>
>>
>>       * tree-nrv.c (pass_nrv::execute): Don't disable when address is taken.
>>
>> --
>> Marc Glisse
>
>> Index: gcc/tree-nrv.c
>> ===================================================================
>> --- gcc/tree-nrv.c    (revision 216286)
>> +++ gcc/tree-nrv.c    (working copy)
>> @@ -210,21 +210,20 @@ pass_nrv::execute (function *fun)
>>                   return 0;
>>               }
>>             else
>>               found = rhs;
>>
>>             /* The returned value must be a local automatic variable of the
>>                same type and alignment as the function's result.  */
>>             if (TREE_CODE (found) != VAR_DECL
>>                 || TREE_THIS_VOLATILE (found)
>>                 || !auto_var_in_fn_p (found, current_function_decl)
>> -               || TREE_ADDRESSABLE (found)
>>                 || DECL_ALIGN (found) > DECL_ALIGN (result)
>>                 || !useless_type_conversion_p (result_type,
>>                                                TREE_TYPE (found)))
>>               return 0;
>>           }
>>         else if (gimple_has_lhs (stmt))
>>           {
>>             tree addr = get_base_address (gimple_get_lhs (stmt));
>>              /* If there's any MODIFY of component of RESULT,
>>                 then bail out.  */
>
>
>         Jakub

Re: NRV with address taken

[Marc Glisse]

On Thu, 16 Oct 2014, Richard Biener wrote:

> Does this fix PR63537?

PR63537 is already fine for me with trunk, NRV replaces ret with retval 
everywhere. It does so even if I add f(&ret); in the function with void 
f(vec*);

>> I'd worry if both result and found are address taken before the pass, then
>> trying to merge them together might mean something meant to have different
>> addresses collapses into the same object.
>
> I'd not worry about that.  But I think what the code tries to avoid is failing
> to adjust a use.  But I can't think of a case that isn't handled if it properly
> replaces uses in address-taking operations (and asms).
>
> For example it fails to walk PHI nodes where &var can appear as argument.
>
> Otherwise it relies on walk_gimple_op and walk_tree which should work.
>
> The other thing is aliasing though - if 'found' is TREE_ADDRESSABLE
> then points-to sets may contain 'found' but they are not adjusted to
> contain '<result>' afterwards.  Thus consider
>
> X a;
> X *p = &a;
> a.x = 1;
> p->x = ...;
> ... = a.x;
> return a;
>
> where after replacing 'a' with '<result>' p->x will no longer alias the
> store that now looks like <result>.x and thus we'd happily CSE
> <result>.x across the pointer store.  Now NRV runs quite late
> but we do preserve points-to information to RTL (and RTL expansion
> handles stack slot sharing fine with points-to sets - but we'd need to
> handle NRV the same here).

Ah, ok. It would be great to paste some of this in tree-nrv.c, unless you 
think it will be too much.

-- 
Marc Glisse

Re: NRV with address taken

[Richard Biener]

On Thu, Oct 16, 2014 at 11:03 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 16 Oct 2014, Richard Biener wrote:
>
>> Does this fix PR63537?
>
>
> PR63537 is already fine for me with trunk, NRV replaces ret with retval
> everywhere. It does so even if I add f(&ret); in the function with void
> f(vec*);
>
>>> I'd worry if both result and found are address taken before the pass,
>>> then
>>> trying to merge them together might mean something meant to have
>>> different
>>> addresses collapses into the same object.
>>
>>
>> I'd not worry about that.  But I think what the code tries to avoid is
>> failing
>> to adjust a use.  But I can't think of a case that isn't handled if it
>> properly
>> replaces uses in address-taking operations (and asms).
>>
>> For example it fails to walk PHI nodes where &var can appear as argument.
>>
>> Otherwise it relies on walk_gimple_op and walk_tree which should work.
>>
>> The other thing is aliasing though - if 'found' is TREE_ADDRESSABLE
>> then points-to sets may contain 'found' but they are not adjusted to
>> contain '<result>' afterwards.  Thus consider
>>
>> X a;
>> X *p = &a;
>> a.x = 1;
>> p->x = ...;
>> ... = a.x;
>> return a;
>>
>> where after replacing 'a' with '<result>' p->x will no longer alias the
>> store that now looks like <result>.x and thus we'd happily CSE
>> <result>.x across the pointer store.  Now NRV runs quite late
>> but we do preserve points-to information to RTL (and RTL expansion
>> handles stack slot sharing fine with points-to sets - but we'd need to
>> handle NRV the same here).
>
>
> Ah, ok. It would be great to paste some of this in tree-nrv.c, unless you
> think it will be too much.

I think it would be great to integrate NRV with RTL expansion instead
and thus handle the TREE_ADDRESSABLE case correct.  (simply
merge stack-slots of <retval> and 'found'!?)

Richard.

> --
> Marc Glisse