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

[David Malcolm <dmalcolm at redhat dot com>]

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?

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