From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18697 invoked by alias); 26 Oct 2012 16:54:29 -0000 Received: (qmail 18681 invoked by uid 22791); 26 Oct 2012 16:54:27 -0000 X-SWARE-Spam-Status: No, hits=-4.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_BIG_TO_CC,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-oa0-f47.google.com (HELO mail-oa0-f47.google.com) (209.85.219.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Oct 2012 16:54:20 +0000 Received: by mail-oa0-f47.google.com with SMTP id h1so3039479oag.20 for ; Fri, 26 Oct 2012 09:54:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-system-of-record:x-gm-message-state; bh=lzHxz+lQBxta9vz6f7qXWxPcneBooBinzLvvy/Ul/as=; b=ZgBuA75G6BU6TUdDW7JPKLReiOL+qoXv3ttZOKlfYa4YDkpBVI3dFgjN8wOvmIB14R hmzu90JHUEJkw0WXBM/T2KbK02LUPvsJjI/Dsokw5mqkY1RUqBmBK66BB3geGNooD/eb NrKvLxgfwBR3lZ4igonc9Dg+/jOXhCdQ7G1n2NyYUXudCLv7Nc3ZipAwb1bzjgC2PArA xjhp026TtTLsJ9hR+ceQEjnQIAPp+nTUSSFzzGxmeWA2CYyjOq9kJVWLHQocLP9BHm6C r+ekLkZkri/CRO/haM23LF9I42aUfy9a1w+StSeMW682wCh+Z64n5GfVWIoUnOPE5/sP kWlQ== MIME-Version: 1.0 Received: by 10.182.31.13 with SMTP id w13mr19135849obh.29.1351270459465; Fri, 26 Oct 2012 09:54:19 -0700 (PDT) Received: by 10.182.176.106 with HTTP; Fri, 26 Oct 2012 09:54:19 -0700 (PDT) In-Reply-To: References: Date: Fri, 26 Oct 2012 17:28:00 -0000 Message-ID: Subject: Re: User directed Function Multiversioning via Function Overloading (issue5752064) From: Sriraman Tallam To: Xinliang David Li Cc: Jan Hubicka , Diego Novillo , Jason Merrill , Jan Hubicka , Mark Mitchell , Nathan Sidwell , "H.J. Lu" , Richard Guenther , Uros Bizjak , reply@codereview.appspotmail.com, GCC Patches Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true X-Gm-Message-State: ALoCoQn+/bo2mUNPA1xfJLkO5W0oeaq0t2Cnhc/S+dxS1E6vP5EiLU7+ufV9yqoojd8PnIT/7cupNyYMLZcqJFgFeMg08WcPOxmTMb31osOjKjHuJ+MXkdH9NxGO1/nA5cS4CJkMmaNq/rFjbRr+eRalwc5LyYTAs4DCyGn29AlWsOAtSJp/B001YhRLT+avYJbhShAOJ2N7 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: 2012-10/txt/msg02437.txt.bz2 On Fri, Oct 26, 2012 at 9:07 AM, Xinliang David Li wrote: > On Fri, Oct 26, 2012 at 8:54 AM, Jan Hubicka wrote: >> Hi, >> sorry for jumping in late, for too long I did not had chnce to look at my TODO. >> I have two comments... >>> Index: gcc/cgraphbuild.c >>> =================================================================== >>> --- gcc/cgraphbuild.c (revision 192623) >>> +++ gcc/cgraphbuild.c (working copy) >>> @@ -34,6 +34,7 @@ along with GCC; see the file COPYING3. If not see >>> #include "ipa-utils.h" >>> #include "except.h" >>> #include "ipa-inline.h" >>> +#include "target.h" >>> >>> /* Context of record_reference. */ >>> struct record_reference_ctx >>> @@ -317,8 +318,23 @@ build_cgraph_edges (void) >>> bb); >>> decl = gimple_call_fndecl (stmt); >>> if (decl) >>> - cgraph_create_edge (node, cgraph_get_create_node (decl), >>> - stmt, bb->count, freq); >>> + { >>> + struct cgraph_node *callee = cgraph_get_create_node (decl); >>> + /* If a call to a multiversioned function dispatcher is >>> + found, generate the body to dispatch the right function >>> + at run-time. */ >>> + if (callee->dispatcher_function) >>> + { >>> + tree resolver_decl; >>> + gcc_assert (callee->function_version >>> + && callee->function_version->next); >>> + gcc_assert (targetm.generate_version_dispatcher_body); >>> + resolver_decl >>> + = targetm.generate_version_dispatcher_body (callee); >>> + gcc_assert (resolver_decl != NULL_TREE); >>> + } >>> + cgraph_create_edge (node, callee, stmt, bb->count, freq); >>> + } >> I do not really think resolver generation belongs here + I would preffer >> build_cgraph_edges to really just build the edges. >>> Index: gcc/cgraph.c >>> =================================================================== >>> --- gcc/cgraph.c (revision 192623) >>> +++ gcc/cgraph.c (working copy) >>> @@ -1277,6 +1277,16 @@ cgraph_mark_address_taken_node (struct cgraph_node >>> node->symbol.address_taken = 1; >>> node = cgraph_function_or_thunk_node (node, NULL); >>> node->symbol.address_taken = 1; >>> + /* If the address of a multiversioned function dispatcher is taken, >>> + generate the body to dispatch the right function at run-time. This >>> + is needed as the address can be used to do an indirect call. */ >>> + if (node->dispatcher_function) >>> + { >>> + gcc_assert (node->function_version >>> + && node->function_version->next); >>> + gcc_assert (targetm.generate_version_dispatcher_body); >>> + targetm.generate_version_dispatcher_body (node); >>> + } >> >> Similarly here. I also think this way you will miss aliases of the multiversioned >> functions. > >> >> I am not sure why the multiversioning is tied with the cgraph build and the >> datastructure is put into cgraph_node itself. It seems to me that your >> dispatchers are in a way related to thunks - i.e. they are inserted into >> callgraph and once they become reachable their body needs to be produced. I >> think generate_version_dispatcher_body should thus probably be done from >> cgraph_analyze_function. (to make the function to be seen by analyze_function >> you will need to make it to be finalized at the time you set >> dispatcher_function flag. > > This seems reasonable -- Sri, do you see any problems with this suggestion? No, I will make this change asap. > >> >> I would also put the dispatcher datastructure into on-side hash by node->uid. >> (i.e. these are rare and thus the datastructure should be small) >> symbol table is critical for WPA stage memory use and I plan to remove as much >> as possible from the nodes in near future. For this reason I would preffer >> to not add too much of stuff that is not going to be used by majority of nodes. >> OK, will change as suggested. > > I had the concern on the increasing the size of core data structure too. Thanks, -Sri. > > thanks, > > David > >> Honza