Re: NRV with address taken

[Richard Biener <richard dot guenther at gmail dot com>]

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