[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/
>
>
>