[PATCH] D83852: [llvm-profdata] Implement llvm-profdata overlap for sample profiles

[Wei He via Phabricator via llvm-commits]

weihe marked 2 inline comments as done.
weihe added inline comments.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:858
+  /// This function updates sample overlap statistics of an overlap function in
+  /// base and test profile. It also calculates a function-internal similarity
+  /// FIS as follows:
----------------
hoyFB wrote:
> function-level similarity? 
Thank you for the comment! This is not exactly the function-level similarity we report in the tool, but an intermediate result towards it. The formula of function-level similarity is given at line 882.

I renamed this function to `computeSampleFunctionInternalOverlap()` and moved it to a private member to reduce confusions.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1048
+  for (const auto &F : BaseHotFunc) {
+    const auto &Match = TestHotFunc.find(F.first());
+    if (Match == TestHotFunc.end())
----------------
hoyFB wrote:
> I think you want just `const auto Match = ...`.  The return value will be returned by reference if it is large.
Thank you for the suggestion! I've changed the code accordingly.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1050
+    if (Match == TestHotFunc.end())
+      continue;
+
----------------
hoyFB wrote:
> May just increase `HotFuncOverlap.UnionCount` before continue, instead of erasing every matched function from `TestHotFunc`? like
> 
> 
> ```
> for (const auto &F : TestHotFunc) {
>     if (BaseHotFunc.count(F.first()))
>        ++HotFuncOverlap.OverlapCount;
>     else
>        ++HotFuncOverlap.UnionCount;
> }
> ```
> 
This suggestion is really good! The refactored code is much cleaner. Thank you very much!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83852