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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:370
+        Count = NumSamples;
+      NumSamples -= Count;
+      CallTargets.erase(I);
----------------
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.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:375
+
+    return NoneType();
+  }
----------------
wenlei wrote:
> You used optional instead relying on zero to indicate no removal here because there could be count be zero count returned due to mismatch but removal is still happening? 
Yes, it is to identify mismatch or zero-count call targets. The latter should be very rare though.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:528
+      if (Count)
+        NodeProfile->subTotalSamples(*Count);
     }
----------------
wenlei wrote:
> nit: subtractTotalSamples to be clear.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125266