[PATCH] D51643: [SampleFDO] Make sample profile loader unaware of compact format change

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: include/llvm/ProfileData/SampleProf.h:464
+        /// original local name. In sample profile, the suffixes of function
+        /// names are all stripped. Since it is possible that the mapper is
+        /// built in post-thin-link phase and var promotion has been done,
----------------
danielcdh wrote:
> davidxl wrote:
> > danielcdh wrote:
> > > wmi wrote:
> > > > davidxl wrote:
> > > > > why are those suffixes stripped in sample profile?
> > > > It is possible that the binary sampled is built in thinlto mode and some functions are promoted. The profile generated from such binary will contain names after promotion. If such profile is used in thinlto pre-link-phase or nonthinlto build, we need to strip those names from sample before we can find a match in the module.  
> > > > 
> > > > Indeed if there are multiple local functions with the same name are promoted, their samples will be merged together after the strip, which will introduce some imprecision. Maybe this part can be improved -- don't strip names from sample, but always promote the local names in the module before trying to match it with the name from sample. I am not sure whether the effort is worthy. Dehao may already evaluate it before? 
> > > I remember that name stripping is important as there are many different types of suffixes added to the function name which makes it impossible to match. But I do not remember the exact impact. It may be worth re-evaluate.
> > > 
> > > For the local functions, what's the promoted name look like after thinlto? If it's deterministic, then we should not strip it.
> > How are local functions handled without thinlto for samplePGO? Instrumentatation PGO always qualify the name with module name.
> They'll just share the function name have all the profiles merged, which I agree is bad, but I don't think we have a way to identify which module this function is coming from.
Since this issue is not something new, we can defer this later.


Repository:
  rL LLVM

https://reviews.llvm.org/D51643