Re: Examples of gimple statement API (was Re: [PATCH 03/89] Introduce gimple_bind and use it for accessors.)

[Richard Biener <richard dot guenther at gmail dot com>]

On Thu, Apr 24, 2014 at 4:59 PM, David Malcolm <dmalcolm@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 <law@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.

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)

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