From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12078 invoked by alias); 6 Aug 2014 21:42:25 -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 12062 invoked by uid 89); 6 Aug 2014 21:42:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f44.google.com Received: from mail-qa0-f44.google.com (HELO mail-qa0-f44.google.com) (209.85.216.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 06 Aug 2014 21:42:22 +0000 Received: by mail-qa0-f44.google.com with SMTP id f12so3154695qad.31 for ; Wed, 06 Aug 2014 14:42:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=NMmqX+IE0vHMl94Vip8jOyzz7ZbH0t5r7H3w9c67bsw=; b=VyLfgd+U3BixP3i497eVQ2oHxCq0SpjgMYVkOvOtoa541lsPFGKP+P+82fpuS+2tLq lIdRirRF6OmotistoEj4XMT/vCDFE6fGSVqnpu8I+mFZoWrvMAln7A7UZFlIm81Ghuo8 T2QFO3hPgOaDKNmMz7IINO0JFP0x9E1BF8EmiUu35mTbaiaHFZ2D3YCRDfDlLMuzooX3 93mjepeMGMhimmUFVCMJfNbmAwtOnrvs/M6QDu6VMw8fk4bU/RXcT1cgHm1/p28FzI/c Xe/ilsEltxhXz9lc4UkJpjYFNqQcoyDiXbNtOxuOMqnL/5s4K4x/LIgNE6/Bv9fGZvJX cMlg== X-Gm-Message-State: ALoCoQnDCcyh7Opmey4P9WErH15kH1XtyNNWyXswArGSxVjRlZkHOm7mM6df1W2xIbXNsw26njfa MIME-Version: 1.0 X-Received: by 10.229.84.133 with SMTP id j5mr21096362qcl.14.1407361340248; Wed, 06 Aug 2014 14:42:20 -0700 (PDT) Received: by 10.229.210.2 with HTTP; Wed, 6 Aug 2014 14:42:20 -0700 (PDT) In-Reply-To: References: <20140707184843.GB12716@kam.mff.cuni.cz> Date: Wed, 06 Aug 2014 21:42:00 -0000 Message-ID: Subject: Re: [PATCH] C++ thunk section names From: Sriraman Tallam To: Jan Hubicka Cc: Xinliang David Li , GCC Patches , Teresa Johnson Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00748.txt.bz2 Hi, Just wondering if you got a chance to look at this? Sri On Tue, Jul 8, 2014 at 10:45 AM, Sriraman Tallam wrote: > On Tue, Jul 8, 2014 at 10:38 AM, Sriraman Tallam wrote: >> On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka wrote: >>> Hello, >>> I apologize for taking so long to get into this patch. I ad busy time (wedding >>> and teaching), should be back in regular schedule now. >>> >>>> Sri, can you provide examples to show why putting thunks into the same >>>> section as the target function when function reorder is on can be bad >>>> ? >>> >>> C++ ABI specify that they are in the same section, but I can't think of the >>> case where this would break. >>> Hmm, I suppose it is the TARGET_USE_LOCAL_THUNK_ALIAS_P code that breaks - >>> you end up with code in two sections where one is accessing local comdat >>> of the toher. I would also like to see testcase that breaks and is fixed by >>> your patch. I would expect that here, by not copying the section name, >>> you actually make things wose. >> >> Here is an example where the thunk and the original function get >> placed in different sections. >> >> class base_class_1 >> { >> public: >> virtual void vfn () {} >> }; >> >> class base_class_2 >> { >> public: >> virtual void vfn () {} >> }; >> void foo(); >> class need_thunk_class : public base_class_1, public base_class_2 >> { >> public: >> virtual void vfn () { >> for (int i = 0; i < 100000; ++i) >> foo(); >> } >> }; >> >> int main (int argc, char *argv[]) >> { >> base_class_1 *bc1 = new need_thunk_class (); >> bc1->vfn(); >> return 0; >> } >> >> int glob = 0; >> __attribute__((noinline)) >> void foo() >> { >> glob++; >> } >> >> >> I am making the function that needs thunk hot. Now, >> >> $ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition >> -fprofile-generate -ffunction-sections >> $ a.out >> $ g++ thunkex.cc -O2 -fno-reorder-blocks-and-partition -fprofile-use >> -ffunction-sections -c >> $ objdump -d thunkex.o >> >> Disassembly of section .text.hot._ZN16need_thunk_class3vfnEv: >> >> 0000000000000000 <_ZN16need_thunk_class3vfnEv>: >> 0: 53 push %rbx >> 1: bb a0 86 01 00 mov $0x186a0,%ebx >> ... >> >> Disassembly of section .text._ZN16need_thunk_class3vfnEv: >> >> 0000000000000000 <_ZThn8_N16need_thunk_class3vfnEv>: >> 0: 48 83 ef 08 sub $0x8,%rdi >> .... >> >> When the original function gets moved to .text.hot, the thunk does >> not. It is not always the case that the thunk should either. > > I forgot to add that this becomes confusing because, in this case, the > thunk is the only function sitting in a section whose name does not > correspond to its assembler name. If we are not going to have thunk > section names the same as the original function when profiles are > available and -freorder-functions is used, we as well change the name > of the thunk's section to correspond to its assembler name. That was > the intention of the patch. > > Thanks > Sri > > >> >> Thanks >> Sri >> >> >> >> >>> >>> I think we need to deal with this later; use_tunk is done long before >>> profiling is read and before we decide whether code is hot/cold. I suppose >>> the function reordering code may need to always walk whole comdat group and >>> ensure that sections are same? >>> I.e. pick the highest profile of a function in the group, resolve unique section >>> on it and then copy section names? I had verifier checking that section names >>> within one comdat groups are same, perhaps it was part of the reverted patch >>> for AIX. I will try to get that one back in now. >>> >>> Jan >>>> >>>> Thanks, >>>> >>>> David >>>> >>>> On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam wrote: >>>> > Hi Honza, >>>> > >>>> > Could you review this patch when you find time? >>>> > >>>> > Thanks >>>> > Sri >>>> > >>>> > On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam wrote: >>>> >> Ping. >>>> >> >>>> >> On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam wrote: >>>> >>> Ping. >>>> >>> >>>> >>> On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam wrote: >>>> >>>> Ping. >>>> >>>> >>>> >>>> On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam wrote: >>>> >>>>> Ping. >>>> >>>>> >>>> >>>>> On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam wrote: >>>> >>>>>> Hi, >>>> >>>>>> >>>> >>>>>> I would like this patch reviewed and considered for commit when >>>> >>>>>> Stage 1 is active again. >>>> >>>>>> >>>> >>>>>> Patch Description: >>>> >>>>>> >>>> >>>>>> A C++ thunk's section name is set to be the same as the original function's >>>> >>>>>> section name for which the thunk was created in order to place the two >>>> >>>>>> together. This is done in cp/method.c in function use_thunk. >>>> >>>>>> However, with function reordering turned on, the original function's section >>>> >>>>>> name can change to something like ".text.hot." or >>>> >>>>>> ".text.unlikely." in function default_function_section in varasm.c >>>> >>>>>> based on the node count of that function. The thunk function's section name >>>> >>>>>> is not updated to be the same as the original here and also is not always >>>> >>>>>> correct to do it as the original function can be hotter than the thunk. >>>> >>>>>> >>>> >>>>>> I have created a patch to not name the thunk function's section to be the same >>>> >>>>>> as the original function when function reordering is enabled. >>>> >>>>>> >>>> >>>>>> Thanks >>>> >>>>>> Sri