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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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];
----------------
davidxl wrote:
> 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.
The loop has 2 iterations, so I think should be fine.

`for (uint32_t Kind = 0; Kind <= 1; ++Kind)`

We could use `zeroinitializer` but that would require that we have other means to know `@llvm.instrprof.value.profile` isn't used.


================
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,
----------------
davidxl wrote:
> 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.
`computeNumValueSiteCounts`  visits every `@llvm.instrprof.value.profile` instruction.

If `computeNumValueSiteCounts` is called, `NS` will be non-zero. So I guess it is a good indicator, regardless of `ValueProfileStaticAlloc`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103717