| Message ID | or1tcy3zds.fsf@livre.home |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 14, 2015 at 5:25 AM, Alexandre Oliva <aoliva@redhat.com> wrote: > On Oct 12, 2015, Richard Biener <richard.guenther@gmail.com> wrote: > >> On Sat, Oct 10, 2015 at 3:16 PM, Alexandre Oliva <aoliva@redhat.com> wrote: >>> On Oct 9, 2015, Richard Biener <richard.guenther@gmail.com> wrote: >>> >>>> Ok. Note that I think emit_block_move shouldn't mess with the addressable flag. >>> >>> I have successfully tested a patch that stops it from doing so, >>> reverting https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49429#c11 but >>> according to bugs 49429 and 49454, it looks like removing it would mess >>> with escape analysis introduced in r175063 for bug 44194. The thread >>> that introduces the mark_addressable calls suggests some discomfort with >>> this solution, and even a suggestion that the markings should be >>> deferred past the end of expand, but in the end there was agreement to >>> go with it. https://gcc.gnu.org/ml/gcc-patches/2011-06/msg01746.html > >> Aww, indeed. Of course the issue is that we don't track pointers to the >> stack introduced during RTL properly. > >> Thanks for checking. Might want to add a comment before that >> addressable setting now that you've done the archeology. > > I decided to give the following approach a try instead. The following > patch was regstrapped on x86_64-linux-gnu and i686-linux-gnu. > Ok to install? It looks ok to me but lacks a comment in mark_addressable_1 why we do this queueing when currently expanding to RTL. Richard. > Would anyone with access to hpux (pa and ia64 are both affected) give it > a spin? > > > defer mark_addressable calls during expand till the end of expand > > From: Alexandre Oliva <aoliva@redhat.com> > > for gcc/ChangeLog > > * gimple-expr.c: Include hash-set.h and rtl.h. > (mark_addressable_queue): New var. > (mark_addressable): Factor actual marking into... > (mark_addressable_1): ... this. Queue it up during expand. > (mark_addressable_2): New. > (flush_mark_addressable_queue): New. > * gimple-expr.h (flush_mark_addressable_queue): Declare. > * cfgexpand.c: Include gimple-expr.h. > (pass_expand::execute): Flush mark_addressable queue. > --- > gcc/cfgexpand.c | 3 +++ > gcc/gimple-expr.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++-- > gcc/gimple-expr.h | 1 + > 3 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index eaad859..a362e17 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -51,6 +51,7 @@ along with GCC; see the file COPYING3. If not see > #include "internal-fn.h" > #include "tree-eh.h" > #include "gimple-iterator.h" > +#include "gimple-expr.h" > #include "gimple-walk.h" > #include "cgraph.h" > #include "tree-cfg.h" > @@ -6373,6 +6374,8 @@ pass_expand::execute (function *fun) > /* We're done expanding trees to RTL. */ > currently_expanding_to_rtl = 0; > > + flush_mark_addressable_queue (); > + > FOR_BB_BETWEEN (bb, ENTRY_BLOCK_PTR_FOR_FN (fun)->next_bb, > EXIT_BLOCK_PTR_FOR_FN (fun), next_bb) > { > diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c > index 2a6ba1a..db249a3 100644 > --- a/gcc/gimple-expr.c > +++ b/gcc/gimple-expr.c > @@ -35,6 +35,8 @@ along with GCC; see the file COPYING3. If not see > #include "gimplify.h" > #include "stor-layout.h" > #include "demangle.h" > +#include "hash-set.h" > +#include "rtl.h" > > /* ----- Type related ----- */ > > @@ -823,6 +825,50 @@ is_gimple_mem_ref_addr (tree t) > || decl_address_invariant_p (TREE_OPERAND (t, 0))))); > } > > +/* Hold trees marked addressable during expand. */ > + > +static hash_set<tree> *mark_addressable_queue; > + > +/* Mark X as addressable or queue it up if called during expand. */ > + > +static void > +mark_addressable_1 (tree x) > +{ > + if (!currently_expanding_to_rtl) > + { > + TREE_ADDRESSABLE (x) = 1; > + return; > + } > + > + if (!mark_addressable_queue) > + mark_addressable_queue = new hash_set<tree>(); > + mark_addressable_queue->add (x); > +} > + > +/* Adaptor for mark_addressable_1 for use in hash_set traversal. */ > + > +bool > +mark_addressable_2 (tree const &x, void * ATTRIBUTE_UNUSED = NULL) > +{ > + mark_addressable_1 (x); > + return false; > +} > + > +/* Mark all queued trees as addressable, and empty the queue. To be > + called right after clearing CURRENTLY_EXPANDING_TO_RTL. */ > + > +void > +flush_mark_addressable_queue () > +{ > + gcc_assert (!currently_expanding_to_rtl); > + if (mark_addressable_queue) > + { > + mark_addressable_queue->traverse<void*, mark_addressable_2> (NULL); > + delete mark_addressable_queue; > + mark_addressable_queue = NULL; > + } > +} > + > /* Mark X addressable. Unlike the langhook we expect X to be in gimple > form and we don't do any syntax checking. */ > > @@ -838,7 +884,7 @@ mark_addressable (tree x) > && TREE_CODE (x) != PARM_DECL > && TREE_CODE (x) != RESULT_DECL) > return; > - TREE_ADDRESSABLE (x) = 1; > + mark_addressable_1 (x); > > /* Also mark the artificial SSA_NAME that points to the partition of X. */ > if (TREE_CODE (x) == VAR_DECL > @@ -849,7 +895,7 @@ mark_addressable (tree x) > { > tree *namep = cfun->gimple_df->decls_to_pointers->get (x); > if (namep) > - TREE_ADDRESSABLE (*namep) = 1; > + mark_addressable_1 (*namep); > } > } > > diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h > index 3d1c89f..2917d2752c 100644 > --- a/gcc/gimple-expr.h > +++ b/gcc/gimple-expr.h > @@ -52,6 +52,7 @@ extern bool is_gimple_asm_val (tree); > extern bool is_gimple_min_lval (tree); > extern bool is_gimple_call_addr (tree); > extern bool is_gimple_mem_ref_addr (tree); > +extern void flush_mark_addressable_queue (void); > extern void mark_addressable (tree); > extern bool is_gimple_reg_rhs (tree); > > > > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
On Oct 14, 2015, Richard Biener <richard.guenther@gmail.com> wrote: > On Wed, Oct 14, 2015 at 5:25 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >> On Oct 12, 2015, Richard Biener <richard.guenther@gmail.com> wrote: >> >>> On Sat, Oct 10, 2015 at 3:16 PM, Alexandre Oliva <aoliva@redhat.com> wrote: >>>> On Oct 9, 2015, Richard Biener <richard.guenther@gmail.com> wrote: >>>> >>>>> Ok. Note that I think emit_block_move shouldn't mess with the addressable flag. >>>> >>>> I have successfully tested a patch that stops it from doing so, >>>> reverting https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49429#c11 but >>>> according to bugs 49429 and 49454, it looks like removing it would mess >>>> with escape analysis introduced in r175063 for bug 44194. The thread >>>> that introduces the mark_addressable calls suggests some discomfort with >>>> this solution, and even a suggestion that the markings should be >>>> deferred past the end of expand, but in the end there was agreement to >>>> go with it. https://gcc.gnu.org/ml/gcc-patches/2011-06/msg01746.html >> >>> Aww, indeed. Of course the issue is that we don't track pointers to the >>> stack introduced during RTL properly. >> >>> Thanks for checking. Might want to add a comment before that >>> addressable setting now that you've done the archeology. >> >> I decided to give the following approach a try instead. The following >> patch was regstrapped on x86_64-linux-gnu and i686-linux-gnu. >> Ok to install? > It looks ok to me but lacks a comment in mark_addressable_1 why we > do this queueing when currently expanding to RTL. >> +/* Mark X as addressable or queue it up if called during expand. */ >> + >> +static void >> +mark_addressable_1 (tree x) How about this: /* Mark X as addressable or queue it up if called during expand. We don't want to apply it immediately during expand because decls are made addressable at that point due to RTL-only concerns, such as uses of memcpy for block moves, and TREE_ADDRESSABLE changes is_gimple_reg, which might make it seem like a variable that used to be a gimple_reg shouldn't have been an SSA name. So we queue up this flag setting and only apply it when we're done with GIMPLE and only RTL issues matter. */
On 11/02/2015 06:11 PM, Alexandre Oliva wrote: > On Oct 14, 2015, Richard Biener <richard.guenther@gmail.com> wrote: > >> On Wed, Oct 14, 2015 at 5:25 AM, Alexandre Oliva <aoliva@redhat.com> wrote: >>> On Oct 12, 2015, Richard Biener <richard.guenther@gmail.com> wrote: >>> >>>> On Sat, Oct 10, 2015 at 3:16 PM, Alexandre Oliva <aoliva@redhat.com> wrote: >>>>> On Oct 9, 2015, Richard Biener <richard.guenther@gmail.com> wrote: >>>>> >>>>>> Ok. Note that I think emit_block_move shouldn't mess with the addressable flag. >>>>> >>>>> I have successfully tested a patch that stops it from doing so, >>>>> reverting https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49429#c11 but >>>>> according to bugs 49429 and 49454, it looks like removing it would mess >>>>> with escape analysis introduced in r175063 for bug 44194. The thread >>>>> that introduces the mark_addressable calls suggests some discomfort with >>>>> this solution, and even a suggestion that the markings should be >>>>> deferred past the end of expand, but in the end there was agreement to >>>>> go with it. https://gcc.gnu.org/ml/gcc-patches/2011-06/msg01746.html >>> >>>> Aww, indeed. Of course the issue is that we don't track pointers to the >>>> stack introduced during RTL properly. >>> >>>> Thanks for checking. Might want to add a comment before that >>>> addressable setting now that you've done the archeology. >>> >>> I decided to give the following approach a try instead. The following >>> patch was regstrapped on x86_64-linux-gnu and i686-linux-gnu. >>> Ok to install? > >> It looks ok to me but lacks a comment in mark_addressable_1 why we >> do this queueing when currently expanding to RTL. > >>> +/* Mark X as addressable or queue it up if called during expand. */ >>> + >>> +static void >>> +mark_addressable_1 (tree x) > > How about this: > > /* Mark X as addressable or queue it up if called during expand. We > don't want to apply it immediately during expand because decls are > made addressable at that point due to RTL-only concerns, such as > uses of memcpy for block moves, and TREE_ADDRESSABLE changes > is_gimple_reg, which might make it seem like a variable that used > to be a gimple_reg shouldn't have been an SSA name. So we queue up > this flag setting and only apply it when we're done with GIMPLE and > only RTL issues matter. */ Sounds good. jeff
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index eaad859..a362e17 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -51,6 +51,7 @@ along with GCC; see the file COPYING3. If not see #include "internal-fn.h" #include "tree-eh.h" #include "gimple-iterator.h" +#include "gimple-expr.h" #include "gimple-walk.h" #include "cgraph.h" #include "tree-cfg.h" @@ -6373,6 +6374,8 @@ pass_expand::execute (function *fun) /* We're done expanding trees to RTL. */ currently_expanding_to_rtl = 0; + flush_mark_addressable_queue (); + FOR_BB_BETWEEN (bb, ENTRY_BLOCK_PTR_FOR_FN (fun)->next_bb, EXIT_BLOCK_PTR_FOR_FN (fun), next_bb) { diff --git a/gcc/gimple-expr.c b/gcc/gimple-expr.c index 2a6ba1a..db249a3 100644 --- a/gcc/gimple-expr.c +++ b/gcc/gimple-expr.c @@ -35,6 +35,8 @@ along with GCC; see the file COPYING3. If not see #include "gimplify.h" #include "stor-layout.h" #include "demangle.h" +#include "hash-set.h" +#include "rtl.h" /* ----- Type related ----- */ @@ -823,6 +825,50 @@ is_gimple_mem_ref_addr (tree t) || decl_address_invariant_p (TREE_OPERAND (t, 0))))); } +/* Hold trees marked addressable during expand. */ + +static hash_set<tree> *mark_addressable_queue; + +/* Mark X as addressable or queue it up if called during expand. */ + +static void +mark_addressable_1 (tree x) +{ + if (!currently_expanding_to_rtl) + { + TREE_ADDRESSABLE (x) = 1; + return; + } + + if (!mark_addressable_queue) + mark_addressable_queue = new hash_set<tree>(); + mark_addressable_queue->add (x); +} + +/* Adaptor for mark_addressable_1 for use in hash_set traversal. */ + +bool +mark_addressable_2 (tree const &x, void * ATTRIBUTE_UNUSED = NULL) +{ + mark_addressable_1 (x); + return false; +} + +/* Mark all queued trees as addressable, and empty the queue. To be + called right after clearing CURRENTLY_EXPANDING_TO_RTL. */ + +void +flush_mark_addressable_queue () +{ + gcc_assert (!currently_expanding_to_rtl); + if (mark_addressable_queue) + { + mark_addressable_queue->traverse<void*, mark_addressable_2> (NULL); + delete mark_addressable_queue; + mark_addressable_queue = NULL; + } +} + /* Mark X addressable. Unlike the langhook we expect X to be in gimple form and we don't do any syntax checking. */ @@ -838,7 +884,7 @@ mark_addressable (tree x) && TREE_CODE (x) != PARM_DECL && TREE_CODE (x) != RESULT_DECL) return; - TREE_ADDRESSABLE (x) = 1; + mark_addressable_1 (x); /* Also mark the artificial SSA_NAME that points to the partition of X. */ if (TREE_CODE (x) == VAR_DECL @@ -849,7 +895,7 @@ mark_addressable (tree x) { tree *namep = cfun->gimple_df->decls_to_pointers->get (x); if (namep) - TREE_ADDRESSABLE (*namep) = 1; + mark_addressable_1 (*namep); } } diff --git a/gcc/gimple-expr.h b/gcc/gimple-expr.h index 3d1c89f..2917d2752c 100644 --- a/gcc/gimple-expr.h +++ b/gcc/gimple-expr.h @@ -52,6 +52,7 @@ extern bool is_gimple_asm_val (tree); extern bool is_gimple_min_lval (tree); extern bool is_gimple_call_addr (tree); extern bool is_gimple_mem_ref_addr (tree); +extern void flush_mark_addressable_queue (void); extern void mark_addressable (tree); extern bool is_gimple_reg_rhs (tree);