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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:503
       continue;
+    uint64_t DC = Count * getDuplicationFactor(LeafLoc->Callsite.Discriminator);
     FunctionProfile.addCalledTargetSamples(LeafLoc->Callsite.LineOffset,
----------------
wlei wrote:
> wenlei wrote:
> > nit: `Count *= getDuplicationFactor(LeafLoc->Callsite.Discriminator)`? Same for other places.
> Fixed!
Am I not on the latest version? This seems not fixed.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:535-538
-    } else {
-      Discriminator = DILocation::getBaseDiscriminatorFromDiscriminator(
-          CallerFrame.Discriminator,
-          /* IsFSDiscriminator */ false);
----------------
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. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109934