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

[Xinliang David Li via llvm-commits]

On Sat, Nov 14, 2015 at 7:41 AM, Nathan Slingerland <slingn at gmail.com>
wrote:

> 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'?
>

right.


>
> 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.
>

Yes -- that is not desired -- the scaling needs to be applied while
merging.

>
> 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.
>

yes -- this way. The change should be small -- changing the add to
saturatingAdd in various places (with weight passed in).

thanks,

David

>
>
> http://reviews.llvm.org/D14547
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151114/09274f43/attachment.html>