Ping.
On Wed, Aug 6, 2014 at 2:42 PM, Sriraman Tallam <tmsri...@google.com> wrote: > Hi, > > Just wondering if you got a chance to look at this? > > Sri > > On Tue, Jul 8, 2014 at 10:45 AM, Sriraman Tallam <tmsri...@google.com> wrote: >> On Tue, Jul 8, 2014 at 10:38 AM, Sriraman Tallam <tmsri...@google.com> wrote: >>> On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka <hubi...@ucw.cz> 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 <tmsri...@google.com> >>>>> 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 <tmsri...@google.com> >>>>> > wrote: >>>>> >> Ping. >>>>> >> >>>>> >> On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam <tmsri...@google.com> >>>>> >> wrote: >>>>> >>> Ping. >>>>> >>> >>>>> >>> On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam >>>>> >>> <tmsri...@google.com> wrote: >>>>> >>>> Ping. >>>>> >>>> >>>>> >>>> On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam >>>>> >>>> <tmsri...@google.com> wrote: >>>>> >>>>> Ping. >>>>> >>>>> >>>>> >>>>> On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam >>>>> >>>>> <tmsri...@google.com> 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.<orginal>" or >>>>> >>>>>> ".text.unlikely.<original>" 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