<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 8, 2017 at 11:28 AM, David Li via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">davidxl added inline comments.<br>
<br>
<br>
================<br>
<span class="gmail-">Comment at: lib/Transforms/<wbr>Instrumentation/<wbr>InstrProfiling.cpp:546<br>
+      if (Kind == IPVK_MemOPSize)<br>
+        TotalNS += I * (MemOPSizeRangeLast + 1);<br>
+    }<br>
</span><span class="gmail-">----------------<br>
xur wrote:<br>
> davidxl wrote:<br>
</span><span class="gmail-">> > Larger range does not mean on average the number of values tracked will be proportionally larger. Perhaps using a different heuristic for allocation size? Besides, TotalNS tracks the number of value sites, overloading it for number of values does not make sense<br>
> I removed this change. Now MemOPSize profile uses the same heuristic as indirect-call profiling.<br>
</span>With this default heuristics, do you see any dropped value profiling at runtime?<br>
<br></blockquote><div><br></div><div>I need to rerun the tests. Will report back.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
================<br>
Comment at: tools/llvm-profdata/llvm-<wbr>profdata.cpp:449<br>
<br>
+typedef struct ValueSitesStats {<br>
+  ValueSitesStats()<br>
----------------<br>
Can you split out this part as a refactoring patch?<br>
<br></blockquote><div><br></div><div>Split patch submitted here:</div><div><a href="https://reviews.llvm.org/D30752">https://reviews.llvm.org/D30752</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D28965" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28965</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>