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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

Thanks for the work. It is a very useful feature.



================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:966-967
+  ///    function A, given BS(i) returned by computeBlockSimilarity(), compute
+  ///    FIS(A) = 2.0 - sum_i(1.0 - BS(i)), ranging in [0.0f to 2.0f] with 0.0
+  ///    meaning no overlap.
+  double computeSampleFunctionInternalOverlap(
----------------
Can we make the similarity within range 0~1 to be consistent with  Block and profile similarity? It is more natural to reason the similarity with range 0~1.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1068-1074
+  auto BaseSample = BaseFunc.getBodySamples().cbegin();
+  auto TestSample = TestFunc.getBodySamples().cbegin();
+  auto BaseSampleEnd = BaseFunc.getBodySamples().cend();
+  auto TestSampleEnd = TestFunc.getBodySamples().cend();
+  uint64_t BaseSampleValue = 0;
+  uint64_t TestSampleValue = 0;
+  while (BaseSample != BaseSampleEnd || TestSample != TestSampleEnd) {
----------------
Seemly a lot of complexity of the function comes from lock step iteration of the maps from two profiles at the same time. Could you extract the lock step iteration logic into a separate class? This way we don't have to deal with the logic multiple times in iterating BodySampleMap, CallsiteSamplesMap and FunctionSamplesMap.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1093-1098
+      FuncOverlap.UnionSample += std::max(BaseSampleValue, TestSampleValue);
+      FuncOverlap.OverlapSample += std::min(BaseSampleValue, TestSampleValue);
+      ++FuncOverlap.OverlapCount;
+      Difference += 1.0 - computeBlockSimilarity(BaseSampleValue,
+                                                 TestSampleValue, FuncOverlap);
+      updateHotBlockOverlap(BaseSampleValue, TestSampleValue, 1);
----------------
updateForUnmatchedBlock is a special case of the block above. We may be able to share the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83852