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

Thu Sep 19 14:51:12 PDT 2019

Yes, sorry for being unclear, I mean that the counter, data, and value
variables do not need to be put in a group together. They can all be
individually made comdat in the usual way that is used for inline
functions, inline variables, templated things, etc. That should save object
file size for ELF, since we will no longer need a distinct __profv_${sym}
symbol table entry.

On Thu, Sep 19, 2019 at 2:47 PM Rong Xu <xur at google.com> wrote:

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