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

[David Li via Phabricator via llvm-commits]

davidxl 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 {
----------------
How to tell if count or percentage is stored here?


================
Comment at: llvm/include/llvm/ProfileData/InstrProf.h:597
+  uint64_t NumEntries;
+  double EdgeCount;
+  double ValueCounts[IPVK_Last - IPVK_First + 1];
----------------
The name EdgeCount is misleading -- it sounds like number of 'edges' in CFG.  Perhaps just 'TotalCount' or 'CountSum'


================
Comment at: llvm/include/llvm/ProfileData/InstrProf.h:612
+  // Sum of the total count values for the base profile.
+  CountSumOrPercent BaseSum;
+  // Sum of the total count values for the test profile.
----------------
BaseSum-->Base


================
Comment at: llvm/include/llvm/ProfileData/InstrProf.h:614
+  // Sum of the total count values for the test profile.
+  CountSumOrPercent TestSum;
+  // Overlap lap score. Should be in range of [0.0f to 1.0f].
----------------
TestSum --> Test


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:482
 
+void InstrProfRecord::getCountSums(CountSumOrPercent &Sum) const {
+  uint64_t FuncSum = 0;
----------------
This is not 'getCountSums' -- but 'accumuateCounts' -- perhaps rename it.


================
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) {
----------------
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.
}


================
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);
----------------
already be computed and is nonzero


================
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);
----------------
Perhaps just check the function hash?


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:596
+                                       FuncLevelOverlap.TestSum.EdgeCount);
+    FuncLevelOverlap.Overlap.EdgeCount += FuncScore;
+    FuncLevelOverlap.Overlap.NumEntries = Other.Counts.size();
----------------
+= or just = ?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:212
+  if (Error E = ReaderOrErr.takeError()) {
+    // Skip the empty profiles by returning sliently.
+    instrprof_error IPE = InstrProfError::take(std::move(E));
----------------
-> silently

I don't see a return here.


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

https://reviews.llvm.org/D60977