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. 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