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

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.

lgtm, thanks.



================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:535-538
-    } else {
-      Discriminator = DILocation::getBaseDiscriminatorFromDiscriminator(
-          CallerFrame.Discriminator,
-          /* IsFSDiscriminator */ false);
----------------
wlei wrote:
> wenlei wrote:
> > 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. 
> I found that for CS line-number based context, the leaf frame of one address can be the middle of the whole context. In that case, we still need to manually decode the base discriminator for that frame.  This might cause some inconsistencies. 
Ok, sounds good. 


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

https://reviews.llvm.org/D109934