Re: [PATCH 01/16] read-md.c: Add various cleanups to ~rtx_reader

[Richard Sandiford]

Sorry, haven't had time to read the full series yet, but:

David Malcolm  writes:
> On Wed, 2016-10-05 at 17:51 +0200, Bernd Schmidt wrote:
>> On 10/05/2016 06:14 PM, David Malcolm wrote:
>> > The selftests for the RTL frontend require supporting multiple
>> > reader instances being alive one after another in-process, so
>> > this lack of cleanup would become a leak.
>> 
>> > +  /* Initialize global data.  */
>> > +  obstack_init (_obstack);
>> > +  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p,
>> > 0);
>> > +  obstack_init (_loc_obstack);
>> > +  joined_conditions = htab_create (161, leading_ptr_hash,
>> > leading_ptr_eq_p, 0);
>> > +  obstack_init (_conditions_obstack);
>> > +  md_constants = htab_create (31, leading_string_hash,
>> > +leading_string_eq_p, (htab_del) 0);
>> > +  enum_types = htab_create (31, leading_string_hash,
>> > +  leading_string_eq_p, (htab_del) 0);
>> > +
>> > +  /* Unlock the stdio streams.  */
>> > +  unlock_std_streams ();
>> 
>> Hmm, but these are global statics. Shouldn't they first be moved to 
>> become class members?
>
> [CCing Richard Sandiford]
>
> I tried to move these into class rtx_reader, but doing so rapidly
> became quite invasive, with many of functions in the gen* tools
> becoming methods.

Is that just to avoid introducing explicit references to the global
rtx_reader object in the gen* tools?  If so, then TBH adding those
references sound better to me than tying generator-specific functions
to the rtx reader (not least because most of them do more than just
read rtl).

> Arguably that would be a good thing, but there are a couple of issues:
>
> (a) some of these functions take "vec" arguments; moving them from
> static functions to being class methods requires that vec.h has been
> included when the relevant class decl is parsed.

I don't think including vec.h more often should be a blocker though. :-)

> (b) rtx_reader turns into a bug dumping ground of methods, for the
> union of all of the various gen* tools.
>
> One way to alleviate these issues would be to do the split of
> rtx_reader into base_rtx_reader vs rtx_reader from patch 9 of the kit:
>   https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00273.html
> and perhaps to split out part of read-md.h into a new read-rtl.h.
>
> Before I reorganize the patches, does this approach sound reasonable?
>
> Alternatively, a less invasive approach is to have an accessor for
> these fields, so that things using them can get at them via the
> rtx_reader_ptr singleton e.g.:
>
> void
> grow_string_obstack (char ch)
> {
>obstack_1grow (rtx_reader_ptr->get_string_obstack (), ch);
> }
>
> and similar.

I think it's OK for the generators to refer rtx_reader_ptr directly.
Obviously that makes the patches more invasive, but hopefully the
extra changes are mechanical.

Thanks,
Richard

Re: [PATCH 01/16] read-md.c: Add various cleanups to ~rtx_reader

[David Malcolm]

On Wed, 2016-10-05 at 17:51 +0200, Bernd Schmidt wrote:
> On 10/05/2016 06:14 PM, David Malcolm wrote:
> > The selftests for the RTL frontend require supporting multiple
> > reader instances being alive one after another in-process, so
> > this lack of cleanup would become a leak.
> 
> > +  /* Initialize global data.  */
> > +  obstack_init (_obstack);
> > +  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p,
> > 0);
> > +  obstack_init (_loc_obstack);
> > +  joined_conditions = htab_create (161, leading_ptr_hash,
> > leading_ptr_eq_p, 0);
> > +  obstack_init (_conditions_obstack);
> > +  md_constants = htab_create (31, leading_string_hash,
> > + leading_string_eq_p, (htab_del) 0);
> > +  enum_types = htab_create (31, leading_string_hash,
> > +   leading_string_eq_p, (htab_del) 0);
> > +
> > +  /* Unlock the stdio streams.  */
> > +  unlock_std_streams ();
> 
> Hmm, but these are global statics. Shouldn't they first be moved to 
> become class members?

[CCing Richard Sandiford]

I tried to move these into class rtx_reader, but doing so rapidly
became quite invasive, with many of functions in the gen* tools
becoming methods.

Arguably that would be a good thing, but there are a couple of issues:

(a) some of these functions take "vec" arguments; moving them from
static functions to being class methods requires that vec.h has been
included when the relevant class decl is parsed.

(b) rtx_reader turns into a bug dumping ground of methods, for the
union of all of the various gen* tools.

One way to alleviate these issues would be to do the split of
rtx_reader into base_rtx_reader vs rtx_reader from patch 9 of the kit:
  https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00273.html
and perhaps to split out part of read-md.h into a new read-rtl.h.

Before I reorganize the patches, does this approach sound reasonable?

Alternatively, a less invasive approach is to have an accessor for
these fields, so that things using them can get at them via the
rtx_reader_ptr singleton e.g.:

void
grow_string_obstack (char ch)
{
   obstack_1grow (rtx_reader_ptr->get_string_obstack (), ch);
}

and similar.

Thoughts?
Dave

Re: [PATCH 01/16] read-md.c: Add various cleanups to ~rtx_reader

[Bernd Schmidt]

On 10/05/2016 06:14 PM, David Malcolm wrote:

The selftests for the RTL frontend require supporting multiple
reader instances being alive one after another in-process, so
this lack of cleanup would become a leak.



+  /* Initialize global data.  */
+  obstack_init (_obstack);
+  ptr_locs = htab_create (161, leading_ptr_hash, leading_ptr_eq_p, 0);
+  obstack_init (_loc_obstack);
+  joined_conditions = htab_create (161, leading_ptr_hash, leading_ptr_eq_p, 0);
+  obstack_init (_conditions_obstack);
+  md_constants = htab_create (31, leading_string_hash,
+ leading_string_eq_p, (htab_del) 0);
+  enum_types = htab_create (31, leading_string_hash,
+   leading_string_eq_p, (htab_del) 0);
+
+  /* Unlock the stdio streams.  */
+  unlock_std_streams ();


Hmm, but these are global statics. Shouldn't they first be moved to 
become class members?



Bernd