Message ID | CABu31nO20-qqcMU68LihzsSC+cXs7n=ppkB6RgAZVn4gwuONBA@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Fri, Dec 7, 2012 at 1:05 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > Hello, > > ree.c's data structures are not GC-allocated, so its GTY markers and > its TODO_ggc_collect are unnecessary. > > Will commit as obvious after the usual testing on my favourite platforms :-) IMHO TODO_ggc_collect should go and we should collect after each pass run. Richard. > * ree.c (struct ext_cand): Remove GTY markers. > (pass_ree): Remove TODO_ggc_collect. > > Index: ree.c > =================================================================== > --- ree.c (revision 194247) > +++ ree.c (working copy) > @@ -243,7 +243,7 @@ > > /* This structure represents a candidate for elimination. */ > > -typedef struct GTY(()) ext_cand > +typedef struct ext_cand > { > /* The expression. */ > const_rtx expr; > @@ -958,7 +958,6 @@ > 0, /* properties_provided */ > 0, /* properties_destroyed */ > 0, /* todo_flags_start */ > - TODO_ggc_collect | > TODO_verify_rtl_sharing, /* todo_flags_finish */ > } > };
On Fri, Dec 7, 2012 at 10:08 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Dec 7, 2012 at 1:05 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >> Hello, >> >> ree.c's data structures are not GC-allocated, so its GTY markers and >> its TODO_ggc_collect are unnecessary. >> >> Will commit as obvious after the usual testing on my favourite platforms :-) > > IMHO TODO_ggc_collect should go and we should collect after each pass run. IOPEHO (*) ggc_collect should go, period. I don't think that running it after each pass would be a good start: It would make any serious test with GCAC checking enabled almost impossible. In the mean time -- comments on my patch? Ciao! Steven (* In Other Peope's Equally Humble Opinions :-) )
On Fri, Dec 7, 2012 at 10:42 AM, Steven Bosscher wrote:
> In the mean time -- comments on my patch?
Oh, never mind, I forgot I was going to commit this as obvious :-)
Ciao!
Steven
On Fri, Dec 07, 2012 at 10:51:39AM +0100, Steven Bosscher wrote: > On Fri, Dec 7, 2012 at 10:42 AM, Steven Bosscher wrote: > > In the mean time -- comments on my patch? > > Oh, never mind, I forgot I was going to commit this as obvious :-) I'd say removing the GTY(()) on a struct that is never GC allocated nor interesting to PCH is obvious, the removal of TODO_ggc_collect is not so. Right now it is set by passes that expect to potentially create enough GC garbage that a collection might be desirable. You haven't explained why you think it is the case of ree.c. Jakub
On Fri, Dec 7, 2012 at 10:57 AM, Jakub Jelinek wrote: > I'd say removing the GTY(()) on a struct that is never GC allocated > nor interesting to PCH is obvious, the removal of TODO_ggc_collect is not > so. Right now it is set by passes that expect to potentially > create enough GC garbage that a collection might be desirable. > You haven't explained why you think it is the case of ree.c. You're right, I should have explained it. Perhaps somewhere during the development of ree.c struct ext_cand was GGC-allocated, in which case TODO_ggc_collect would make sense, to collect the ext_cands (there can be many of them). But since ext_cands are not GGC-allocated, the only garbage "produced" by ree.c is when it removes redundant extensions (via delete_insn). This is typically only a very small percentage of the instructions, so there isn't a lot of garbage produced. I'll leave the TODO in place, and only remove the GTY marker from struct ext_cand. Ciao! Steven
On Fri, Dec 07, 2012 at 11:25:38AM +0100, Steven Bosscher wrote: > Perhaps somewhere during the development of ree.c struct ext_cand was > GGC-allocated, in which case TODO_ggc_collect would make sense, to > collect the ext_cands (there can be many of them). But since ext_cands > are not GGC-allocated, the only garbage "produced" by ree.c is when it > removes redundant extensions (via delete_insn). This is typically only > a very small percentage of the instructions, so there isn't a lot of > garbage produced. That isn't the only garbage produced, it also updates the PATTERNs of various instructions with something different. Have you done any measurement on what kind of percentage of the instructions is modified, or is "very small percentage" just a guess? E.g. on x86_64 it changes quite a lot of instructions if I remember well. I'm not saying we can't live without GC collection after REE, just saying it isn't at all obvious. Jakub
On Fri, Dec 7, 2012 at 11:34 AM, Jakub Jelinek wrote: > On Fri, Dec 07, 2012 at 11:25:38AM +0100, Steven Bosscher wrote: >> Perhaps somewhere during the development of ree.c struct ext_cand was >> GGC-allocated, in which case TODO_ggc_collect would make sense, to >> collect the ext_cands (there can be many of them). But since ext_cands >> are not GGC-allocated, the only garbage "produced" by ree.c is when it >> removes redundant extensions (via delete_insn). This is typically only >> a very small percentage of the instructions, so there isn't a lot of >> garbage produced. > > That isn't the only garbage produced, it also updates the PATTERNs of > various instructions with something different. Have you done any > measurement on what kind of percentage of the instructions is modified, > or is "very small percentage" just a guess? I almost never guess, and try to measure the impact of my patches. In this case, I couldn't even get the garbage collector to trigger without bringing the collection threshold down by a factor 10 or so. Ciao! Steven
Index: ree.c =================================================================== --- ree.c (revision 194247) +++ ree.c (working copy) @@ -243,7 +243,7 @@ /* This structure represents a candidate for elimination. */ -typedef struct GTY(()) ext_cand +typedef struct ext_cand { /* The expression. */ const_rtx expr; @@ -958,7 +958,6 @@ 0, /* properties_provided */ 0, /* properties_destroyed */ 0, /* todo_flags_start */ - TODO_ggc_collect | TODO_verify_rtl_sharing, /* todo_flags_finish */ } };