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

[Xinliang David Li via llvm-commits]

On Fri, Feb 26, 2016 at 1:14 PM, Vedant Kumar <vsk at apple.com> wrote:

> > 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?
>
> Also, is that an ELF-specific issue?


yes.


> Either way, it would be helpful to extend the test with negative checks
> (CHECK-NOT that there are duplicated symbols on ELF, or with any other
> format).
>

This requires multiple modules and linking step so the test should be added
elsewhere.

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