On Tue, Nov 5, 2013 at 1:30 PM, David Malcolm <dmalc...@redhat.com> wrote:
> On Tue, 2013-11-05 at 12:47 +0100, Richard Biener wrote:
>> On Mon, Nov 4, 2013 at 10:43 PM, David Malcolm <dmalc...@redhat.com> wrote:
>> > 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.
>>
>> Does that work (using gimple_call * objects) for our garbage collector?
>> That is, does it know it is looking at a 'gimple'? It doesn't matter for
>> this
>> case as all stmts are reachable from struct function as sequence of 'gimple',
>> but in general?
> Yes (as of r204146, I believe).
>
> For example, the patch converts cgraph_edge's call_stmt to be a
> "gimple_call", rather than just a "gimple", but gengtype handles this by
> emitting a call to the base class visitor for call_stmt i.e.
> gimple_statement_base:
>
> void
> gt_ggc_mx_cgraph_edge (void *x_p)
> {
> struct cgraph_edge * x = (struct cgraph_edge *)x_p;
> [...snip chain_next/prev handling...]
> [...snip other fields...]
> gt_ggc_m_21gimple_statement_base ((*x).call_stmt);
> [..etc..]
> }
Ah, nice to know.
Thanks,
Richard.
>
