[PATCH] LLVM changes for indirect call target profiling support

[Betul Buyukkurt]

Hi David,

I agree to your suggestions. Not only not chaining the headers, I'm also
looking into not chaining the ProfileData. I'll move the value related
fields into another struct w/ a pointer to the ProfileData variable of the
function. This data will be in its own section. Thanks for the review
comments.

-Betul

> Betul, I have some suggestions on major restructuring of the code, so

>  there are not many detailed review comments in this round.

> 

> David

> 

> ================

>  Comment at: docs/LangRef.rst:8086

>  @@ +8085,3 @@

>  +

>  +Arguments:

>  +""""""""""

> 

> - The argument order of the intrinsic is not consistent with the underlying runtime function. It might be better to keep them in sync.

> 

>   ================ 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 ---------------- Can entity anything else other than function? If not, make it clear 'name of the function ...'.

> 

>   ================ 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 ---------------- Why do we need to do this at instruction level? Function level mismatch checking by the consumer should be good enough.

> 

>   ================ 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`` ---------------- The fourth argument is the value of the expression being profiled.

> 

>   ================ 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. ---------------- The last argument is the index of the instrumented expression within the function.

> 

>   ================ Comment at: include/llvm/IR/IntrinsicInst.h:386 @@ +385,3 @@ + +    GlobalVariable *getName() const { +      return cast<GlobalVariable>( ---------------- Maybe GetFunctionName?

> 

>   ================ Comment at: include/llvm/ProfileData/InstrProfReader.h:149 @@ +148,3 @@ +    struct ProfileDataV1 V1Data; +    const IntPtrT FunctionPointer; +    const uint32_t NumIndirectCallSites; ---------------- 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.

> 

>   ================ Comment at: include/llvm/ProfileData/InstrProfReader.h:162 @@ -161,2 +161,3 @@ }; - +  struct RawHeaderV2 { +    struct RawHeaderV1 V1Header; ---------------- 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 ..

> 

>   http://reviews.llvm.org/D8908

> 

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

> 

> 



http://reviews.llvm.org/D8908

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