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

[Rong Xu via llvm-commits]

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/fac81242/attachment.html>