[PATCH] D17654: [PGO] Remove duplicate profile entries for functions with available_externally linkage

Fri Feb 26 13:36:41 PST 2016

On Fri, Feb 26, 2016 at 1:33 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Fri, Feb 26, 2016 at 1:27 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Fri, Feb 26, 2016 at 1:18 PM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> On Fri, Feb 26, 2016 at 1:07 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Fri, Feb 26, 2016 at 12:32 PM, David Li via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> davidxl created this revision.
>>>>> davidxl added a reviewer: vsk.
>>>>> davidxl added a subscriber: llvm-commits.
>>>>>
>>>>> The per function data and counter variables for available externally
>>>>> functions are created with linkonce linkage (to prevent compiler from
>>>>> dropping counter vars). However on ELF based systems, linkonce symbols are
>>>>> created as weak symbols and the duplicated entries are not removed by the
>>>>> linker.
>>>>
>>>>
>>>> That ^ seems surprising/strange/confusing/problematic - any idea what's
>>>> going on there? Is that a bug in the compiler? linker? specification of
>>>> some kind?
>>>>
>>>
>>> I am not sure what you questions is. Duplicate definitions of weak
>>> symbols are allowed so linker won't discard the duplicate copies. The
>>> symbol reference will be resolved the strong definition by the linker.
>>>
>>
>> Generally, I'm trying to understand why the counter is handled
>> differently from the function itself.
>>
>> The comment mentions "To avoid link errors, profile counters for
>> function with available_externally linkage needs to be changed to
>> linkonce linkage. "
>> Why is that? Can we not rely on the counter to be emitted where the
>> function is emitted? (where the external definition of the function is)
>>
>>
>
> This is a good question. This is a design choice made long ago. For
> function with regular external linkage, the profile counter's linkage is
> set to 'internal', possibly to reduce the size overhead (by eliminating
> symbol table entries). Because of this, the available_external function's
> profile counter will no longer have a backup definition to be resolved to
> so their counter's linkage gets changed to linkonce.
>

It seems like the compiler's in control of available_externally
definitions, would it be possible that it would be in control of not
internalizing counters it knows it may use available_externally elsewhere?
(ie: when emitting the external definition of a thing that the compiler may
emit available_externally definitions for, do not internalize the counter?)

