From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8437 invoked by alias); 5 Nov 2013 12:33:01 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 8428 invoked by uid 89); 5 Nov 2013 12:33:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.5 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,RDNS_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mail-wi0-f175.google.com Received: from Unknown (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Tue, 05 Nov 2013 12:32:59 +0000 Received: by mail-wi0-f175.google.com with SMTP id hm4so2007230wib.14 for ; Tue, 05 Nov 2013 04:32:50 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.180.185.179 with SMTP id fd19mr16316794wic.18.1383654770435; Tue, 05 Nov 2013 04:32:50 -0800 (PST) Received: by 10.195.12.114 with HTTP; Tue, 5 Nov 2013 04:32:50 -0800 (PST) In-Reply-To: <1383654609.5282.79.camel@surprise> References: <5271CBF9.2070005@redhat.com> <1383236801-13234-1-git-send-email-dmalcolm@redhat.com> <52741EE2.3030100@redhat.com> <20131101214148.GF27813@tucnak.zalov.cz> <52742162.2010009@redhat.com> <20131101215709.GG27813@tucnak.zalov.cz> <1383346704.5282.44.camel@surprise> <52779EF5.8000401@redhat.com> <1383601413.5282.62.camel@surprise> <1383654609.5282.79.camel@surprise> Date: Tue, 05 Nov 2013 12:52:00 -0000 Message-ID: Subject: Re: [PATCH 0/6] Conversion of gimple types to C++ inheritance (v3) From: Richard Biener To: David Malcolm Cc: Andrew MacLeod , Jakub Jelinek , GCC Patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00375.txt.bz2 On Tue, Nov 5, 2013 at 1:30 PM, David Malcolm wrote: > On Tue, 2013-11-05 at 12:47 +0100, Richard Biener wrote: >> On Mon, Nov 4, 2013 at 10:43 PM, David Malcolm 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. >