[PATCH] LLVM changes for indirect call target profiling support

[Betul Buyukkurt]

================
Comment at: docs/LangRef.rst:8089
@@ +8088,3 @@
+
+The first argument is a pointer to a global variable containing the
+name of the entity being instrumented. This should generally be the
----------------
davidxl wrote:
> Can entity anything else other than function? If not, make it clear 'name of the function ...'.
The description for instrprof_increment intrinsic uses the same language, so I did not make the proposed change to preserve the genericity.

================
Comment at: docs/LangRef.rst:8093
@@ +8092,3 @@
+
+The second argument is a hash value that can be used by the consumer
+of the profile data to detect changes to the instrumented source. It
----------------
davidxl wrote:
> Why do we need to do this at instruction level? Function level mismatch checking by the consumer should be good enough.
It's function level. Hash values are used to check changes in the function's source.

================
Comment at: docs/LangRef.rst:8099
@@ +8098,3 @@
+The third argument represents the kind of the value profiling that is done.
+The fourth argument refers to the value profiled at the instrumented expression.
+The last argument indicates to which of the instrumented expressions for ``name``
----------------
davidxl wrote:
> The fourth argument is the value of the expression being profiled.
Done.

================
Comment at: docs/LangRef.rst:8100
@@ +8099,3 @@
+The fourth argument refers to the value profiled at the instrumented expression.
+The last argument indicates to which of the instrumented expressions for ``name``
+should the profiled value be recorded. It should be >= 0.
----------------
davidxl wrote:
> The last argument is the index of the instrumented expression within the function.
Done.

================
Comment at: include/llvm/IR/IntrinsicInst.h:386
@@ +385,3 @@
+
+    GlobalVariable *getName() const {
+      return cast<GlobalVariable>(
----------------
davidxl wrote:
> Maybe GetFunctionName?
I tried to stay consistent with instrprof_increment intrinsic, thus this change is not implemented. I'll revisit, if insistent.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:149
@@ +148,3 @@
+    struct ProfileDataV1 V1Data;
+    const IntPtrT FunctionPointer;
+    const uint32_t NumIndirectCallSites;
----------------
davidxl wrote:
> Changing ProfileData structure like this will eventually lead to reader code to be come really messy and unmanageable. 
> 
> This is a simple way to avoid this change -- simply 'peel' the value profile related per-function data into a different data section. It will make adding new value profilers in the future much simpler.
I agree. I tried to derive another class using this reader class as a base. Thus, made sure that the code for the RawInstrProfReader is not changed. However, the peeling and moving value data into its own section had one issue i.e. reliance on the linker to make sure the ith elements of ProfileData and ValueData elements correspond to the same instrumented function in the same order.

We read and process the raw profile data in iteration of ProfileData elements. I was not sure if the elements of ProfileData and ValueData sections would always correlate to one another.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:162
@@ -161,2 +161,3 @@
   };
-
+  struct RawHeaderV2 {
+    struct RawHeaderV1 V1Header;
----------------
davidxl wrote:
> Similarly, instead of changing the common header, simply split out the value profiler related control info into subheaders, so that the file layout looks like this:
> 
>     RawHeader
>     ValueProfilerHeader1
>     ValueProfilerHeader2
> 
> The version number in the main raw header indicates the presence or absense of the value profiler header in the file, and can decide to read it or skip it.
> 
> By so doing, the header reading code can be shared between different versions.
> 
> I strongly suggest you change like this ..
> 
In the current runtime, the value header gives the combined value data size for all value kinds. The order w/in the raw profile data file looks as follows:

The value data entries for value kinds [0-ValueKindLast] for Function 1
The value data entries for value kinds [0-ValueKindLast] for Function 2  ...
The value data entries for value kinds [0-ValueKindLast] for Function N

Header gives the combined value data size for all value kinds for all functions. The profile data file and all readers/writers are future proofed as they're already set up to output/read multiple value kinds.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:140
@@ -147,1 +139,3 @@
+  std::vector<InstrProfIndirectTargetRecord> IndirectTargetResults;
+  struct ProfileDataV1 {
     const uint32_t NameSize;
----------------
davidxl wrote:
> 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 {
> ...
> };
Not sure, if I understood the concern here. We only care about reading the old data (v1) w/ the new (v2) reader. I used the version names (v1/v2) to match the data structures to the relevant data formats explicitly.

I'm doing precisely what you're suggesting, except that ProfileDataBase == ProfileDataV1 and ProfileData == ProfileDataV2. I'll do the name change if the change is insisted.

http://reviews.llvm.org/D8908

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