[PATCH] LLVM changes for indirect call target profiling support

[Xinliang David Li]

thanks. This will not only make the formate mostly backward
compatible, but also make the changes simpler and more isolated.
Adding more value profilers in the future will also be easy  by
following the same boiler plate.

David


On Fri, May 8, 2015 at 4:15 PM, Betul Buyukkurt <betulb at codeaurora.org> wrote:
>
> 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/
>>
>>
>>
>
>