[PATCH] D14547: [llvm-profdata] Add support for weighted merge of profile data

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/ProfileData/InstrProf.h:324
@@ +323,3 @@
+
+  instrprof_error mergeValueProfData(uint32_t ValueKind,
+                                     InstrProfRecord &Src) {
----------------
Why moving this definition in-class? Try to avoid bringing irrelevant changes like this. It makes it harder to compare two versions.

================
Comment at: include/llvm/ProfileData/InstrProf.h:413
@@ -403,1 +412,3 @@
 
+instrprof_error InstrProfRecord::merge(InstrProfRecord &Other) {
+  // If the number of counters doesn't match we either have bad data
----------------
Moving this function from InstrProfWriter.cpp to here is fine, but can you submit a different patch for that?

================
Comment at: include/llvm/Support/MathExtras.h:656
@@ -655,1 +655,3 @@
 
+/// \brief Add two unsigned integers, X and Y, of type T.
+/// Clamp the result to the maximum representable value of T on overflow.
----------------
Can you submit the saturating math change in a separate patch?

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:117
@@ -140,1 +116,3 @@
+
+    instrprof_error result = Dest.merge(I);
 
----------------
Result is not checked here.

================
Comment at: lib/ProfileData/InstrProfWriter.cpp:120
@@ +119,3 @@
+    // We keep track of the max function count as we go for simplicity.
+    if (Dest.Counts[0] > MaxFunctionCount)
+      MaxFunctionCount = Dest.Counts[0];
----------------
Since you refactored the code with an else branch, you can sink the max function count update and return statement after the the if-then-else.  I think this should be done in a separate patch (same one as moving the merge from writer cpp to instrprof.cpp)

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:88
@@ +87,3 @@
+    for (auto &I : *Reader) {
+      if (InputFile.Weight > 1) {
+        I.scale(InputFile.Weight);
----------------
There does seem to be a need to exposed the 'scale' method publicly -- it is better to make AddRecord to take an optional weight parameter as the previous version, and let AddRecord's implementation to call private scale method. 

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:138
@@ -118,2 +137,3 @@
+      }
       ProfileMap[FName].merge(Samples);
     }
----------------
Similarly here merge can take an optional weight parameter.


http://reviews.llvm.org/D14547