Hi,
Just wondering if you got a chance to look at this?
Sri
On Tue, Jul 8, 2014 at 10:45 AM, Sriraman Tallam <tmsri...@google.com> wrote:
> 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
