[PATCH] D13308: Value profiling - remaining LLVM changes

[Betul Buyukkurt via llvm-commits]

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.

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