Re: [PATCH] C++ thunk section names

[Sriraman Tallam <tmsriram at google dot com>]

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