[PATCH] Indirect call target profiling compiler-rt changes

[betulb at codeaurora.org]

Hi David,

Thanks for your comments. I'm making further changes to run time to make
sure the current scheme is extensible to allow profiling of multiple
different value kinds at once. The necessary changes does not seem to be
extensive. I'll have my updated patches available in two-three days.
Thanks,
-Betul

> ================
> Comment at: lib/profile/InstrProfiling.c:72
> @@ +71,3 @@
> +static inline uint32_t __llvm_profile_mem_assign(uintptr_t *Dest,
> uintptr_t Src) {
> +  static pthread_mutex_t DataMutex[64] = {PTHREAD_MUTEX_INITIALIZER};
> +  uint32_t Success = 0;
> ----------------
> No need for locking.
>
> ================
> Comment at: lib/profile/InstrProfiling.c:75
> @@ +74,3 @@
> +  uint32_t Index = ((uintptr_t)Dest >> 3) & 0x3F;
> +  pthread_mutex_lock(&DataMutex[Index]);
> +  if (Dest && 0 == *Dest) {
> ----------------
> No need for locking. The following cod can be replaced with
>
> Success = __sync_bool_compare_and_swap(Dest, 0, Src);
> return Success;
>
> ================
> Comment at: lib/profile/InstrProfiling.c:85
> @@ +84,3 @@
> +__attribute__((visibility("hidden")))
> +void __llvm_profile_instrument_indirect_call_site(uint8_t *TargetAddress,
> +  void *Data_, uint32_t CounterIndex) {
> ----------------
> Make the implementation more general matching the interface change.
>
> ================
> Comment at: lib/profile/InstrProfiling.c:106
> @@ +105,3 @@
> +        (uintptr_t)Mem)) {
> +      free(Mem);
> +      return;
> ----------------
> Consider defining and using allocator/deallocator interfaces (or macros).
>
> ================
> Comment at: lib/profile/InstrProfiling.c:117
> @@ +116,3 @@
> +    if (TargetAddress == CurrentIndirectTargetData->TargetAddress) {
> +      CurrentIndirectTargetData->NumTaken++;
> +      return;
> ----------------
> Consider using __sync_fetch_and_add
>
> ================
> Comment at: lib/profile/InstrProfiling.c:175
> @@ +174,3 @@
> +
> +    CurrentFunctionIndex++;
> +    if (!I->IndirectCallSiteCounters)
> ----------------
> Is there better function id to be used here? and how to detect mismatch?
>
> ================
> Comment at: lib/profile/InstrProfiling.c:193
> @@ +192,3 @@
> +          (__llvm_profile_indirect_target_data*)CurrentFunctionIndex;
> +        if (++CurrentResultIndex >= NumDataCount)
> +          return NumDataCount;
> ----------------
> How can this happen?
>
> ================
> Comment at: lib/profile/InstrProfiling.h:34
> @@ +33,3 @@
> +
> +typedef struct __llvm_profile_indirect_target_data {
> +  const uint8_t *TargetAddress;
> ----------------
> Perhaps change its name to __llvm_value_profile_target_data
>
> ================
> Comment at: lib/profile/InstrProfiling.h:35
> @@ +34,3 @@
> +typedef struct __llvm_profile_indirect_target_data {
> +  const uint8_t *TargetAddress;
> +  uint64_t NumTaken;
> ----------------
> change the type to uint64 and name to be TargetValue?
>
> ================
> Comment at: lib/profile/InstrProfiling.h:39
> @@ -31,1 +38,3 @@
> +  struct __llvm_profile_indirect_target_data *Next;
> +} __llvm_profile_indirect_target_data;
>
> ----------------
> Change it ot __llvm_value_profile_target_data
>
> ================
> Comment at: lib/profile/InstrProfiling.h:48
> @@ +47,3 @@
> +  const uint32_t NumIndirectCallSites;
> +  const uint8_t *FunctionPointer;
> +  __llvm_profile_indirect_target_data **IndirectCallSiteCounters;
> ----------------
> __llvm_prf_names is one of the biggest contributor to the binary size
> bloat. In the longer run we will need to get rid of the name based profile
> matching and rely on profile_id (hash) plus source line and cfg cksum if
> needed.
>
> When that happens, there will no need to record the function address
> because the value profiling site can be moved into the callee body which
> directly records the id associated with the callee.
>
> For now, this is fine.
>
> ================
> Comment at: lib/profile/InstrProfiling.h:78
> @@ +77,3 @@
> + */
> +void __llvm_profile_instrument_indirect_call_site(uint8_t *TargetAddress,
> +  void *Data_, uint32_t CounterIndex);
> ----------------
> Change the interface to be more general:
>
> __llvm_instrument_value_profile
>
> Add one more parameter to indicate the value profile kind. For now, only
> IC promotion is defined
>
> enum ValueProfileKind{
>   IndirectCallTarget = 0,
>  ....
> };
>
>
> ================
> Comment at: lib/profile/InstrProfiling.h:86
> @@ +85,3 @@
> + */
> +uint64_t __llvm_profile_gather_indirect_target_data(
> +  __llvm_profile_indirect_target_data** IndirectTargetProfilingResults);
> ----------------
> Similarly, make this interface more general.
>
> http://reviews.llvm.org/D9009
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>