On Mon, 2013-11-04 at 08:19 -0500, Andrew MacLeod wrote:
> On 11/01/2013 06:58 PM, David Malcolm wrote:
> > On Fri, 2013-11-01 at 22:57 +0100, Jakub Jelinek wrote:
> >> On Fri, Nov 01, 2013 at 05:47:14PM -0400, Andrew MacLeod wrote:
> >>> On 11/01/2013 05:41 PM, Jakub Jelinek wrote:
> >>>> On Fri, Nov 01, 2013 at 05:36:34PM -0400, Andrew MacLeod wrote:
> >>>>>    static inline void
> >>>>> ! gimple_call_set_lhs (gimple gs, tree lhs)
> >>>>>    {
> >>>>> -   GIMPLE_CHECK (gs, GIMPLE_CALL);
> >> The checking you are removing here.
> >>
> >>> What checking?  There ought to be no checking at all in this
> >>> example...  gimple_build_call_vec returns a gimple_call, and
> >>> gimple_call_set_lhs()  doesn't have to check anything because it
> >>> only accepts gimple_call's.. so there is no checking other than the
> >>> usual "does my parameter match" that the compiler has to do...
> >> and want to replace it by checking of the types at compile time.
> >> The problem is that it uglifies the source too much, and, when you
> >> actually don't have a gimple_call but supposedly a base class of it,
> >> I expect you'd do as_a which is not only further uglification, but has
> >> runtime cost also for --enable-checking=release.
> > I can have a look next week at every call to gimple_call_set_lhs in the
> > tree, and see to what extent we know at compile-time that the initial
> > arg is indeed a call (of the ones I quickly grepped just now, most are
> > from gimple_build_call and friends, but one was from a gimple_copy).
> >
> > FWIW I did some performance testing of the is_a/as_a code in the earlier
> > version of the patch, and it didn't have a noticable runtime cost
> > compared to the GIMPLE_CHECK in the existing code:
> > Size of compiler executable:
> > http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01920.html
> > Compile times:
> > http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00171.html
> I actually really dislike as_a<> and is_a<>, and  think code needs to be 
> restructured rather than use them, other than possibly at the very 
> bottom level when we're allocating memory or something like that, or 
> some kind of emergency :-)...   If we require frequent uses of those, 
> I'd be against it, I find them quite ugly.
> 
> Like I said in the other reply, no rush, I don't think any of this 
> follow up is appropriate this late in stage 1.  It would be more of an 
> "interest" examination right now.. at least in my opinion...  I suspect 
> thinks like gimple_assign are more complex cases, but without looking 
> its hard to tell for sure.

I tried converting gimple_call_set_lhs to accept a gimple_call, rather
than a gimple, and excitingly, it was easiest to also convert
cgraph_edge's call_stmt to also be a gimple_call, rather than just a
gimple.

Am attaching a patch (on top of the patch series being discussed) which
adds this compile-time typesafety; bootstrap is in-progress.   IMHO very
little use of is-a.h was needed (5 instances of as_a, and 3 of dyn_cast;
no use of is_a).

I'm also attaching a followup patch which eliminates gimple_call_set_lhs
in favor of a method of gimple_statement_call.