[PATCH] LLVM changes for value profiling support

[Diego Novillo]

This looks generally OK to me.  At a high-level:

- 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.
- The patch needs IR tests.

Some detailed notes and questions below.


================
Comment at: include/llvm/ProfileData/InstrProf.h:43
@@ -39,1 +42,3 @@
 
+const std::error_category &instrprof_category();
+
----------------
This line move seems unnecessary.

================
Comment at: include/llvm/ProfileData/InstrProf.h:54
@@ +53,3 @@
+    reserved_2 = 2,
+    reserved_3 = 3,
+    last = 3,
----------------
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.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:18
@@ -17,3 +17,2 @@
 
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringExtras.h"
----------------
Please keep this. The file still uses ArrayRefs.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:249
@@ +248,3 @@
+      case 2: return ReadDataV1V2(K, D, N);
+      case 3: return ReadDataV3(K, D, N);
+    }
----------------
A question on format evolution. How far back should we go as the file format evolves?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4923
@@ -4923,1 +4922,3 @@
+  case Intrinsic::instrprof_instrument_target:
+    llvm_unreachable("instrprof failed to lower an value profiling call");
   case Intrinsic::frameescape: {
----------------
s/an value/a value/

================
Comment at: lib/ProfileData/InstrProfReader.cpp:306
@@ -283,2 +305,3 @@
+    std::vector<uint64_t> Counts;
     Counts.clear();
     Counts.reserve(RawCounts.size());
----------------
No need to clear Counts if you just declared it.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:399
@@ +398,3 @@
+      endian::readNext<uint64_t, little, unaligned>(D);
+    if (1 != FormatVersion)
+      ++I;
----------------
I find FormatVersion != 1 easier to read.

================
Comment at: test/Instrumentation/InstrProfiling/linkage.ll:26
@@ -25,3 +25,3 @@
 ; CHECK: @"__llvm_profile_counters_linkage.ll:foo_internal" = internal global
-; CHECK: @"__llvm_profile_data_linkage.ll:foo_internal" = internal constant
+; CHECK: @"__llvm_profile_data_linkage.ll:foo_internal" = internal
 define internal void @foo_internal() {
----------------
Any way that we could keep these variables in .rodata?

http://reviews.llvm.org/D8908

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/