[PATCH] LLVM changes for indirect call target profiling support

[David]

Looks good overall. Some of the refactoring (e.g. IndexedProfReader related ones) also look cleaner (than the base version). See below for other comments:


================
Comment at: include/llvm/ProfileData/InstrProf.h:27
@@ +26,3 @@
+    indirect_call_target = 0,
+    last = 1
+};
----------------
I suggest change it to 3 with reserved slots for new value profiler in the future.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:179
@@ -184,2 +178,3 @@
   std::error_code readHeader(const RawHeader &Header);
+  std::error_code readData(InstrProfRecord &Record);
   template <class IntT>
----------------
InstrProfLookupTrait class has readData method. To avoid confusion, change this one to

readInstProfCounts or readInstrProfBranchCounts ..

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:310
@@ +309,3 @@
+    DataBuffer.clear();
+    uint64_t NumCounts;
+    uint64_t NumEntries = N / sizeof(uint64_t);
----------------
initialize the variable.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:318
@@ +317,3 @@
+
+      if (++I >= NumEntries)
+        return data_type();
----------------
Error handling?

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:329
@@ +328,3 @@
+      // If we have more counts than data, this is bogus.
+      if (I + NumCounts > NumEntries)
+        return data_type();
----------------
No error handling or warning? 

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:352
@@ +351,3 @@
+    for (unsigned I = 0; I < DataBufferSize; ++I) {
+      if (D - Start + sizeof (uint64_t) + sizeof(uint32_t) >= N)
+        return data_type();
----------------
Add a comment to explain here.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:356
@@ +355,3 @@
+      uint32_t CountsSize = endian::readNext<uint32_t, little, unaligned>(D);
+      if (D - Start + CountsSize * sizeof(uint64_t) >= N)
+        return data_type();
----------------
error handling?

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:359
@@ +358,3 @@
+      CounterBuffer.clear();
+      for (unsigned I = 0; I < CountsSize; ++I)
+        CounterBuffer.push_back(endian::readNext<uint64_t, little, unaligned>(D));
----------------
use a different induction variable name even though it is in a different scope.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:367
@@ +366,3 @@
+        if (D - Start + 2 * sizeof(uint32_t) > N)
+          return data_type();
+        uint32_t ValueKind = endian::readNext<uint32_t, little, unaligned>(D);
----------------
All the errors related corrupted data should be handled somewhere, e.g, jumping to  common error handling label...

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:373
@@ +372,3 @@
+        ValueBuffer.clear();
+        for (unsigned I = 0; I < ValuesSize; ++I) {
+          if (D - Start + sizeof(uint32_t) >= N) return data_type();
----------------
use a different name for induction variable.

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:34
@@ +33,3 @@
+    CounterIndex(CounterIndex) {}
+  SmallString<32> Name;
+  uint64_t Value;
----------------
C++ mangled names are usually very long.

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:35
@@ +34,3 @@
+  SmallString<32> Name;
+  uint64_t Value;
+  uint64_t NumTaken;
----------------
Value and Name do not co-exist -- is it possible to make a union out of it to save space?

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:45
@@ +44,3 @@
+    : Name(Name), Hash(Hash), Counts(Counts) {}
+  void clearValues() {
+    uint32_t i = instr_value_prof_kind::first;
----------------
The method name is a little misleading. It seems to imply  branch profile counts are also cleared.

Suggest to change it to 'clearValueProfData'..

http://reviews.llvm.org/D8908

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