| Message ID | alpine.DEB.2.02.1410160711190.30950@stedding.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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
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. */