<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Nov 14, 2015 at 7:41 AM, Nathan Slingerland <span dir="ltr"><<a href="mailto:slingn@gmail.com" target="_blank">slingn@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">slingn added inline comments.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/ProfileData/InstrProf.h:324<br>
@@ +323,3 @@<br>
+<br>
+  instrprof_error mergeValueProfData(uint32_t ValueKind,<br>
+                                     InstrProfRecord &Src) {<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Why moving this definition in-class? Try to avoid bringing irrelevant changes like this. It makes it harder to compare two versions.<br>
</span>It looks like this is part of the implementation of merge(). Is there a need for it to be public?<br>
<br>
I will refactor in a separate change set.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/ProfileData/InstrProf.h:413<br>
@@ -403,1 +412,3 @@<br>
<br>
+instrprof_error InstrProfRecord::merge(InstrProfRecord &Other) {<br>
+  // If the number of counters doesn't match we either have bad data<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Moving this function from InstrProfWriter.cpp to here is fine, but can you submit a different patch for that?<br>
</span>Will do.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/Support/MathExtras.h:656<br>
@@ -655,1 +655,3 @@<br>
<br>
+/// \brief Add two unsigned integers, X and Y, of type T.<br>
+/// Clamp the result to the maximum representable value of T on overflow.<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> Can you submit the saturating math change in a separate patch?<br>
</span>Will do.<br>
<span class=""><br>
================<br>
Comment at: lib/ProfileData/InstrProfWriter.cpp:120<br>
@@ +119,3 @@<br>
+    // We keep track of the max function count as we go for simplicity.<br>
+    if (Dest.Counts[0] > MaxFunctionCount)<br>
+      MaxFunctionCount = Dest.Counts[0];<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> 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)<br>
</span>OK.<br>
<span class=""><br>
================<br>
Comment at: tools/llvm-profdata/llvm-profdata.cpp:88<br>
@@ +87,3 @@<br>
+    for (auto &I : *Reader) {<br>
+      if (InputFile.Weight > 1) {<br>
+        I.scale(InputFile.Weight);<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> 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.<br>
</span>I assume you meant 'does not seem to be a need'?<br></blockquote><div><br></div><div>right.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br></div><div>Yes -- that is not desired -- the scaling needs to be applied while merging. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br></blockquote><div><br></div><div>yes -- this way. The change should be small -- changing the add to saturatingAdd in various places (with weight passed in).</div><div><br></div><div>thanks,</div><div><br></div><div>David</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="http://reviews.llvm.org/D14547" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14547</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>