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):
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
> >
> >
