[PATCH] D109934: [llvm-profgen] Add duplication factor for line-number based profile

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:400
     if (!FrameVec.empty()) {
+      Count *= getDuplicationFactor(FrameVec.back().Callsite.Discriminator);
       FunctionSamples &FunctionProfile =
----------------
It looks like we get all duplication factors from callsite duplication factor, which is a bit weird given duplication factor applies to normal locations too. However this is a confusion cause by misleading naming. The name SampleContextFrame::Callsite is misleading because when used to represent leaf, there's no call site. It's FuncName/Location, rather than CallerName/Callsite.. I think the struct name was rename from Callsite into SampleContextFrame, but member names were kept as call site. Perhaps we should rename too, Hongtao?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:535-538
-    } else {
-      Discriminator = DILocation::getBaseDiscriminatorFromDiscriminator(
-          CallerFrame.Discriminator,
-          /* IsFSDiscriminator */ false);
----------------
hoy wrote:
> wlei wrote:
> > wenlei wrote:
> > > By removing this part, we will have duplication factor as part of location in NameStrings and call stack, is that what we want? Logically, duplication factor does not belong to location.
> > I checked that our internal tools also used duplication factor this way, i. e decoding it when we need it in profile generation. 
> > 
> > Alternatively,  we can also have a hash map like {offset, duplication factor} offset2DuplicationFactorMap which can be built on-demand as well, which one do you think is better?
> > 
> > 
> > 
> Looks like the duplication factor is dropped during profile key generation (for CS it's dropped in `getExpandedContext`), while it is used when computing sample counts. So it should work. 
> 
> Adding a map is cleaner, but I'm a bit concerned about its efficiency. 
> 
> 
I guess practically we only need to keep duplication factor for leaf frame, but not for middle call sites.. That way we don't need extra map. And call sites won't have different location due to duplication factor. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109934