[PATCH] Updated automated patch (was Re: [PATCH 3/6] Automated part of conversion of gimple types to use C++ inheritance)
David Malcolm
dmalcolm@redhat.com
Mon Nov 18 22:17:00 GMT 2013
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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Automated-part-of-conversion-of-gimple-types-to-use-.patch
Type: text/x-patch
Size: 121672 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20131118/2a5fa342/attachment.bin>
More information about the Gcc-patches
mailing list