This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/6] Conversion of gimple types to C++ inheritance (v3)


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?

I have various changes that are mostly done that I'm trying to get
finished and into trunk for stage1.  These can be seen at:
https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/README.rst

If you want me to do more follow cleanups to gimple, I can do those.

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

I'd love to make use of compile-time type-safety like this, but does
this have to gate this initial step?

The transition to strongly-typed getters/setters could be done one
at-a-time, I guess.


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

(nods)

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

Nice.  It's readable (IMHO), and the type-checking (that it's a call) is
enforced at compile-time, rather than at run-time (and only in
ENABLE_CHECKING builds).

Sadly, this may need some further gengtype work: the simple inheritance
support I'm using in this patch series will barf if it sees methods
within a class.  So I can have a look at supporting methods in gengtype,
I guess.   Though the change above of gimple_call_set_lhs to accepting a
subclass ptr gives a middle-ground.

> 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 
By "instances" presumably you meant "subclasses"?

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

I think writing them out explicitly is better than doing them with
preprocessor magic: if we go down the route described above of wanting
to add methods, the various subclasses will eventually gain their own
methods, so we'll want code rather than building it all from .def files.

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

I'd be up for working on a followup patch that adds such subclasses, and
I'd be up for changing the accessors to give up compile-time
type-safety.  I'd do it one accessor at a time.

> > Again, as noted in the earlier patch series, the names of the structs
> > are rather verbose.  I would prefer to also rename them all to eliminate
> > the "_statement" component:
> >    "gimple_statement_base" -> "gimple_base"
> >    "gimple_statement_phi"  -> "gimple_phi"
> >    "gimple_statement_omp"  -> "gimple_omp"
> > etc, but I didn't do this to mimimize the patch size.  But if the core
> > maintainers are up for that, I can redo the patch series with that
> > change also, or do that as a followup.
> 
> As mentioned, I'd rather see the short names be typedefs for pointers to 
> the long names since we're typically using the pointers.

[FWIW, I'm not a fan of such typedefs, but I'll defer to you in this]

Presumably we'd want typedefs for the extra subclasses as well?

Also, I take it we'd use the "printable_name" from gimple.def - though
would we use it for the subclass name, or for the ptr typedef?

> Other than stomping on the same bits at the moment (less so once 
> gimple-stmt.[ch] is split out), these changes are completely orthogonal, 
> but in line with with my direction.  I think It should be done sooner or 
> later....  My preference is to also see the follow up work carried out 
> as well.  Or at least a plan for it.

So you like the approach, provided I commit to the followup work? [1]

OK, I'll try to create some followup patches next week.  I'd prefer to
get the conversion to inheritance into trunk sooner rather than later,
though.

Thanks for looking at this (and for your cleanup work!)

Dave
[1] and the patches still need formal review.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]