Re: [PATCH] C++ thunk section names

[Sriraman Tallam <tmsriram at google dot com>]

Ping.



On Wed, Aug 6, 2014 at 2:42 PM, Sriraman Tallam <tmsriram@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 <tmsriram@google.com> wrote:
>> On Tue, Jul 8, 2014 at 10:38 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>> On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka <hubicka@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 <tmsriram@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 <tmsriram@google.com> wrote:
>>>>> >> Ping.
>>>>> >>
>>>>> >> On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>> >>> Ping.
>>>>> >>>
>>>>> >>> On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>> >>>> Ping.
>>>>> >>>>
>>>>> >>>> On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>>> >>>>> Ping.
>>>>> >>>>>
>>>>> >>>>> On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam <tmsriram@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