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

[David Blaikie via llvm-commits]

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?


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