On Thu, Oct 16, 2014 at 9:31 AM, Jakub Jelinek <ja...@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.gli...@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
