Updated automated patch (was Re: [PATCH 3/6] Automated part of conversion of gimple types to use C++ inheritance)

On Thu, 2013-11-14 at 00:48 -0700, Jeff Law wrote:
> On 10/31/13 10:26, David Malcolm wrote:
> > gcc/
> >
> > 	Patch autogenerated by refactor_gimple.py from
> > 	https://github.com/davidmalcolm/gcc-refactoring-scripts
> > 	revision 74cd3d5f06565c318749d0fb9f35b565dae28daa
> [ ... ]
> This is fine with the usual conditions.

This patch has bitrotten somewhat against trunk due to the
reorganization of gimple.h and related headers.

I regenerated it and am bootstrapping now.  I glanced over it and
nothing major seems to have changed; just changes due to the movement of
code between files.   Am attaching the changed patch.

>   diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c
> > index e430050..ed0d6df 100644
> > --- a/gcc/gimple-iterator.c
> > +++ b/gcc/gimple-iterator.c
> > @@ -67,7 +67,7 @@ update_bb_for_stmts (gimple_seq_node first, gimple_seq_node last,
> >   {
> >     gimple_seq_node n;
> >
> > -  for (n = first; n; n = n->gsbase.next)
> > +  for (n = first; n; n = n->next)
> So just a quite note.  If I'm reading this corectly, this should make 
> things marginally easier in the debugger when looking at objects?  It 
> drives me absolutely nuts having to figure out how to get through the 
> base class to the fields I care about.

I think so, yes, though you'll have to cast it to the appropriate
subclass by hand; rather than the status quo of getting multiple
screenfuls of text, you'll just get the gimple_statement_base fields:

(gdb) p stmt
$9 = <gimple_assign 0x7ffff0450000>
(gdb) p *(gimple_statement_with_memory_ops*)stmt
$10 = {<gimple_statement_with_memory_ops_base> =
{<gimple_statement_with_ops_base> = {<gimple_statement_base> = {code =
GIMPLE_ASSIGN, no_warning = 0, 
        visited = 0, nontemporal_move = 0, plf = 1, modified = 0,
has_volatile_ops = 0, subcode = 67, uid = 0, location = 2147483648,
num_ops = 3, bb = 
    <basic_block 0x7ffff042a1a0 (2)>, next = <gimple 0x0>, prev =
<gimple_assign 0x7ffff0450000>}, use_ops = 0x7ffff0452008}, vdef = <tree
0x0>, vuse = 
    <tree 0x0>}, op = {0x7ffff030acf0}}

This would clearly be nicer with the followup of having an (empty)
subclass for assignments, so that one could do:
(gdb) p *(gimple_statement_assign*)stmt

We may be able to automate printing the appropriate subclass in
gdbhooks.py

> I didn't look at every hunk in this patch carefully.  Just spot checked 
> thigns.
> 
> 
> >   }
> >
> >   /* Set the nowait flag on OMP_RETURN statement S.  */
> > @@ -1661,7 +1973,7 @@ static inline void
> >   gimple_omp_return_set_nowait (gimple s)
> >   {
> >     GIMPLE_CHECK (s, GIMPLE_OMP_RETURN);
> > -  s->gsbase.subcode |= GF_OMP_RETURN_NOWAIT;
> > +  s->subcode |= GF_OMP_RETURN_NOWAIT;
> So is there some reason the GIMPLE_CHECK was left in here rather than 
> doing the downcasting?  This happens in other places.

The script isn't particularly smart, and here it's only removing the
"->gsbase." part.  It doesn't automatically remove GIMPLE_CHECK uses: it
only introduces a downcast if it needs to, and given that this accessor
only pokes at base class things it didn't.  Hence the existing
typechecking is preserved.

I want to convert accessors like this into taking a subclass ptr, hence
it could eventually become:
  gimple_omp_return_set_nowait (gimple_omp_return *s)
(there are only two uses of it in the tree) and at that point, the
GIMPLE_CHECK can be removed (with the type-checking enforced at compile
time).

> > @@ -1681,8 +1993,9 @@ gimple_omp_return_nowait_p (const_gimple g)
> >   static inline void
> >   gimple_omp_return_set_lhs (gimple g, tree lhs)
> >   {
> > -  GIMPLE_CHECK (g, GIMPLE_OMP_RETURN);
> > -  g->gimple_omp_atomic_store.val = lhs;
> > +  gimple_statement_omp_atomic_store *omp_atomic_store_stmt =
> > +    as_a <gimple_statement_omp_atomic_store> (g);
> > +  omp_atomic_store_stmt->val = lhs;
> Contrast to prior hunk.  This one, AFAICT elimates the GIMPLE_CHECK here 
> and does it as part of the downcasting, right?

Yes: this accessor make use of fields of the subclass: specifically the
val within the omp_atomic_store, and hence needs to do the checked
downcast ("as_a").  Given that as_a performs equivalent runtime checking
to GIMPLE_CHECK, I opted for the script to remove the GIMPLE_CHECK in
such cases.

> I wonder how far we have to go with this before GIMPLE_CHECK goes away :-)
> 
> >
> > @@ -1723,7 +2038,7 @@ static inline void
> >   gimple_omp_section_set_last (gimple g)
> >   {
> >     GIMPLE_CHECK (g, GIMPLE_OMP_SECTION);
> > -  g->gsbase.subcode |= GF_OMP_SECTION_LAST;
> > +  g->subcode |= GF_OMP_SECTION_LAST;
> Another example of the GIMPLE_CHECK hanging around.  On purpose?

Again, given that we only poke at "subcode", no as_a is needed, and
hence the script keeps the GIMPLE_CHECK.

FWIW, this particular accessor is only used in one place
(omp-low.c:lower_omp_sections).

> >
> > @@ -1746,9 +2061,9 @@ gimple_omp_parallel_set_combined_p (gimple g, bool combined_p)
> >   {
> >     GIMPLE_CHECK (g, GIMPLE_OMP_PARALLEL);
> >     if (combined_p)
> > -    g->gsbase.subcode |= GF_OMP_PARALLEL_COMBINED;
> > +    g->subcode |= GF_OMP_PARALLEL_COMBINED;
> >     else
> > -    g->gsbase.subcode &= ~GF_OMP_PARALLEL_COMBINED;
> > +    g->subcode &= ~GF_OMP_PARALLEL_COMBINED;
> Likewise.
...and again, this one only pokes at "subcode", which is in the base
class.  Likewise, this accessor is only used in one place
(omp-low.c:lower_omp_taskreg), and could be changed in a followup to
accept a gimple_omp_parallel *, dropping the GIMPLE_CHECK.

> > @@ -1771,7 +2086,7 @@ gimple_omp_atomic_set_need_value (gimple g)
> >   {
> >     if (gimple_code (g) != GIMPLE_OMP_ATOMIC_LOAD)
> >       GIMPLE_CHECK (g, GIMPLE_OMP_ATOMIC_STORE);
> > -  g->gsbase.subcode |= GF_OMP_ATOMIC_NEED_VALUE;
> > +  g->subcode |= GF_OMP_ATOMIC_NEED_VALUE;
> Likewise.
...likewise this only looks at subcode and hence doesn't need a
downcast.  It's used in 2 places, both within gimplify_omp_atomic, both
of which can trivially be converted to work on subclass ptrs (the
statements in question come directly from
gimple_build_omp_atomice_{load|store} calls).

> And so-on.
> 
> I don't see anything objectionable.  Just want to make sure the script 
> and/or the by-hand stuff didn't miss some of the conversions.

Thanks.   I'm attaching the regenerated patch.  As noted above, I
believe that the only changes are due to functions moving between source
files; specifically the move of the following from gimple.h to
gimple-iterator.h (quoting the ChangeLog):
	* gimple-iterator.h (gsi_one_before_end_p): Likewise.
	(gsi_next): Likewise.
	(gsi_prev): Likewise.

OK for trunk?

Updated automated patch (was Re: [PATCH 3/6] Automated part of conversion of gimple types to use C++ inheritance)

Commit Message

Comments

Patch