[PATCH] C++ thunk section names

[PATCH] C++ thunk section names

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

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

[-- Attachment #2: thunk_section_name_patch.txt --]
[-- Type: text/plain, Size: 2271 bytes --]

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. 


Index: testsuite/g++.dg/thunk_section_name.C
===================================================================
--- testsuite/g++.dg/thunk_section_name.C	(revision 0)
+++ testsuite/g++.dg/thunk_section_name.C	(revision 0)
@@ -0,0 +1,30 @@
+/* { dg-require-named-sections "" } */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-reorder-blocks-and-partition -ffunction-sections" } */
+
+class base_class_1
+{
+public:
+  virtual void vfn () {}
+};
+
+class base_class_2
+{
+public:
+  virtual void vfn () {}
+};
+
+class need_thunk_class : public base_class_1, public base_class_2
+{
+public:
+  virtual void vfn () {} 
+};
+
+int main (int argc, char *argv[])
+{
+  base_class_1 *c = new need_thunk_class ();
+  c->vfn();
+  return 0;
+}
+
+/* { dg-final { scan-assembler "\.text\._ZThn8_N16need_thunk_class3vfnEv" } } */
Index: cp/method.c
===================================================================
--- cp/method.c	(revision 207517)
+++ cp/method.c	(working copy)
@@ -362,8 +362,10 @@ use_thunk (tree thunk_fndecl, bool emit_p)
 	{
 	  resolve_unique_section (thunk_fndecl, 0, flag_function_sections);
 
-	  /* Output the thunk into the same section as function.  */
-	  DECL_SECTION_NAME (thunk_fndecl) = DECL_SECTION_NAME (function);
+	  /* Output the thunk into the same section as function if function reordering
+	     is not switched on.  */
+	  if (!flag_reorder_functions)
+	    DECL_SECTION_NAME (thunk_fndecl) = DECL_SECTION_NAME (function);
 	}
     }
 

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Xinliang David Li]

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
?

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

Re: [PATCH] C++ thunk section names

[Jan Hubicka]

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.

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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.

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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

Re: [PATCH] C++ thunk section names

[Sriraman Tallam]

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