From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12413 invoked by alias); 26 Oct 2012 16:07:13 -0000 Received: (qmail 12392 invoked by uid 22791); 26 Oct 2012 16:07:11 -0000 X-SWARE-Spam-Status: No, hits=-2.6 required=5.0 tests=AWL,BAYES_50,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_BIG_TO_CC,KHOP_RCVD_TRUST,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-la0-f47.google.com (HELO mail-la0-f47.google.com) (209.85.215.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Oct 2012 16:07:06 +0000 Received: by mail-la0-f47.google.com with SMTP id h5so2635299lam.20 for ; Fri, 26 Oct 2012 09:07:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:date:message-id:subject:from:to:cc:content-type :x-system-of-record:x-gm-message-state; bh=ZI6kQFwFQjCcKyKE3T5Yjv0py8xlD3pGc7lKHcEH710=; b=pvTQ/GfkGJgM5uXrixirwTMCL8AeQAfJIEaLwqDxPygYd1ne6QhH8fC+qlNo6uNQlZ sDlnIk9n/vM37htMKBBs5HlydDIevX4QOax8xFXTxr1zNfNdlAuE1PE+m04CAuPXQZ1A evYbxnrNOkERD6Y/q9RrnQhSfsDN/1fBU/zsULdfdFuf72OYyxLW0e+lKuSszQvzbUEa qcfIl6drABkUZHf93IXYlTHIK9aFaHnYcA8Jcf2zTchz8H71CYIFFtS3yHZJOy97ezSa fw6k7aI7948oJOkbi4TiQARLa6Knn0KOSWCPEqJJYtimcPr2BgSKKhREpaivcWJcbyxE qhjA== MIME-Version: 1.0 Received: by 10.112.28.137 with SMTP id b9mr6795196lbh.60.1351267624547; Fri, 26 Oct 2012 09:07:04 -0700 (PDT) Received: by 10.152.23.1 with HTTP; Fri, 26 Oct 2012 09:07:04 -0700 (PDT) Date: Fri, 26 Oct 2012 16:54:00 -0000 Message-ID: Subject: Re: User directed Function Multiversioning via Function Overloading (issue5752064) From: Xinliang David Li To: Jan Hubicka Cc: Sriraman Tallam , 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: ALoCoQlxKb4P+KenUmQ62VTdFB4B1C+qnEmwncwOBChIj4qS11xmxJkEt4rTqpZcJnK4v8c3s091mG+E5wd3gOZEWkXWcDIC3YZZNQolK6yn0HPqixpMu/v6EG3KSHZuauchwV2OSXtNwOpz7gEfgkrJfqspAXp3bCZcueprdezLeWZC4LBa1X6Zd3vG5bsAoP2GBQ+RVvN6 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/msg02435.txt.bz2 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? > > 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. > I had the concern on the increasing the size of core data structure too. thanks, David > Honza