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
