[PATCH] D60977: [llvm-profdata] Add overlap command to compute similarity b/w two profile files

[Rong Xu via Phabricator via llvm-commits]

xur marked 12 inline comments as done.
xur added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/InstrProf.h:594
+// To store the sums of profile count values, or the percentage of
+// the sums of the total count values.
+struct CountSumOrPercent {
----------------
davidxl wrote:
> How to tell if count or percentage is stored here?
The caller should keep track of this. This data structure does not know -- it just a floating points.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:511
+  double Score = 0.0f, FuncLevelScore = 0.0f;
+  for (auto J = Input.ValueData.begin(), JE = Input.ValueData.end(); J != JE;
+       ++J) {
----------------
davidxl wrote:
> The loop nest can be merged into one single loop:
> 
> while (i != ie && j != je) {
> 
>     if (i->v == j->v) {
>        //match:
>        ...
>        i++; j++;
>     } else if (i->v > j->v) {
>         j++;
>         // handle mismatch case -- as one value appears in only one record (assuming count == 0)
>        ...
>     } else {
>         i++;
>          // handle mismatch
>     }
>   }
>   // handle the rest of the mismatched cases.
> }
Yes. The version I used is a copy from merge function. 
Your version is better here because it will terminate when one of arrays reaches the end.



================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:553
+                              uint64_t ValueCutoff) {
+  // FuncLevel CountSum for other should already computed and nonzero.
+  assert(FuncLevelOverlap.TestSum.EdgeCount >= 1.0f);
----------------
davidxl wrote:
> already be computed and is nonzero
Yes. it's true for call chains in this patch. 
I put an assert here just to catch others callers that not init TestSum.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:560
+  if (!Mismatch) {
+    for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; ++Kind) {
+      uint32_t ThisNumValueSites = getNumValueSites(Kind);
----------------
davidxl wrote:
> Perhaps just check the function hash?
FuncHash already being checked (In Reader). This is just to handle the cases that escapes the hash check.


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:596
+                                       FuncLevelOverlap.TestSum.EdgeCount);
+    FuncLevelOverlap.Overlap.EdgeCount += FuncScore;
+    FuncLevelOverlap.Overlap.NumEntries = Other.Counts.size();
----------------
davidxl wrote:
> += or just = ?
they are the same.  the incoming value should be 0 by the initializer. 


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

https://reviews.llvm.org/D60977