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