[PATCH] D13308: Value profiling - remaining LLVM changes

[Xinliang David Li via llvm-commits]

On Mon, Oct 5, 2015 at 10:16 AM, Betul Buyukkurt <betulb at codeaurora.org> wrote:
>
> Hi David,
>
> Thanks for your comments. I've posted earlier today the CL for the
> compiler-rt changes i.e. http://reviews.llvm.org/D9009 .
>
> These two (LLVM and compiler-rt) CL's have to be merged together to not
> break the profiler. The raw profile file is not backwards compatible.
>
> http://reviews.llvm.org/D9009 implements the format discussed in the below
> referenced threads back in early June:
>
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150525/279181.html
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150601/279539.html
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150608/280575.html
>
>> davidxl added inline comments.
>>
>> ================
>> Comment at: include/llvm/IR/IntrinsicInst.h:377
>> @@ +376,3 @@
>> +  /// This represents the llvm.instrprof_value_profile_target intrinsic.
>> +  class InstrProfValueProfileTargetInst : public IntrinsicInst {
>> +  public:
>> ----------------
>> Suggest dropping 'Target' in the name. Just InstrProfValueProfileInstr.
>> Target is strongly tied to indirect call value profiling.
>>
>> ================
>> Comment at: include/llvm/IR/IntrinsicInst.h:403
>> @@ +402,3 @@
>> +
>> +    ConstantInt *getIndex() const {
>> +      return cast<ConstantInt>(const_cast<Value *>(getArgOperand(4)));
>> ----------------
>> Add  a comment: returns value site index.
>>
>> ================
>> Comment at: include/llvm/IR/Intrinsics.td:325
>> @@ +324,3 @@
>> +// through instrumentation based profiling.
>> +def int_instrprof_value_profile_target : Intrinsic<[],
>> +                                                   [llvm_ptr_ty,
>> llvm_i64_ty,
>> ----------------
>> Suggest drop 'target'.
>>
>> ================
>> Comment at: include/llvm/ProfileData/InstrProfReader.h:172
>> @@ -159,1 +171,3 @@
>>    const char *ProfileEnd;
>> +  const uint32_t *CurrentVDataCounts;
>> +  const ValueData *CurrentVData;
>> ----------------
>> Realistically it is not useful to track more than 10 values per value
>> site.
>> For this reason, please change the VDataCount type to be unsigned char.
>>
>> ================
>> Comment at: lib/ProfileData/InstrProfReader.cpp:235
>> @@ -234,1 +234,3 @@
>>    auto NamesSize = swap(Header.NamesSize);
>> +  auto PaddingSize = sizeof(uint64_t) - NamesSize % sizeof(uint64_t);
>> +  auto ValueDataCountsSize = swap(Header.ValueDataCountsSize);
>> ----------------
>> Please draw a diagram describing raw data layout here:
>>
>> Header:
>>   ....
>> prf_data_section:
>>    ....
>> prf_cnts_section:
>>    ...
>> prf_name_section:
>>     ...
>> padding1
>> vdata_counts_section: // # of counts per site
>>   ...
>> padding2
>> vdata_value_section:
>>    ...
>>
>>
>> ================
>> Comment at: lib/ProfileData/InstrProfReader.cpp:333
>> @@ +332,3 @@
>> +      InstrProfValueSiteRecord NewVRecord;
>> +      for (uint32_t VIndex = 0; VIndex < CurrentVDataCount; ++VIndex) {
>> +        uint64_t TargetValue = swap(CurrentVData->Value);
>> ----------------
>> Should CurrentVData be set to Data->Values?
>>
>> Current implementation seems to assume that VData and VDataCounts are laid
>> out in the same order as the prf_data wrt the owning functions. I don't
>> think this assumption is right.
>
> The value profile data appears as linked lists hanging from the per
> function profile data structs at runtime. Therefore, an extra step is
> performed to turn the value profile data lists into a consecutive array
> form. We do that by iterating over the per function profile data structs.
> This as an artifact caused the value profile data and the count arrays to
> be laid out in the same order with the per function profile data.

I think it is better to not depend on this implementation detail at
runtime.   The current implementation also depends on the fact that
raw profile data is accessed sequentially.  Hidden assumptions like
this are not great. It also make the access of value profile data for
a given function is different from edge profile counters.

The problem can be easily solved: there are two fields in ProfileData
structure that can be used for this purpose:
1) the functionPointer field which is currently not used can be used
to store the PerSiteCountsPtr for that function
2) the Values field can be used to store ValuePtr for the function

In the header, add PerSiteCountsDelta and ValueDelta field.

With those,  Value data for a given function can be obtained similarly
to getCounter


David



>
> Raw profile file format is as follows:
>
> Header: Added in two uint64_t elements for value data counts array size
> and value data array size
>
> Data array : Array may end at a uint32_t boundary when generated on
> uint32_t targets. I'll have to fix the current CL to maintain data
> sections ending at uint64_t boundaries. Current CL does not have a padding
> after the data array.
>
> Counters array : same as before
> Name array : same as before
> Padding to end the name array at uint64_t boundary : same as before
>
> Value data counts array: Array of uint32_t values, each value indicates
> how many uint64_t data pairs {TargetValue, NumTaken} to read from the
> value data array per value site.
>
> Padding to end the value data counts array at uint64_t boundary
>
> Value data array: Array of uint64_t
>
> -Betul
>>
>>
>>
>>
>> http://reviews.llvm.org/D13308
>>
>>
>>
>>
>
>