[patch] ree.c: don't abuse GTY

[patch] ree.c: don't abuse GTY

[Steven Bosscher]

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 :-)

        * 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 */
  }
 };

Re: [patch] ree.c: don't abuse GTY

[Richard Biener]

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 */
>   }
>  };

Re: [patch] ree.c: don't abuse GTY

[Steven Bosscher]

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 :-) )

Re: [patch] ree.c: don't abuse GTY

[Steven Bosscher]

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

Re: [patch] ree.c: don't abuse GTY

[Jakub Jelinek]

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

Re: [patch] ree.c: don't abuse GTY

[Steven Bosscher]

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

Re: [patch] ree.c: don't abuse GTY

[Jakub Jelinek]

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

Re: [patch] ree.c: don't abuse GTY

[Steven Bosscher]

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