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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

> When a flat CS profile is converted to a nested profile, the call target samples for inlined callee contexts are left over in the callsite target map.

Good catch.. That is also not consistent with AutoFDO profile -- a callee should be in either call site sample or target map of body sample, but not both.



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


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:375
+
+    return NoneType();
+  }
----------------
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? 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125266