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

[David Blaikie via llvm-commits]

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)


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