[PATCH] D125266: [CSSPGO][CSProfileConverter] Remove call target samples when including callee samples into caller.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:370
+        Count = NumSamples;
+      NumSamples -= Count;
+      CallTargets.erase(I);
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > The asymmetricity between removeCalledTarget and addCalledTarget can be confusing. We're reducing NumSamples in removeCalledTarget, but we are not adding removeCalledTarget in addCalledTarget. Two questions, 1) should NumSamples include target counts, and are we doing that consistently? 2) Any ways to make them the two functions consistent?
> > > NumSamples and target counts are computed separately in llvm-profgen, one is based on lbr ranges while the other is based on branch samples. So they do not have any connection on the profile generator side.
> > > 
> > > I guess we can separate `removeSamples` out of `removeCalledTarget`  to be consistent. The same would also be needed on the FunctionSample side. Maybe just rename `removeCalledTarget` to `removeCalledTargetAndBodySamples`? For now they should always be reduced together.
> > Ok, now I see that they're added separately. I'm wondering what is the definition of `NumSamples`. If the two don't have connection and `NumSamples` is samples for this location, and call target count is the count for specific call targets, then they should be changed independently. 
> > 
> > Here changing the call/callee representation from CallTargets to nested FunctionSamples shouldn't change the value of `NumSamples` because the we would still derived the same value from the same ranges?
> By no connection I mean how they are generated in llvm-profgen and used in the compiler technically. But logically `NumSamples` should be the sum of all target counts. It reflects the the execution count of the callsite. Removing one target means the callsite won't be executed as many times as before. The reduced count will be instead reflected on another code path that promotes the indirect callsite. Annotating block frequency based on the original callsite count (due to the max operation) could be misleading.
Followed up offline. So the key is that NumSamples of call site BodySample only contains execution counts for not inline calls. And nest profile implies inlining, so changing representation from call target to nest profile does need to reduce NumSample for call site.. So it's good here. 

The asymmetricity in naming and what they do is still less than ideal though. Perhaps for SampleRecord we can just make it consistent by having one removeCalledTarget that only removes target, and, and one removeBodySamples that only reduces NumSamples.

For FunctionSamples, removeCalledTarget-> removeCalledTargetAndBodySample?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125266