From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31157 invoked by alias); 14 Jul 2011 13:46:42 -0000 Received: (qmail 31148 invoked by uid 22791); 14 Jul 2011 13:46:41 -0000 X-SWARE-Spam-Status: No, hits=-3.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 14 Jul 2011 13:46:19 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 200BB8F0D1; Thu, 14 Jul 2011 15:46:18 +0200 (CEST) Date: Thu, 14 Jul 2011 14:15:00 -0000 From: Martin Jambor To: Jan Hubicka Cc: GCC Patches Subject: Re: [PATCH] New IPA-CP with real function cloning Message-ID: <20110714134617.GA1349@virgil.arch.suse.de> Mail-Followup-To: Jan Hubicka , GCC Patches References: <20110623192404.GC2736@virgil.arch.suse.de> <20110707160307.GG26368@atrey.karlin.mff.cuni.cz> <20110708165959.GB19992@virgil.arch.suse.de> <20110708182430.GB21021@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20110708182430.GB21021@kam.mff.cuni.cz> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 X-SW-Source: 2011-07/txt/msg01151.txt.bz2 Hi, I'll send a new version of IPA-CP incorporating most of the feedback in a new thread but let me also comment on some of the points here: On Fri, Jul 08, 2011 at 08:24:31PM +0200, Jan Hubicka wrote: > > > > { > > > > /* Pointer to an array of structures describing individual formal > > > > parameters. */ > > > > struct ipcp_lattice *lattices; > > > > > > Hmm, how we get here around the need to mark this GTY(). I.e are we sure that all the known_vals > > > must be referneced from elsewhere at ggc time? > > > > (Scalar) constants that are results of arithmetic jump functions may > > not be referenced from elsewhere, everything else is referenced from > > the jump functions. If it is a problem it is already present in the > > current IPA-CP. ipa_node_params and lattices are not GTYed there > > either. > > Hmm, I guess it is not really problem only because the lattices are used > only in ipa-cp so the values do not really live across GGC call. Well, technically they survive until after inlining (because of indirect inlining which also derives information from the lattices corresponding to node->inlined_to node. Results of arithmetic functions are not going to be accessed during inlining when compiling any reasonable program but... > > > > > > > > /* ipa_edge_args stores information related to a callsite and particularly its > > > > arguments. It can be accessed by the IPA_EDGE_REF macro. */ > > > > typedef struct GTY(()) ipa_edge_args > > > > > > probably edge_summary would be my preferred name. > > > > Ugh, this is the current name, we may change it later. In any event > > the name should probably tell that the summary is about parameters. > > Hmm, OK, it is not bad name after all. :-) > > > > > > > || !inline_summary (node)->versionable > > > > || TYPE_ATTRIBUTES (TREE_TYPE (node->decl)) > > > > > > This is getting an issue for Fortran that attach the function arguments quite commonly, > > > perhaps we should start moving ahead on this and ruling out only the arguments that > > > gets can't be preserved. > > > > Yes, for example in 433.milc basically all the functions are > > considered not versionable because of this. I also thought of not > > removing parameters for such functions. > > > > > Also you need to test attributes of DECL itself. > > > > Really? We are not doing it now, nether for IPA-CP nor for IPA-SRA > > (which is good at triggering problems with these). > > Hmm, all the ipa-inline code checks DECL_ATTRIBUTES only. I believe the type > attributes ends up being copied to decl attributes but not vice versa. > I guess the testcase should be > > __attribute__ ((really_bad_attribute)) > function (int param) > { > use param > } > > and then let ipa-cp to invent param is a constant. what would be such a "really_bad_attribute" ? > > > > > > > > I think this all should be part of can_change_signature_p code, since we can version > > > with all attributes I can thunk of when we don't change signature. > > > > > > > || cgraph_function_body_availability (node) <= AVAIL_OVERWRITABLE) > > > > res = false; > > > > else > > > > /* Removing arguments doesn't work if the function takes varargs > > > > or use __builtin_apply_args. > > > > FIXME: handle this together with can_change_signature flag. */ > > > > for (edge = node->callees; edge; edge = edge->next_callee) > > > > { > > > > tree t = edge->callee->decl; > > > > if (DECL_BUILT_IN_CLASS (t) == BUILT_IN_NORMAL > > > > && (DECL_FUNCTION_CODE (t) == BUILT_IN_APPLY_ARGS > > > > || DECL_FUNCTION_CODE (t) == BUILT_IN_VA_START)) > > > > > > can_change_sigunature will also handle apply_args. > > > Add VA_START code there, too. For the other use of this flag (in i386) VA_START The last one already is VA_START... or do you mean a different one? > > > rules out the change, too, but so far we explicitely tested that in the backend. > > > > > > Iguess with these changes, inline_summary (node)->versionable should coincide > > > with IPA_NODE_REF (node)->node_versionable making this whole code unnecesary > > > (it was supposed to work this way, I am not sure why we now have to versionable > > > flags). > > > > That would be nice. I was wondering why the difference between the > > two. Again, I am yet to decide whether this should be done as a > > followup or within this change. > > OK. BTW, it will be a followup change. > > > If you use for_node_thunk_and_aliases you can remove the recursion you do above > > > and it will work for aliases of thunk correctly, too ;) > > > > But I wouldn't be able to check the edge for hotness. > > BTW currently the edges from thunks miss any profile info. > (i.e. it will be 0). I guess we ought to make ipa-profile pass to estimate those > (it is difficult ot measure count of an alias). > I'm not really looking at the edges from thunks to the actual function. OTOH, I assume that edges to a thunk do have a count and look at that. > If you walk only hot edges, then you need to make your function descent into > both alias refs and thunks calls, or the aliases of thunks will not be seen > then. Well, looking at bits of the patch again now, aliases to thunks might indeed be a problem for a few pieces of it. I'll send the patch nevertheless and ponder about this problem later. Thanks, Martin