This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign


On Tue, 2014-11-11 at 11:43 +0100, Richard Biener wrote:
> On Tue, Nov 11, 2014 at 8:26 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Mon, Nov 10, 2014 at 05:27:50PM -0500, David Malcolm wrote:
> >> On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote:
> >> > On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote:
> >> > > To be constructive here - the above case is from within a
> >> > > GIMPLE_ASSIGN case label
> >> > > and thus I'd have expected
> >> > >
> >> > >     case GIMPLE_ASSIGN:
> >> > >       {
> >> > >         gassign *a1 = as_a <gassign *> (s1);
> >> > >         gassign *a2 = as_a <gassign *> (s2);
> >> > >       lhs1 = gimple_assign_lhs (a1);
> >> > >       lhs2 = gimple_assign_lhs (a2);
> >> > >       if (TREE_CODE (lhs1) != SSA_NAME
> >> > >           && TREE_CODE (lhs2) != SSA_NAME)
> >> > >         return (operand_equal_p (lhs1, lhs2, 0)
> >> > >                 && gimple_operand_equal_value_p (gimple_assign_rhs1 (a1),
> >> > >                                                  gimple_assign_rhs1 (a2)));
> >> > >       else if (TREE_CODE (lhs1) == SSA_NAME
> >> > >                && TREE_CODE (lhs2) == SSA_NAME)
> >> > >         return vn_valueize (lhs1) == vn_valueize (lhs2);
> >> > >       return false;
> >> > >       }
> >> > >
> >> > > instead.  That's the kind of changes I have expected and have approved of.
> >> >
> >> > But even that looks like just adding extra work for all developers, with no
> >> > gain.  You only have to add extra code and extra temporaries, in switches
> >> > typically also have to add {} because of the temporaries and thus extra
> >> > indentation level, and it doesn't simplify anything in the code.
> >>
> >> The branch attempts to use the C++ typesystem to capture information
> >> about the kinds of gimple statement we expect, both:
> >>   (A) so that the compiler can detect type errors, and
> >>   (B) as a comprehension aid to the human reader of the code
> >>
> >> The ideal here is when function params and struct field can be
> >> strengthened from "gimple" to a subclass ptr.  This captures the
> >> knowledge that every use of a function or within a struct has a given
> >> gimple code.
> >
> > 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.
> 
> I see two ways out here.  One is to add overloads to all the functions
> taking the special types like
> 
> tree
> gimple_assign_rhs1 (gimple *);
> 
> or simply add
> 
> gassign *operator ()(gimple *g) { return as_a <gassign *> (g); }
> 
> into a gimple-compat.h header which you include in places that
> are not converted "nicely".

Thanks for the suggestions.

Am I missing something, or is the gimple-compat.h idea above not valid C
++?

Note that "gimple" is still a typedef to
  gimple_statement_base *
(as noted before, the gimple -> gimple * change would break everyone
else's patches, so we talked about that as a followup patch for early
stage3).

Given that, if I try to create an "operator ()" outside of a class, I
get this error:

âgassign* operator()(gimple)â must be a nonstatic member function

which is emitted from cp/decl.c's grok_op_properties:
      /* An operator function must either be a non-static member function
	 or have at least one parameter of a class, a reference to a class,
	 an enumeration, or a reference to an enumeration.  13.4.0.6 */

I tried making it a member function of gimple_statement_base, but that
doesn't work either: we want a conversion
  from a gimple_statement_base * to a gassign *, not
  from a gimple_statement_base   to a gassign *.

Is there some syntactic trick here that I'm missing?  Sorry if I'm being
dumb (I can imagine there's a way of doing it by making "gimple" become
some kind of wrapped ptr class, but that way lies madness, surely).

> Both avoid manually making the compiler happy (which the
> explicit as_a<> stuff is!  It doesn't add any "checking" - it's
> just placing the as_a<> at the callers and thus make the
> runtine ICE fire there).
> 
> As much as I don't like "global" conversion operators I don't
> like adding overloads to all of the accessor functions even more.

(nods)

Some other options:

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.


Option 4: don't convert any accessors, but instead focus on fields of
structs (e.g. "call_stmt" within a cgraph_edge), and on params of other
functions (e.g. phi-manipulation code).  That way we'd avoid the
inconsistency of some accessors using GIMPLE_CHECK and some using
subclasses - all would continue to consistently use GIMPLE_CHECK, but
there would be some extra type-checking and self-documentation of the
expected statement kinds in the code.



FWIW, option 3 is my preferred approach (I've already done the bulk of
the work and it's already been reviewed; it would need an update merger
from trunk, and there's the big gimple to gimple * fixup you wanted).

> Whether you enable them generally or just for selected files
> via a gimple-compat.h will be up to you (but I'd rather get
> rid of them at some point).
> 
> Note this allows seamless transform of "random" functions
> taking a gimple now but really only expecting a single kind.
> 
> Note that we don't absolutely have to rush this all in for GCC 5.
> Being the very first for GCC 6 stage1 is another possibility.
> We just should get it right.

Thanks
Dave


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]