Re: NRV with address taken
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.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
Re: NRV with address taken
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
Re: NRV with address taken
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
On Thu, Oct 16, 2014 at 11:03 AM, Marc Glisse marc.gli...@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
NRV with address taken
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.gli...@inria.fr
* tree-nrv.c (pass_nrv::execute): Don't disable when address is taken.
--
Marc GlisseIndex: 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. */
