Re: [PATCH] Add gimple-compat.h (was Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign)

[Jeff Law <law at redhat dot com>]

On 11/14/14 08:27, David Malcolm wrote:
> > > > > I just don't like all the as_a/is_a stuff enforced everywhere,
> > > > > it means more typing, more temporaries, more indentation.
> > > > > So, as I view it, instead of the checks being done cheaply (yes, I think
> > > > > the gimple checking as we have right now is very cheap) under the
> > > > > hood by the accessors (gimple_assign_{lhs,rhs1} etc.), those changes
> > > > > put the burden on the developers, who has to check that manually through
> > > > > the as_a/is_a stuff everywhere, more typing and uglier syntax.
> > > > > I just don't see that as a step forward, instead a huge step backwards.
> > > > > But perhaps I'm alone with this.
> > > > > Can you e.g. compare the size of - lines in your patchset combined, and
> > > > > size of + lines in your patchset?  As in, if your changes lead to less
> > > > > typing or more.
So, I'm chiming in a bit late, but just want to touch on a few things.

First, as I've stated before, I see as_a/is_a as generally a wart for 
things we still need to cleanup and redesign.  I do not want to see them 
sprinkled throughout GCC.  If we find ourselves adding a bunch of these, 
then we've got some redesign/rethinking that needs to be done.

Yes, I know some will be necessary and some are more like markers for 
the limits of where we are with the gimple class work, particularly 
since we're trying to stage in this work rather than convert everything 
at once (which I believe, realistically, is impossible).


> > > > I see two ways out here.  One is to add overloads to all the functions
> > > > taking the special types like
IIRC Andrew was doing similar things as a temporary measure in the 
gimple/tree type work as well.  Basically the overloads were to allow 
the two schemes to co-exist while conversions were done with the express 
intent that the overloads were to disappear when conversion is complete. 
 I'd be comfortable with a similar mechanism for this work as well.

> > > Option 3: only convert the "easy" accessors: the ones I already did in
> > > the /89 patch kit, as reviewed by Jeff, and rebased by me recently,
> > > which is this 92-patch kit:
> > >    "[gimple-classes, committed 00/92] Initial slew of commits":
> > >       https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html
> > > Doing so converts about half of the gimple_foo_ accessors to taking a
> > > gfoo *, giving a mixture of GIMPLE_CHECK vs subclass use.   I believe
> > > the quality of those patches was higher than the later ones on the
> > > branch: I was doing the places that didn't require the invasive/verbose
> > > changes seen in the later patches.  Shelve the remaining ~80
> > > increasingly ugly patches, starting a new branch to contain just the
> > > good ones.
And this would be my preferred option for where we are today.  That ~89 
kit was a step in the right direction.  I think going beyond that for 
the close of stage1 was ambitious :-)

> > Works for me as well.  The compat solution looks somewhat appealing
> > as we can then incrementally fix up things rather than requiring to
> > mass-convert everything.
Exactly.  And it's real easy to see what's depending on those overloads 
as one can simply remove them and try to build.  In many ways, I'd 
prefer the temporary overload solution for the next round of work in 
this space so that conversions can occur piecemeal instead of in large 
series patchsets.

I've got no objection if we have the compat hack in now.   I haven't 
reviewed that work, just no philosophical objections :-)  I would be 
opposed to pushing the gimple class work further than that prior to the 
next stage1 opening.


jeff