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

[Wei He via Phabricator via llvm-commits]

weihe added inline comments.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1053-1054
+  /// score from FS scores of functions.
+  double weightByImportance(double FuncSimilarity, uint64_t BaseFuncSample,
+                            uint64_t TestFuncSample) const;
+};
----------------
wmi wrote:
> Both weightFuncSimilarity and weightByImportance considers the weight of the function in the profile, so at the beginning I felt confused what are their difference. I find out the difference is  weightFuncSimilarity absorbs the weight difference into the similarity so the similarity is still in the range of 0~1, while weightByImportance multiplies the similarity by weight ratio of the function in the profile (the average ratio of the two profiles), so the aggregate similarity of all the functions in the profiles will be in the range of 0~1. Please correct me if I am wrong. But it is better to make the intention of these two functions more clear in the comments.
Yes, that's right! In fact, the previous `weightFuncSimilarity()` is part of the computation of "function-level similarity", whereas `weightByImportance()` is part of the computation that aggregates "function-level similarity" to "profile-level similarity". So the two functions use weights in slightly different ways.

I also realized these two functions may be confusing, so I grouped the previous `weightFuncSimilarity()` (renamed as `weightForFuncSimilarity()`) and `computeSampleFunctionInternalOverlap()` into one `computeSampleFunctionOverlap()` function. In addition, I added comments to `weightForFuncSimilarity()` and `weightByImportance()` to explain the different purpose of these functions.

Thank you for pointing this out!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83852