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

[Nathan Slingerland via llvm-commits]

slingn added inline comments.

================
Comment at: include/llvm/ProfileData/InstrProf.h:324
@@ +323,3 @@
+
+  instrprof_error mergeValueProfData(uint32_t ValueKind,
+                                     InstrProfRecord &Src) {
----------------
davidxl wrote:
> Why moving this definition in-class? Try to avoid bringing irrelevant changes like this. It makes it harder to compare two versions.
It looks like this is part of the implementation of merge(). Is there a need for it to be public?

I will refactor in a separate change set.

================
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
----------------
davidxl wrote:
> Moving this function from InstrProfWriter.cpp to here is fine, but can you submit a different patch for that?
Will do.

================
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.
----------------
davidxl wrote:
> Can you submit the saturating math change in a separate patch?
Will do.

================
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];
----------------
davidxl wrote:
> 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)
OK.

================
Comment at: tools/llvm-profdata/llvm-profdata.cpp:88
@@ +87,3 @@
+    for (auto &I : *Reader) {
+      if (InputFile.Weight > 1) {
+        I.scale(InputFile.Weight);
----------------
davidxl wrote:
> 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. 
I assume you meant 'does not seem to be a need'?

OK. That will mean making a copy of the record internally in order to scale it since AddRecord shouldn't assume that it is allowed to mutate the passed in record.

The other alternative is to revert to the previous method of applying the weight during merge. That would be more efficient (than making a temporary scaled record) but spreads out the weighting logic across a number of methods.


http://reviews.llvm.org/D14547