On Fri, 2013-11-01 at 17:36 -0400, Andrew MacLeod wrote:
> On 10/31/2013 12:26 PM, David Malcolm wrote:
> > [Shamelessly hijacking Andrew's thread about gimple.h refactoring,
> > since this seems on-topic for that, and I'm keen to hear from Andrew on
> > how the following would interact with his work - I *think* our two
> > cleanups are orthogonal.
> 
> Mostly orthogonal anyway... just stomping on the same bits :-).
> 
> Since you hijacked a planning thread, do you plan to take this any 
> further, or make this change and move on to something else?
> 
> It is a start, but it doesnt do the rest of the work that needs doing to 
> really take advantage of it... which will be extensive.
> for instance, we should change:
> 
>    static inline void
> ! gimple_call_set_lhs (gimple gs, tree lhs)
>    {
> -   GIMPLE_CHECK (gs, GIMPLE_CALL);
>      gimple_set_op (gs, 0, lhs);
> to
>      static inline void
> ! gimple_call_set_lhs (gimple_statement_call *gs, tree lhs)
>    {
>      gimple_set_op (gs, 0, lhs);
> 
> 
> but then every location that calls it needs an appropriate change:
> 
> !       gimple call;
> !       call = gimple_build_call_vec (build_fold_addr_expr_loc (0, 
> alias), vargs);
>          gimple_call_set_lhs (call, atree);
> 
> --- 1518,1524 ----
> 
> !       gimple_statement_call *call;
> !       call = as_a<gimple_statement_call> (gimple_build_call_vec 
> (build_fold_addr_expr_loc (0, alias), vargs));
>          gimple_call_set_lhs (call, atree);
> 
> And in fact there is a ripple effect to then change 
> gimple_build_call_vec to simply return a gimple_statement_call *... Then 
> this doesn't look as ugly either...
> 
> !       gimple_statement_call *call;
> !       call = gimple_build_call_vec (build_fold_addr_expr_loc (0, 
> alias), vargs);
>          gimple_call_set_lhs (call, atree);
> 
> that is looking much better :-)
> 
> 
> Leaving the names as they are should be ok, but the I'd also add a 
> typedef for the pointer without the 'statement' in the name.. ie
>      typedef gimple_statement_call *gimple_call;
> That seems in line with what we do with 'gimple' right now and the short 
> form is the type we'd normally use.
> 
> That adds a touch of difficulty with "as_a", since that requires the 
> type name, not the shorthand pointer.... so you have something like
> gimple_call call = as_a <gimple_statement_call> blah().
> I think as the changes to use the gimple_call type are pushed through 
> all the callers and callee's, the requirement of as_a and the long name 
> being ugly begins to rapidly disappear...  it'll only exist in the core 
> creation routines and no one will usually see it.   So I don't *think* 
> this is an issue...  but it is an ugly transition if its only partially 
> done.
> 
> And eventually we can pull the accessor routines and others into the 
> class itself:
>         gimple_call call;
>         call = gimple_build_call_vec (build_fold_addr_expr_loc (0, 
> alias), vargs);
>         call->set_lhs (atree);
> 
> Which results in a similar feel to the new gimple_type, gimple_decl, 
> etc. classes with my upcoming tree wrappers.  Changing gimple statements 
> to behave something like this is actually mentioned in the plan, but out 
> in the future once trees have been taken care of.
> 
> I would also plan to create instances for each of the gimple statements 
> that don't have them now, like gimple_statement_assign. Its lumped in as 
> a general GSS_WITH_MEM_OPS, so there is no gimple_statement_assign 
> class/struct to use.
> 
> It would really be nice to use the DEFGSCODE macro and gimple.def to 
> make this happen automagically somehow... Its tantalizingly close now I 
> think, especially combined with the info in gsstruct.def... Although if 
> we are moving to proper classes eventually its probably better to 
> explicitly write the required class for a statement kind.
> 
> That all said, this change enables that work to proceed if someone wants 
> to do it.
> 
> My question is: Is anyone going to do it, and if so,  who and when? :-)

Here's a followup patch which ensures that every gimple code has its own
subclass, by adding empty subclasses derived from the GSS_-based
subclasses as appropriate (I don't bother for gimple codes that already
have their own subclass due to having their own GSS layout).  I also
copied the comments from gimple.def into gimple.h, so that Doxygen picks
up on the descriptions and uses them to describe each subclass.

Posting for discussion (i.e. am still bootstrapping this; it also needs
the gengtype fix posted as
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00425.html)

You can see what the resulting gimple class hierarchy (as reported by
Doxygen) looks like here:
http://dmalcolm.fedorapeople.org/gcc/2013-11-05/doxygen/html/structgimple__statement__base.html