<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 11, 2015 at 8:29 AM, Diego Novillo <span dir="ltr"><<a href="mailto:dnovillo@google.com" target="_blank">dnovillo@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This looks generally OK to me.  At a high-level:<br>
<br>
- I'm not sure what to do wrt old versions. We can keep accumulating back compatibility as the format evolves, but at some point we'll want to stop.<br>
- The patch needs IR tests.<br>
<br>
Some detailed notes and questions below.<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/InstrProf.h:43<br>
@@ -39,1 +42,3 @@<br>
<br>
+const std::error_category &instrprof_category();<br>
+<br>
----------------<br>
This line move seems unnecessary.<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/InstrProf.h:54<br>
@@ +53,3 @@<br>
+    reserved_2 = 2,<br>
+    reserved_3 = 3,<br>
+    last = 3,<br>
----------------<br>
Any idea on why reserving 3 slots? Why not more? Or fewer? I guess I'm asking what other kinds of value profiling do you have in mind for future changes.<br></blockquote><div><br></div><div>I missed this one. Three is good enough for the near future, for instance stringOp value profiling, div/rem value profiling, etc.</div><div> </div><div>David</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: include/llvm/ProfileData/InstrProfReader.h:18<br>
@@ -17,3 +17,2 @@<br>
<br>
-#include "llvm/ADT/ArrayRef.h"<br>
 #include "llvm/ADT/StringExtras.h"<br>
----------------<br>
Please keep this. The file still uses ArrayRefs.<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/InstrProfReader.h:249<br>
@@ +248,3 @@<br>
+      case 2: return ReadDataV1V2(K, D, N);<br>
+      case 3: return ReadDataV3(K, D, N);<br>
+    }<br>
----------------<br>
A question on format evolution. How far back should we go as the file format evolves?<br>
<br>
================<br>
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4923<br>
@@ -4923,1 +4922,3 @@<br>
+  case Intrinsic::instrprof_instrument_target:<br>
+    llvm_unreachable("instrprof failed to lower an value profiling call");<br>
   case Intrinsic::frameescape: {<br>
----------------<br>
s/an value/a value/<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProfReader.cpp:306<br>
@@ -283,2 +305,3 @@<br>
+    std::vector<uint64_t> Counts;<br>
     Counts.clear();<br>
     Counts.reserve(RawCounts.size());<br>
----------------<br>
No need to clear Counts if you just declared it.<br>
<br>
================<br>
Comment at: lib/ProfileData/InstrProfReader.cpp:399<br>
@@ +398,3 @@<br>
+      endian::readNext<uint64_t, little, unaligned>(D);<br>
+    if (1 != FormatVersion)<br>
+      ++I;<br>
----------------<br>
I find FormatVersion != 1 easier to read.<br>
<br>
================<br>
Comment at: test/Instrumentation/InstrProfiling/linkage.ll:26<br>
@@ -25,3 +25,3 @@<br>
 ; CHECK: @"__llvm_profile_counters_linkage.ll:foo_internal" = internal global<br>
-; CHECK: @"__llvm_profile_data_linkage.ll:foo_internal" = internal constant<br>
+; CHECK: @"__llvm_profile_data_linkage.ll:foo_internal" = internal<br>
 define internal void @foo_internal() {<br>
----------------<br>
Any way that we could keep these variables in .rodata?<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D8908&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=GrEGXkW4iLTbG_5Ip-qeZYQZtC3Mt4NdnsouVsEW2Ms&s=pd7fzP_0sQ81f_szgse5T1wTn9winEBFjnnQNZiELGc&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D8908</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=GrEGXkW4iLTbG_5Ip-qeZYQZtC3Mt4NdnsouVsEW2Ms&s=itNe2ULc8dL8aj6LHDt2nb3Vkwe6oA-GTuBsOIY8BSY&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>