[PATCH] LLVM changes for indirect call target profiling support

[David]

I have some preliminary comments. Here is a a more general comment:

I think this patch needs major rework to consolidate a lot changes. I think the same reader and in-memory data structure should be used to handle both versions of profile data on disk -- instead of the proposed way of splitting structures ..


================
Comment at: include/llvm/ProfileData/InstrProfReader.h:140
@@ -147,1 +139,3 @@
+  std::vector<InstrProfIndirectTargetRecord> IndirectTargetResults;
+  struct ProfileDataV1 {
     const uint32_t NameSize;
----------------
Why do we care about reading old profile data with new reader? If version mismatches, why not just report error and exit? 

On the other hand, v1 data should be readable by v2 reader (but not the other way around), so why is there need to keep the V1 data record ?

In other words, you can change V1 --> Base

struct ProfileDataBase {
...
};

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:147
@@ -152,2 +146,3 @@
   };
-  struct RawHeader {
+  struct ProfileDataV2 {
+    struct ProfileDataV1 V1Data;
----------------
just 

struct ProfileData {
   struct ProfileDataBase ...
};

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:151
@@ +150,3 @@
+    const IntPtrT FunctionPointer;
+    const IntPtrT IndirectTargetResults; // Always NULL, used only during runtime.
+  };
----------------
Make it ValueProfileResults[ValueProfileKindLast];

Similarly in other places.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:153
@@ -154,1 +152,3 @@
+  };
+  struct RawHeaderV1 {
     const uint64_t Magic;
----------------
RawHeaderBase

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:162
@@ -161,2 +161,3 @@
   };
-
+  struct RawHeaderV2 {
+    struct RawHeaderV1 V1Header;
----------------
RawHeader

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:167
@@ +166,3 @@
+  };
+  struct IndirectTargetProfilingData {
+    const IntPtrT TargetAddress;
----------------
--> ValueProfileData

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:168
@@ +167,3 @@
+  struct IndirectTargetProfilingData {
+    const IntPtrT TargetAddress;
+    const uint64_t NumTaken;
----------------
--> TargetValue or Value

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:178
@@ +177,3 @@
+  uint64_t IndirectTargetDataDelta;
+  const ProfileDataV1 *DataV1;
+  const ProfileDataV1 *DataV1End;
----------------
These are very confusing: there are two disk formats (v1, v2), but it does not mean we need have two in-memory data structure to store them.  The shared ProfileData should be able to handle both disk format raw data. 

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:24
@@ +23,3 @@
+// Profiling information for a single indirect call target
+struct InstrProfIndirectTargetRecord {
+  InstrProfIndirectTargetRecord() {};
----------------
This record type should be generalized to be ValueProfileRecord

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:29
@@ +28,3 @@
+    NumTaken(NumTaken), CounterIndex(CounterIndex) {}
+  SmallString<32> TargetFunctionName;
+  uint64_t NumTaken;
----------------
Why recording the raw name here? It can greatly increase the size overhead of the profile data. Can it be an offset/index into the string/name table?  Using an id here allows uint64 type which makes it possible to share the record type with other value profilers.

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:47
@@ +46,3 @@
+  std::vector<uint64_t> Counts;
+  std::vector<InstrProfIndirectTargetRecord> IndirectTargetResults;
+};
----------------
Should be an array of vectors indexed by profiler kind.

================
Comment at: include/llvm/ProfileData/InstrProfRecord.h:58
@@ +57,3 @@
+  std::vector<uint64_t> Counts;
+  std::vector<InstrProfIndirectTargetRecord> IndirectTargetResults;
+};
----------------
Similarly here.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:229
@@ +228,3 @@
+  FormatVersion = swap(Header->Version);
+  if (2 == FormatVersion) {
+    if (CurrentPos + sizeof(RawHeaderV2) > End)
----------------
Why can't v1 and v2 format share the same head reader? V2 reader should be able to read v1 head data -- the only difference is missing value profile data.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:244
@@ -228,3 +243,3 @@
 std::error_code
-RawInstrProfReader<IntPtrT>::readHeader(const RawHeader &Header) {
+RawInstrProfReader<IntPtrT>::readHeader(const RawHeaderV1 &Header) {
   if (swap(Header.Version) != getRawVersion())
----------------
I don't see the need to keep this function unless it is kept as refactored code shared by v2 readHeader.

http://reviews.llvm.org/D8908

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