[PATCH] Indirect call target profiling compiler-rt changes

[Betul Buyukkurt]

Thanks for the comments.


================
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;
----------------
davidxl wrote:
> No need for locking.
Done.

================
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) {
----------------
davidxl wrote:
> No need for locking. The following cod can be replaced with
> 
> Success = __sync_bool_compare_and_swap(Dest, 0, Src);
> return Success;
Done.

================
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) {
----------------
davidxl wrote:
> Make the implementation more general matching the interface change.
Done.

================
Comment at: lib/profile/InstrProfiling.c:106
@@ +105,3 @@
+        (uintptr_t)Mem)) {
+      free(Mem);
+      return;
----------------
davidxl wrote:
> Consider defining and using allocator/deallocator interfaces (or macros).
This is not done in this revision. Will revisit if requested again.

================
Comment at: lib/profile/InstrProfiling.c:117
@@ +116,3 @@
+    if (TargetAddress == CurrentIndirectTargetData->TargetAddress) {
+      CurrentIndirectTargetData->NumTaken++;
+      return;
----------------
davidxl wrote:
> Consider using __sync_fetch_and_add
Not implemented in this revision. Will revisit if requested again.

================
Comment at: lib/profile/InstrProfiling.c:175
@@ +174,3 @@
+
+    CurrentFunctionIndex++;
+    if (!I->IndirectCallSiteCounters)
----------------
davidxl wrote:
> Is there better function id to be used here? and how to detect mismatch?
I'm not clear if I understood the questions here. The implicit order of the data array is used as an index for the value data to refer back to the data associated w/ the instrumented function. What do you mean by mismatch?

================
Comment at: lib/profile/InstrProfiling.c:193
@@ +192,3 @@
+          (__llvm_profile_indirect_target_data*)CurrentFunctionIndex;
+        if (++CurrentResultIndex >= NumDataCount)
+          return NumDataCount;
----------------
davidxl wrote:
> How can this happen?
It can happen in threaded environments. While the data is copied to the array, another thread can preempt and update the linked lists by adding an entry. This would change the total number of value data count. We do not want to write out of bounds of the array allocated at the beginning of the routine.


================
Comment at: lib/profile/InstrProfiling.h:34
@@ +33,3 @@
+
+typedef struct __llvm_profile_indirect_target_data {
+  const uint8_t *TargetAddress;
----------------
davidxl wrote:
> Perhaps change its name to __llvm_value_profile_target_data
Modified to __llvm_profile_value_data. 

================
Comment at: lib/profile/InstrProfiling.h:35
@@ +34,3 @@
+typedef struct __llvm_profile_indirect_target_data {
+  const uint8_t *TargetAddress;
+  uint64_t NumTaken;
----------------
davidxl wrote:
> change the type to uint64 and name to be TargetValue?
Done.

================
Comment at: lib/profile/InstrProfiling.h:39
@@ -31,1 +38,3 @@
+  struct __llvm_profile_indirect_target_data *Next;
+} __llvm_profile_indirect_target_data;
 
----------------
davidxl wrote:
> Change it ot __llvm_value_profile_target_data
Changed to __llvm_profile_value_data.

================
Comment at: lib/profile/InstrProfiling.h:49
@@ -38,1 +48,3 @@
+  const uint8_t *FunctionPointer;
+  __llvm_profile_indirect_target_data **IndirectCallSiteCounters;
 } __llvm_profile_data;
----------------
davidxl wrote:
> This can be changed into an array indexed by value profiler kind. Similarly, the NumIndirectCallsites.
> 
> const uint32_t NumValueSites[ValueProfileKindLast];
> __llvm_value_profile_target_data **ValueProfileCounters[ValueProfileKindLast];
Done.

================
Comment at: lib/profile/InstrProfiling.h:78
@@ +77,3 @@
+ */
+void __llvm_profile_instrument_indirect_call_site(uint8_t *TargetAddress,
+  void *Data_, uint32_t CounterIndex);
----------------
davidxl wrote:
> 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,
>  ....
> };
> 
Chaned to __llvm_profile_instrument_target. target here refers to the target expressions that are instrumented for value profiling.

================
Comment at: lib/profile/InstrProfiling.h:86
@@ +85,3 @@
+ */
+uint64_t __llvm_profile_gather_indirect_target_data(
+  __llvm_profile_indirect_target_data** IndirectTargetProfilingResults);
----------------
davidxl wrote:
> Similarly, make this interface more general.
Done.

http://reviews.llvm.org/D9009

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/