On Fri, Apr 25, 2014 at 5:28 PM, David Malcolm <dmalc...@redhat.com> wrote: > On Fri, 2014-04-25 at 10:37 +0200, Richard Biener wrote: >> On Thu, Apr 24, 2014 at 4:59 PM, David Malcolm <dmalc...@redhat.com> wrote: >> > On Thu, 2014-04-24 at 09:09 -0400, Andrew MacLeod wrote: >> >> On 04/24/2014 04:33 AM, Richard Biener wrote: >> >> > On Wed, Apr 23, 2014 at 11:23 PM, Jeff Law <l...@redhat.com> wrote: >> >> >> On 04/23/14 15:13, David Malcolm wrote: >> >> >>> On Wed, 2014-04-23 at 15:04 -0600, Jeff Law wrote: >> >> >>>> On 04/21/14 10:56, David Malcolm wrote: >> >> >>>>> This updates all of the gimple_bind_* accessors in gimple.h from >> >> >>>>> taking >> >> >>>>> a >> >> >>>>> plain gimple to taking a gimple_bind (or const_gimple_bind), with >> >> >>>>> the >> >> >>>>> checking happening at the point of cast. >> >> >>>>> >> >> >>>>> Various other types are strengthened from gimple to gimple_bind, and >> >> >>>>> from >> >> >>>>> plain vec<gimple> to vec<gimple_bind>. >> >> >>> >> >> >>> [...] >> >> >>> >> >> >>>> This is fine, with the same requested changes as #2; specifically >> >> >>>> using >> >> >>>> an explicit cast rather than hiding the conversion in a method. Once >> >> >>>> those changes are in place, it's good for 4.9.1. >> >> >>> Thanks - presumably you mean >> >> >>> "good for *trunk* after 4.9.1 is released" >> >> >> Right. Sorry for the confusion. >> >> > Note I still want that less-typedefs (esp. the const_*) variants to be >> >> > explored. >> >> > Changing this will touch all the code again, so I'd like to avoid that. >> >> > >> >> > That is, shouldn't we go back to 'gimple' being 'gimple_statement_base' >> >> > and not 'gimple_statement_base *'? The main reason that we have so >> >> > many typedefs is that in C you had to use 'struct foo' each time you >> >> > refer to foo as a type - I suppose it was then convenient to do the >> >> > typedef to the pointer type. With 'gimple' being not a pointer type >> >> > we get const correctness in the way people would expect it to work. >> >> > [no, I don't suggest you change 'tree' or 'const_tree' at this point, >> >> > just >> >> > gimple (and maybe gimple_seq) as you are working on the 'gimple' >> >> > type anyway]. >> >> > >> >> > >> >> >> >> So if we change 'gimple' everywhere to be 'gimple *', can we just >> >> abandon the 'gimple' typedef completely and go directly to using >> >> something like gimple_stmt, or some other agreeable name instead? >> >> >> >> I think its more descriptive and then frees up the generic 'gimple' name >> >> should we decide to do something more with namespaces in the future... >> > >> > There have been a few different proposals as to what the resulting >> > gimple API might look like, in various subthreads of this discusssion, >> > so I thought it might help the discussion to gather up the proposals, >> > and to apply them to some specific code examples, to see what the >> > results might look like. >> > >> > So here are a couple of code fragments, from gcc/graphite-sese-to-poly.c >> > and gcc/tree-ssa-uninit.c respectively: >> > >> > Status quo >> > ========== >> > >> > static gimple >> > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > vec<gimple> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > gimple def, loop_phi, phi, close_phi = stmt; >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > gimple_stmt_iterator gsi; >> > vec<gimple> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > The currently-posted patch series >> > ================================= >> > Here's the cumulative effect of the patch series I posted, using the >> > casting methods of the base class (the "stmt->as_a_gimple_phi" call): >> > >> > -static gimple >> > +static gimple_phi >> > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > vec<gimple> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > + gimple def; >> > + gimple_phi loop_phi, phi, close_phi = stmt->as_a_gimple_phi (); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > Direct use of is-a.h, retaining typedefs of pointers >> > ==================================================== >> > The following patch shows what the above might look like using the patch >> > series as posted, but eliminating the casting methods in favor of >> > direct use of the is-a.h API (but still using lots of typedefs of >> > pointers): >> > >> > -static gimple >> > +static gimple_phi >> > detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > vec<gimple> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple def; >> > + gimple_phi loop_phi, phi, close_phi = as_a <gimple_phi> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > I posted an example of porting a patch in the series to this approach as: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01549.html >> > >> > Explicit pointers, rather than typedefs >> > ======================================= >> > Richi suggested making pointers be explicit rather than hidden in the >> > typedefs in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html >> > which might give something like this: >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static gimple_phi * >> > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> >> > *in, >> > + vec<gimple *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple *def; >> > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> >> > (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > Changing the meaning of "gimple" makes this a much bigger patch IMHO >> > than what I've currently posted. One way to achieve this could be a >> > mega-patch (ugh) which ports the whole middle-end at once to change the >> > "pointerness" of "gimple", probably auto-generated and, once that's in >> > place, then look at introducing subclass usage. >> > >> > Note: it's more fiddly than a simply sed of "gimple" to "gimple *" (or >> > whatever); consider the gimple_phi decls above, which would change >> > thusly: >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > + gimple *def, *loop_phi, *phi, *close_phi = stmt; >> > >> > Implicit naming >> > =============== >> > Several people have suggested that the "gimple_" prefix is redundant. >> > >> > Andrew MacLeod suggested in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html >> > that we could simple drop the "gimple_" prefix. Combining this with the >> > pointer approach, for example, gives: >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static phi * >> > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple *> >> > *in, >> > + vec<gimple *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple *def; >> > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + phi_iterator gsi; >> > + vec<phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > though it could also be done with typedefs of pointers, rather than the >> > "explicit pointers" above. >> > >> > >> > Namespaces (explicit) >> > ===================== >> > Continuing with the idea that the "gimple_" prefix is redundant, Andrew >> > MacLeod also suggested in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html >> > that we could repurpose "gimple" to be a namespace. Here's what things >> > might look like written out in full (perhaps using gimple::stmt to be >> > the base class): >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static gimple::phi * >> > +detect_commutative_reduction (scop_p scop, gimple::stmt *stmt, >> > + vec<gimple::stmt *> *in, >> > + vec<gimple::stmt *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple::stmt *def; >> > + gimple::phi loop_phi, phi, close_phi = as_a <gimple::phi *> >> > (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple::phi_iterator gsi; >> > + vec<gimple::phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > This may require some gengtype support, for the case of fields within >> > structs. >> > >> > Andrew suggested renaming "gimple" to "gimple_stmt" in: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01297.html >> > as a possible migration path towards this. >> > >> > Namespaces (implicit) >> > ===================== >> > The above is, of course, verbose - I'm mostly posting it to clarify the >> > following, which uses a "using" decl to eliminate all of the "gimple::" >> > from the above: >> > >> > using gimple; >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static phi * >> > +detect_commutative_reduction (scop_p scop, stmt *stmt, >> > + vec<stmt *> *in, >> > + vec<stmt *> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + stmt *def; >> > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + phi_iterator gsi; >> > + vec<phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > This would require some gengtype support (again, for the case of fields >> > within structs). >> > >> > C++ references (without namespaces) >> > =================================== >> > Richi suggested the use of references rather than pointers when the >> > address is required to be non-NULL: >> > http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01427.html >> > though there's been some pushback on C++ references in the past e.g. >> > from Jeff: >> > http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01430.html >> > >> > Here's what the "Explicit pointers, rather than typedefs" might look >> > like, but with references rather than ptrs for some vars where NULL >> > isn't valid: >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static gimple_phi * >> > +detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple &> >> > *in, >> > + vec<gimple &> *out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > + gimple *def; >> > + gimple_phi *loop_phi, *phi, *close_phi = as_a <gimple_phi *> >> > (stmt); >> > tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + gimple_phi_iterator gsi; >> > + vec<gimple_phi &> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > ...though arguably there's plenty more conversion of the above that >> > could be done: "def" is initialized once, with a non-NULL ptr, so >> > arguably could be reference, but that would require moving the decl down >> > to the point of initialization so I didn't touch it above. I think use >> > of references would tend to break up such declarations of locals. >> > >> > C++ references (with implicit namespaces) >> > ========================================= >> > ...and here's what the above "namespaces with a using decl" approach >> > might look like with references: >> > >> > using gimple; >> > >> > -static gimple >> > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, >> > - vec<gimple> *out) >> > +static phi * >> > +detect_commutative_reduction (scop_p scop, stmt &stmt, >> > + vec<stmt &> &in, >> > + vec<stmt &> &out) >> > { >> > if (scalar_close_phi_node_p (stmt)) >> > { >> > - gimple def, loop_phi, phi, close_phi = stmt; >> > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); >> > - tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); >> > + stmt *def; >> > + phi *loop_phi, phi, close_phi = as_a <phi *> (stmt); >> > + tree init, lhs, arg = gimple_phi_arg_def (*close_phi, 0); >> > >> > if (TREE_CODE (arg) != SSA_NAME) >> > >> > /* ...etc... */ >> > >> > static unsigned int >> > execute_late_warn_uninitialized (void) >> > { >> > basic_block bb; >> > - gimple_stmt_iterator gsi; >> > - vec<gimple> worklist = vNULL; >> > + phi_iterator gsi; >> > + vec<phi *> worklist = vNULL; >> > pointer_set_t *added_to_worklist; >> > >> > >> > So the above hopefully gives an idea of what a more compile-time >> > type-safe gimple API might look like; sorry if I've mischaracterized any >> > of the ideas. I believe they're roughly sorted by increasing >> > invasiveness, and by increasing "C++ ness" (both of which give me >> > pause). >> > >> > Thoughts? >> >> The 'should gimple be a pointer typedef' issue is rather orthogonal >> and it merely affects how we do const-correctness >> (but it affects your ongoing work, thus I brought it up - also because >> you address the const correctness issue). >> >> It's convenient to do such change first IMHO. And I never liked the >> const_ typedef variants that were introduced. The main reason for >> them was probably to avoid all the churn of replacing the tree pointer >> typedef with a tree (non-pointer) typedef. The mistake was to >> repeat that for 'gimple' ... >> >> I have no strong opinion on const correctness in general, but I do >> have a strong opinion against introducing more of the const_* >> typedefs. Those simply feel completely bogus (and alienate the new >> GCC developers we want to attract with C++ and all these changes (uh? >> heh!?)). >> >> So if you turn gimple up-side-down (which you do with more static >> type checking) then please fix const correctness first. > > OK. I've started looking at this, and immediately ran into an annoying > gengtype limitation; it can't yet cope with anything beyond: > > vec<ID1, ID2, ..., IDN> > > and so complains with things like: > > vec<gimple *> > vec<const gimple *> > > so I'll have a look at fixing that, since that would otherwise block the > more concrete things like: > > vec<const gimple_phi *> > > that motivate this upheaval. > >> As of namespaces - yes, ideally we'd move the various prefixes >> to namespaces (that a gimple pass can simply use for example). >> But that's again an orthogonal issue. You could always add the >> namespace with your work and add typedefs like >> >> typedef gimple::phi gimple_phi; >> >> (though that invites inconsistent coding-style, using gimple::phi >> vs. gimple_phi throughout the code) > > OK. Given your preference for doing this without typedefs, I'm working > on an automated way to convert "gimple" / "const_gimple" to... > something, making it (I hope) relatively easy to choose whether that > something should be: > gimple * const gimple * > or > gimple_stmt * const gimple_stmt * > or > gimple::stmt * const gimple::stmt * > or > stmt * const stmt * > with the last one maybe using C++ namespaces with a "using gimple" decl > (and thus actually a "gimple::stmt", or maybe just being a plain class. > > One much more invasive possible change I didn't mention in the > "Examples" email would be to convert the gimple accessors to be actual > methods, giving something like this (in this case built on top of the > renaming, with implicit namespaces idea):
Yeah ... the usual argument against this is that it makes grepping harder (unless you make it phi->phi_arg_def () ...). Not the very strongest argument I'd say, but I'd like to defer this to a separate discussion ... ;) We'd still have the mix of tree and GIMPLE objects everywhere so mixing both styles will make our code-base very ugly (unless you volunteer to apply the same transforms to 'tree' ...). So, please not at this point in time ;) Thanks, Richard. > using gimple; > > -static gimple > -detect_commutative_reduction (scop_p scop, gimple stmt, vec<gimple> *in, > - vec<gimple> *out) > +static phi * > +detect_commutative_reduction (scop_p scop, stmt *stmt, > + vec<stmt *> *in, > + vec<stmt *> *out) > { > if (scalar_close_phi_node_p (stmt)) > { > - gimple def, loop_phi, phi, close_phi = stmt; > - tree init, lhs, arg = gimple_phi_arg_def (close_phi, 0); > + stmt *def; > + phi *loop_phi, *phi, *close_phi = as_a <phi *> (stmt); > + tree init, lhs, arg = close_phi->arg_def (0); > > if (TREE_CODE (arg) != SSA_NAME) > > /* ...etc... */ > > static unsigned int > execute_late_warn_uninitialized (void) > { > basic_block bb; > - gimple_stmt_iterator gsi; > - vec<gimple> worklist = vNULL; > + phi_iterator gsi; > + vec<phi *> worklist = vNULL; > pointer_set_t *added_to_worklist; > > (i.e. note how the call to gimple_phi_arg_def has become a method call). > > I assumed that such a change would be simply too much upheaval (and > would impact greppability), but since you seem to be advocating for a > "if we're going to do it, do it properly" position, I was wondering your > thoughts on that kind of change? (the patches would be *much* bigger, > of course, since it means changing every callsite rather than just every > decl). Again, this may be simply orthogonal to the original goal of > expressing the gimple codes in a way that can be checked at > compile-time. I'm not especially keen on such a further transition - > more work and churn [I can envisage a transition path in which the > accessor functions become calls to methods so that no callsites need > changing, and then the accessor functions gradually get removed, porting > their callsites to use methods]. > > Dave > >> Richard. >> >> > FWIW, my own preference is for "Direct use of is-a.h, retaining typedefs >> > of pointers" but that may be because it would be less work for me ;) >> > >> > Andrew: I know you've been working on improving the typesafety of >> > expressions vs types in the middle-end. I'm curious as to what the >> > above code fragments look like with just your changes? >> > >> > Hope this is useful. >> > Dave >> > >> > > >