[PATCH] D67579: [PGO] Use linkonce_odr linkage for __profd_ variables in comdat groups

[Rong Xu via llvm-commits]

I did not get the runtime performance. But there is something wrong with
the profile. Some count values are too large.

@Reid Kleckner <rnk at google.com> I looked at 372182. It seems to still set
all the variable as COMDAT. But instead of grouping them together, you have
a different name for each variable. Is this what you mean as not using
comdat group?

-Rong

On Thu, Sep 19, 2019 at 2:13 PM Xinliang David Li <davidxl at google.com>
wrote:

> difference in performance? How about CSPGO?
>
> On Thu, Sep 19, 2019 at 12:25 PM Rong Xu <xur at google.com> wrote:
>
>> I tested the idea using llvm bootstrap.
>> Without comdat group, the object file sizes as well as the library
>> archive sizes are smaller.
>> But the section sizes in the final binary (like clang-10) increase
>> significantly.
>> This is using lld linker. I think we need more investigation here.
>>
>> Here is the section sizes for clang-10.
>> =========== with comdat grouping ===============
>>  26 __llvm_prf_cnts 008a9a08  00000000058a29c8  00000000058a29c8
>>  0569f9c8  2**3
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  27 __llvm_prf_data 004842c0  000000000614c3d0  000000000614c3d0
>>  05f493d0  2**3
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  28 __llvm_prf_vals 00047d28  00000000065d0690  00000000065d0690
>>  063cd690  2**3
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  29 __llvm_prf_vnds 00142c60  00000000066183c0  00000000066183c0
>>  064153c0  2**5
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  30 __llvm_prf_names 0028f48c  000000000675b020  000000000675b020
>>  06558020  2**0
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  31 __llvm_orderfile 00000000  00000000069ea4ac  00000000069ea4ac
>>  067e74ac  2**2
>>                   CONTENTS, ALLOC, LOAD, DATA
>>
>> ============= without comdat grouping ===============
>>  26 __llvm_prf_cnts 00a896a8  00000000058a2908  00000000058a2908
>>  0569f908  2**3
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  27 __llvm_prf_data 006b4cc0  000000000632bfb0  000000000632bfb0
>>  06128fb0  2**3
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  28 __llvm_prf_vals 00061e88  00000000069e0c70  00000000069e0c70
>>  067ddc70  2**3
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  29 __llvm_prf_vnds 00142c60  0000000006a42b00  0000000006a42b00
>>  0683fb00  2**5
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  30 __llvm_prf_names 0028f48c  0000000006b85760  0000000006b85760
>>  06982760  2**0
>>                   CONTENTS, ALLOC, LOAD, DATA
>>  31 __llvm_orderfile 00000000  0000000006e14bec  0000000006e14bec
>>  06c11bec  2**2
>>                   CONTENTS, ALLOC, LOAD, DATA
>>
>> On Wed, Sep 18, 2019 at 9:58 AM Rong Xu <xur at google.com> wrote:
>>
>>>
>>>
>>> On Tue, Sep 17, 2019 at 8:32 PM Xinliang David Li <davidxl at google.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Sep 17, 2019 at 4:25 PM Rong Xu <xur at google.com> wrote:
>>>>
>>>>> +cc David
>>>>>
>>>>> On Tue, Sep 17, 2019 at 3:20 PM Reid Kleckner via Phabricator <
>>>>> reviews at reviews.llvm.org> wrote:
>>>>>
>>>>>> rnk added a comment.
>>>>>>
>>>>>> From what I understand, the comdat group isn't necessary, so I
>>>>>> disabled it on COFF in r372182. Can we simplify ELF to match?
>>>>>>
>>>>>
>>>> Putting prof sections with the function in the same comdat group is
>>>> probably not needed. There might be some weird interaction with GC and
>>>> instrumentation binary size. If we don't see issues there, it is ok to
>>>> simplify ELF to match.  Rong can help with some internal testing (instr
>>>> build size, performance etc) on internal large app with linux.
>>>>
>>>> They are not in the same compdat group as the functions -- they have
>>> their own group.
>>> I agree that if this affects the data section size (which means
>>> duplicates counter/variables), we should keep the current way. But I don't
>>> double this is the case.
>>> I will do some testing on removing comdat on ELF.
>>>
>>> -Rong
>>>
>>> David
>>>>
>>>>
>>>>> It's probably OK without setting COMDAT for the variables in ELF as
>>>>> the linkonce_odr  linkage can be used to remove the duplicates.
>>>>> On the other hand, it's nature to set the COMDAT to sync the
>>>>> declaration of the function.
>>>>> I'm CCing david to see what is his thoughts.
>>>>>
>>>>> -Rong
>>>>>
>>>>>>
>>>>>>
>>>>>> Repository:
>>>>>>   rL LLVM
>>>>>>
>>>>>> CHANGES SINCE LAST ACTION
>>>>>>   https://reviews.llvm.org/D67579/new/
>>>>>>
>>>>>> https://reviews.llvm.org/D67579
>>>>>>
>>>>>>
>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190919/d24747c8/attachment.html>