>
> David
>
>
>
>>
>>> David
>>>
>>>
>>>>
>>>>
>>>>> For bad consequences it causes, see comments in the code. One example,
>>>>> the profile counts for _ZNKSs7_M_dataEv method is duplicated 655 times in
>>>>> raw profile data of clang. In the merged indexed profile, the counter
>>>>> values are magnified 655x -- totally dwarfed other functions -- this also
>>>>> distorted profile summary a lot leading to not useful profile data.
>>>>>
>>>>> http://reviews.llvm.org/D17654
>>>>>
>>>>> Files:
>>>>>   lib/Transforms/Instrumentation/InstrProfiling.cpp
>>>>>   test/Instrumentation/InstrProfiling/linkage.ll
>>>>>
>>>>> Index: test/Instrumentation/InstrProfiling/linkage.ll
>>>>> ===================================================================
>>>>> --- test/Instrumentation/InstrProfiling/linkage.ll
>>>>> +++ test/Instrumentation/InstrProfiling/linkage.ll
>>>>> @@ -7,6 +7,7 @@
>>>>>  @__profn_foo_weak = weak hidden constant [8 x i8] c"foo_weak"
>>>>>  @"__profn_linkage.ll:foo_internal" = internal constant [23 x i8]
>>>>> c"linkage.ll:foo_internal"
>>>>>  @__profn_foo_inline = linkonce_odr hidden constant [10 x i8]
>>>>> c"foo_inline"
>>>>> + at __profn_foo_extern = linkonce_odr hidden constant [10 x i8]
>>>>> c"foo_extern"
>>>>>
>>>>>  ; COMMON: @__profc_foo = hidden global
>>>>>  ; COMMON: @__profd_foo = hidden global
>>>>> @@ -36,6 +37,15 @@
>>>>>    ret void
>>>>>  }
>>>>>
>>>>> +; LINUX: @__profc_foo_extern = linkonce_odr hidden global
>>>>> {{.*}}section "__llvm_prf_cnts", comdat($__profv_foo_extern), align 8
>>>>> +; LINUX: @__profd_foo_extern = linkonce_odr hidden global
>>>>> {{.*}}section "__llvm_prf_data", comdat($__profv_foo_extern), align 8
>>>>> +; OTHER: @__profc_foo_extern = linkonce_odr hidden global
>>>>> +; OTHER: @__profd_foo_extern = linkonce_odr hidden global
>>>>> +define available_externally void @foo_extern() {
>>>>> +  call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10
>>>>> x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
>>>>> +  ret void
>>>>> +}
>>>>> +
>>>>>  declare void @llvm.instrprof.increment(i8*, i64, i32, i32)
>>>>>
>>>>>  ; OTHER: @__llvm_profile_runtime = external global i32
>>>>> Index: lib/Transforms/Instrumentation/InstrProfiling.cpp
>>>>> ===================================================================
>>>>> --- lib/Transforms/Instrumentation/InstrProfiling.cpp
>>>>> +++ lib/Transforms/Instrumentation/InstrProfiling.cpp
>>>>> @@ -286,8 +286,38 @@
>>>>>    return F->hasAddressTaken();
>>>>>  }
>>>>>
>>>>> -static inline Comdat *getOrCreateProfileComdat(Module &M,
>>>>> +static inline bool needsComdatForCounter(Function &F, Module &M) {
>>>>> +
>>>>> +  if (F.hasComdat())
>>>>> +    return true;
>>>>> +
>>>>> +  Triple TT(M.getTargetTriple());
>>>>> +  if (!TT.isOSBinFormatELF())
>>>>> +    return false;
>>>>> +
>>>>> +  // See createPGOFuncNameVar for more details. To avoid link errors,
>>>>> profile
>>>>> +  // counters for function with available_externally linkage needs to
>>>>> be changed
>>>>> +  // to linkonce linkage. On ELF based systems, this leads to weak
>>>>> symbols to be
>>>>> +  // created. Without using comdat, duplicate entries won't be
>>>>> removed by the
>>>>> +  // linker leading to increased data segement size and raw profile
>>>>> size. Even
>>>>> +  // worse, since the referenced counter from profile per-function
>>>>> data object
>>>>> +  // will be resolved to the common strong definition, the profile
>>>>> counts for
>>>>> +  // available_externally functions will end up being duplicated in
>>>>> raw profile
>>>>> +  // data. This can result in distorted profile as the counts of
>>>>> those dups
>>>>> +  // will be accumulated by the profile merger.
>>>>> +  GlobalValue::LinkageTypes Linkage = F.getLinkage();
>>>>> +  if (Linkage != GlobalValue::ExternalWeakLinkage &&
>>>>> +      Linkage != GlobalValue::AvailableExternallyLinkage)
>>>>> +    return false;
>>>>> +
>>>>> +  return true;
>>>>> +}
>>>>> +
>>>>> +static inline Comdat *getOrCreateProfileComdat(Module &M, Function &F,
>>>>>                                                 InstrProfIncrementInst
>>>>> *Inc) {
>>>>> +  if (!needsComdatForCounter(F, M))
>>>>> +    return nullptr;
>>>>> +
>>>>>    // COFF format requires a COMDAT section to have a key symbol with
>>>>> the same
>>>>>    // name. The linker targeting COFF also requires that the COMDAT
>>>>>    // a section is associated to must precede the associating section.
>>>>> For this
>>>>> @@ -315,8 +345,7 @@
>>>>>    // linking.
>>>>>    Function *Fn = Inc->getParent()->getParent();
>>>>>    Comdat *ProfileVarsComdat = nullptr;
>>>>> -  if (Fn->hasComdat())
>>>>> -    ProfileVarsComdat = getOrCreateProfileComdat(*M, Inc);
>>>>> +  ProfileVarsComdat = getOrCreateProfileComdat(*M, *Fn, Inc);
>>>>>
>>>>>    uint64_t NumCounters = Inc->getNumCounters()->getZExtValue();
>>>>>    LLVMContext &Ctx = M->getContext();
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160226/05d50810/attachment-0001.html>