[PATCH] D103717: [InstrProfiling][ELF] Make __profd_ private if the function does not use value profiling

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:891
+  uint64_t NS = 0;
+  for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
+    NS += PD.NumValueSites[Kind];
----------------
MaskRay wrote:
> davidxl wrote:
> > should this code by guarded by whether value profile is enabled?
> For performance? 
> 
> The code below iterates over the array unconditionally
> ```
>   for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind)
>     Int16ArrayVals[Kind] = ConstantInt::get(Int16Ty, PD.NumValueSites[Kind]);
> ```
> so I think having another loop should be fine.
Basically in that case, there is no need to create the variable in the first place.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:930
   };
-  // If code never references data variables (the symbol is unneeded), and
-  // linker GC cannot discard data variables while the text section is retained,
-  // data variables can be private. This optimization applies on COFF and ELF.
-  if (!DataReferencedByCode && !TT.isOSBinFormatMachO()) {
+  // If the data variable is not referenced by code (when NS==0, i.e. no value
+  // profiling), and the counter keeps the data variable live under linker GC,
----------------
MaskRay wrote:
> davidxl wrote:
> > What is the root cause of the failure when NS == 0 condition is not used?
> 5798be84580be233e4cf34c08ceec8f79e80502e has the message.
> 
> Say a.cc has a prevailing comdat and b.cc's is non-prevailing.
> 
> b.cc's linkonce_odr comdat function may get inlined into a function not in a.cc.
> It is a linker error for the prevailing function to reference a discarded STB_LOCAL symbol (`__profd_` is non-prevailing and thus discarded)
> 
> If we don't change the linkage, `__profd_` is STB_GLOBAL STV_HIDDEN. It is fine to reference such a non-local symbol, which will bind to a.cc's copy.
When ValueProfileStaticAlloc is false, NS will also be zero, but the prf_data will still be referenced --  will it create the same linker error?

If that is the case, I think a better condition for ELF should be :

if (ValueProfileStaticAlloc && NS == 0 || !DataReferencedByCode) ...

!DataReferencedByCode is a bigger hammer to catch all.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103717/new/

https://reviews.llvm.org/D103717