[profile] Fix value profile runtime merging issues

[xur]

Summary

This patch fixed two issues:
(1) value profile merging did not worked as the strong definition of the merge hook function never worked. We were not doing the merging of value profile. This patch remove the weak attribute and assign the hook dynamically.
(2) in case value profile is not merged, truncated the file so that we don't have garbage in the end of the profile.

[davidxl]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

non value profiling data has fixed size, so why is this needed?

[xur]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

Because there are still value counts in both in-memory and in-disk profiles.

Using the test in the patch as the example (running on my machine).
assuming the binary is named as 'gen' and VPMergeHook null.

if we don't do online merge,
"gen" will generate a profile of size 320 bytes.
"gen 1" will generate a profile with size of 352 bytes

If we run "gen 1 && gen", the result binary is still 352 bytes.
But the effective size is 320 bytes. The tail 32 bytes are trash.

[davidxl]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

Without VpMergeHook, we should assert that there is no value profile data -- it should not be dumped in the first place.

[xur]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

current behavior is to write out the in-memory value profile data and discard the value profile data in the file. if there is not value profile from the file, the result is still correct.

I'm not familiar with the use cast the not having libc (where VPMergeHook will be nullptr). Is it not suppose to have any value profile?

I can assert VPMergeHook==nullptr and in-memory valuesites are not empty. That will be easy. Is this what you suggested?

[davidxl]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

In lprofGetVPDataReader method, returns null if VPMergeHook is null.

[xur]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

This will generate incorrect profile: the valuesite in the data section says there are valuesites but VP count is empty.

if we do this, the behavior would be: llvm-profdata show or merge command will stop silently at the first function that has the value-site instrumentation. The rest profile will be discarded.

Note, of course, this only applies to VPMergeHook==nullptr case.

[davidxl]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

On the other hand, it seems safe to do truncation here unconditionally before the writing later.

[davidxl]

lib/profile/InstrProfilingFile.c

230 ↗

(On Diff #139655)

one correction. When Merge is done, the namedata section will be skipped during dumping to speed up the profile write. Make sure you do the truncation to the right size.

[xur]

This is the updated patch. It fixed some issues in previous patch. It also adds a new API for updating value-count in batch mode. Without this, PGO instrumented Clang incurs 20x slow down in building itself (with %9m prefix -- with %m is even greater slowdown ).

[davidxl]

lib/profile/InstrProfilingValue.c

138 ↗

(On Diff #140479)

Is this call getting inlined in the library build? If not, it will introduce unnecessary overhead.

One way to avoid this is to define a static always inline function, and then make the two APIs as wrappers to that common function.

[xur]

I checked the assembly in
clang_rt.profile-i386.dir/ and
clang_rt.profile-x86_64.dir/

Both are inlined.

[xur]

My previous refers to using gcc as the compiler.

With llvm (itself, i.e. trunk) as the compiler, it's not inlined.

I will update the patch using a wrapper.

[xur]

using wrappers to always_inline function to avoid function call overhead.

[davidxl]

lib/profile/InstrProfilingValue.c

135 ↗

(On Diff #140485)

need to put the attribute((..)) into InstrProfilingPort.h under GNUC section.

[davidxl]

lib/profile/InstrProfilingMerge.c

40 ↗

(On Diff #140485)

There is an existing interface for this:

__llvm_profile_get_size_for_buffer_internal

lib/profile/InstrProfilingValue.c

157 ↗

(On Diff #140485)

+= CountValue ?

[xur]

move inline attribute into InstrProfilingPort.h

[xur]

lib/profile/InstrProfilingMerge.c

40 ↗

(On Diff #140485)

using that API needs to pass the begin and end address for each section.
Here we use all the counters recorded in the header.

lib/profile/InstrProfilingValue.c

157 ↗

(On Diff #140485)

yes. I cannot believe this is not caught by all the tests.

[xur]

integrated David's review comments

[davidxl]

lib/profile/InstrProfilingMerge.c

40 ↗

(On Diff #140485)

Those pointers are available via llvm_profile_begin_xxx /llvm_profile_end_xxx.

It is better to avoid duplicating the logic of computing the profile data size.

[xur]

using __llvm_profile_get_size_for_buffer_internal directly to get the profile size.

[xur]

Fixed type cast warnings.

[davidxl]

lib/profile/InstrProfilingFile.c

189 ↗

(On Diff #140509)

What I suggested is to use public APIs:

llvm_profile_begin_data(), llvm_profile_end_data(), llvm_profile_begin_counters(), llvm_profile_end_counters(), so that you don't even need to to assume the raw profile layout here.

[xur]

lib/profile/InstrProfilingFile.c

189 ↗

(On Diff #140509)

you are right! as we have checked the compatibility, both versions should have the same format.

[xur]

integrated David's review comments to use an available api.

[davidxl]

lgtm

[sqlbyme]

Hi,
It appears the commit related to this review has cause a bot failure on
Green Dragon. This failure was masked by an earlier failure which was
reverted a short time ago. Please have a look at:
http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/44030/ and
either revert your commit or notify me if you will be submitting a patch
within the next hour.

Respectfully,
Mike Edwards

compiler-rt/trunk/lib/profile/InstrProfiling.h

  * void __llvm_profile_instrument_target(uint64_t TargetValue, void *Data,
  *                                       uint32_t CounterIndex);
  */
 void INSTR_PROF_VALUE_PROF_FUNC(
 #define VALUE_PROF_FUNC_PARAM(ArgType, ArgName, ArgLLVMType) ArgType ArgName
 #include "InstrProfData.inc"
     );
 
+void __llvm_profile_instrument_target_value(uint64_t TargetValue, void *Data,
+                                            uint32_t CounterIndex,
+                                            uint64_t CounterValue);
+
 /*!
  * \brief Write instrumentation data to the current file.
  *
  * Writes to the file with the last name given to \a *
  * __llvm_profile_set_filename(),
  * or if it hasn't been called, the \c LLVM_PROFILE_FILE environment variable,
  * or if that's not set, the last name set to INSTR_PROF_PROFILE_NAME_VAR,
  * or if that's not set,  \c "default.profraw".

compiler-rt/trunk/lib/profile/InstrProfilingFile.c

if (__llvm_profile_check_compatibility(ProfileBuffer, ProfileFileSize)) {
     (void)munmap(ProfileBuffer, ProfileFileSize);
     PROF_WARN("Unable to merge profile data: %s\n",
               "source profile file is not compatible.");
     return 0;
   }
 
   /* Now start merging */
   __llvm_profile_merge_from_buffer(ProfileBuffer, ProfileFileSize);
-  (void)munmap(ProfileBuffer, ProfileFileSize);
 
+  // Truncate the file in case merging of value profile did not happend to
+  // prevent from leaving garbage data at the end of the profile file.
+  ftruncate(fileno(ProfileFile), __llvm_profile_get_size_for_buffer());
+
+  (void)munmap(ProfileBuffer, ProfileFileSize);
   *MergeDone = 1;
 
   return 0;
 }
 
 /* Create the directory holding the file, if needed. */
 static void createProfileDir(const char *Filename) {
   size_t Length = strlen(Filename);

 }
 
 /* Write profile data to file \c OutputName.  */
 static int writeFile(const char *OutputName) {
   int RetVal;
   FILE *OutputFile;
 
   int MergeDone = 0;
+  VPMergeHook = &lprofMergeValueProfData;
   if (!doMerging())
     OutputFile = fopen(OutputName, "ab");
   else
     OutputFile = openFileForMerging(OutputName, &MergeDone);
 
   if (!OutputFile)
     return -1;
 

compiler-rt/trunk/lib/profile/InstrProfilingMerge.c

 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 #include "InstrProfilingUtil.h"
 
 #define INSTR_PROF_VALUE_PROF_DATA
 #include "InstrProfData.inc"
 
-COMPILER_RT_WEAK void (*VPMergeHook)(ValueProfData *,
-                                     __llvm_profile_data *) = NULL;
+COMPILER_RT_VISIBILITY
+void (*VPMergeHook)(ValueProfData *, __llvm_profile_data *);
+
 COMPILER_RT_VISIBILITY
 uint64_t lprofGetLoadModuleSignature() {
   /* A very fast way to compute a module signature.  */
   uint64_t CounterSize = (uint64_t)(__llvm_profile_end_counters() -
                                     __llvm_profile_begin_counters());
   uint64_t DataSize = __llvm_profile_get_data_size(__llvm_profile_begin_data(),
                                                    __llvm_profile_end_data());
   uint64_t NamesSize =

compiler-rt/trunk/lib/profile/InstrProfilingMergeFile.c

 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 #include "InstrProfilingUtil.h"
 
 #define INSTR_PROF_VALUE_PROF_DATA
 #include "InstrProfData.inc"
 
-void (*VPMergeHook)(ValueProfData *,
-                    __llvm_profile_data *) = &lprofMergeValueProfData;
-
 /* Merge value profile data pointed to by SrcValueProfData into
  * in-memory profile counters pointed by to DstData.  */
 void lprofMergeValueProfData(ValueProfData *SrcValueProfData,
                              __llvm_profile_data *DstData) {
-  unsigned I, S, V, C;
+  unsigned I, S, V, DstIndex = 0;
   InstrProfValueData *VData;
   ValueProfRecord *VR = getFirstValueProfRecord(SrcValueProfData);
   for (I = 0; I < SrcValueProfData->NumValueKinds; I++) {
     VData = getValueProfRecordValueData(VR);
+    unsigned SrcIndex = 0;
     for (S = 0; S < VR->NumValueSites; S++) {
       uint8_t NV = VR->SiteCountArray[S];
       for (V = 0; V < NV; V++) {
-        for (C = 0; C < VData[V].Count; C++)
-          __llvm_profile_instrument_target(VData[V].Value, DstData, S);
+        __llvm_profile_instrument_target_value(VData[SrcIndex].Value, DstData,
+                                               DstIndex, VData[SrcIndex].Count);
+        ++SrcIndex;
       }
+      ++DstIndex;
     }
     VR = getValueProfRecordNext(VR);
   }
 }

compiler-rt/trunk/lib/profile/InstrProfilingPort.h

 #define COMPILER_RT_ALIGNAS(x) __declspec(align(x))
 #define COMPILER_RT_VISIBILITY
 /* FIXME: selectany does not have the same semantics as weak. */
 #define COMPILER_RT_WEAK __declspec(selectany)
 /* Need to include <windows.h> */
 #define COMPILER_RT_ALLOCA _alloca
 /* Need to include <stdio.h> and <io.h> */
 #define COMPILER_RT_FTRUNCATE(f,l) _chsize(_fileno(f),l)
+#define COMPILER_RT_ALWAYS_INLINE __forceinline
 #elif __GNUC__
 #define COMPILER_RT_ALIGNAS(x) __attribute__((aligned(x)))
 #define COMPILER_RT_VISIBILITY __attribute__((visibility("hidden")))
 #define COMPILER_RT_WEAK __attribute__((weak))
 #define COMPILER_RT_ALLOCA __builtin_alloca
 #define COMPILER_RT_FTRUNCATE(f,l) ftruncate(fileno(f),l)
+#define COMPILER_RT_ALWAYS_INLINE inline __attribute((always_inline))
 #endif
 
 #if defined(__APPLE__)
 #define COMPILER_RT_SEG "__DATA,"
 #else
 #define COMPILER_RT_SEG ""
 #endif
 

compiler-rt/trunk/lib/profile/InstrProfilingValue.c

static ValueProfNode *allocateOneNode(__llvm_profile_data *Data, uint32_t Index,
   /* Due to section padding, EndVNode point to a byte which is one pass
    * an incomplete VNode, so we need to skip the last incomplete node. */
   if (Node + 1 > EndVNode)
     return 0;
 
   return Node;
 }
 
-COMPILER_RT_VISIBILITY void
-__llvm_profile_instrument_target(uint64_t TargetValue, void *Data,
-                                 uint32_t CounterIndex) {
+static COMPILER_RT_ALWAYS_INLINE void
+instrumentTargetValueImpl(uint64_t TargetValue, void *Data,
+                          uint32_t CounterIndex, uint64_t CountValue) {
   __llvm_profile_data *PData = (__llvm_profile_data *)Data;
   if (!PData)
     return;
-
+  if (!CountValue)
+    return;
   if (!PData->Values) {
     if (!allocateValueProfileCounters(PData))
       return;
   }
 
   ValueProfNode **ValueCounters = (ValueProfNode **)PData->Values;
   ValueProfNode *PrevVNode = NULL;
   ValueProfNode *MinCountVNode = NULL;
   ValueProfNode *CurVNode = ValueCounters[CounterIndex];
   uint64_t MinCount = UINT64_MAX;
 
   uint8_t VDataCount = 0;
   while (CurVNode) {
     if (TargetValue == CurVNode->Value) {
-      CurVNode->Count++;
+      CurVNode->Count += CountValue;
       return;
     }
     if (CurVNode->Count < MinCount) {
       MinCount = CurVNode->Count;
       MinCountVNode = CurVNode;
     }
     PrevVNode = CurVNode;
     CurVNode = CurVNode->Next;
if (VDataCount >= VPMaxNumValsPerSite) {
      * To handle this situation, user can choose to increase the max
      * number of tracked values per value site. Alternatively, a more
      * expensive eviction mechanism can be implemented. It requires
      * the runtime to track the total number of evictions per-site.
      * When the total number of evictions reaches certain threshold,
      * the runtime can wipe out more than one lowest count entries
      * to give space for hot targets.
      */
-    if (!MinCountVNode->Count || !(--MinCountVNode->Count)) {
+    if (MinCountVNode->Count <= CountValue) {
       CurVNode = MinCountVNode;
       CurVNode->Value = TargetValue;
-      CurVNode->Count++;
-    }
+      CurVNode->Count = CountValue;
+    } else
+      MinCountVNode->Count -= CountValue;
+
     return;
   }
 
   CurVNode = allocateOneNode(PData, CounterIndex, TargetValue);
   if (!CurVNode)
     return;
   CurVNode->Value = TargetValue;
-  CurVNode->Count++;
+  CurVNode->Count += CountValue;
 
   uint32_t Success = 0;
   if (!ValueCounters[CounterIndex])
     Success =
         COMPILER_RT_BOOL_CMPXCHG(&ValueCounters[CounterIndex], 0, CurVNode);
   else if (PrevVNode && !PrevVNode->Next)
     Success = COMPILER_RT_BOOL_CMPXCHG(&(PrevVNode->Next), 0, CurVNode);
 
   if (!Success && !hasStaticCounters) {
     free(CurVNode);
     return;
   }
 }
 
+COMPILER_RT_VISIBILITY void
+__llvm_profile_instrument_target(uint64_t TargetValue, void *Data,
+                                 uint32_t CounterIndex) {
+  instrumentTargetValueImpl(TargetValue, Data, CounterIndex, 1);
+}
+COMPILER_RT_VISIBILITY void
+__llvm_profile_instrument_target_value(uint64_t TargetValue, void *Data,
+                                       uint32_t CounterIndex,
+                                       uint64_t CountValue) {
+  instrumentTargetValueImpl(TargetValue, Data, CounterIndex, CountValue);
+}
+
 /*
  * The target values are partitioned into multiple regions/ranges. There is one
  * contiguous region which is precise -- every value in the range is tracked
  * individually. A value outside the precise region will be collapsed into one
  * value depending on the region it falls in.
  *
  * There are three regions:
  * 1. (-inf, PreciseRangeStart) and (PreciseRangeLast, LargeRangeValue) belong

compiler-rt/trunk/test/profile/instrprof-value-merge.c

+// RUN: %clang_pgogen -o %t -O3 %s
+// RUN: rm -rf %t.profdir
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t 1
+// RUN: env LLVM_PROFILE_FILE=%t.profdir/default_%m.profraw %run %t 1
+// RUN: llvm-profdata show -counts -function=main -ic-targets -memop-sizes %t.profdir/default_*.profraw | FileCheck %s
+
+#include <string.h>
+
+void (*f0)();
+void (*f1)();
+void (*f2)();
+
+char dst[200];
+char src[200];
+volatile int n;
+
+__attribute__((noinline)) void foo() {}
+
+__attribute__((noinline)) void bar() {
+  f0 = foo;
+  f1 = foo;
+  f2 = foo;
+  n = 4;
+}
+int main(int argc, char *argv[]) {
+  int i;
+  bar();
+  if (argc == 1) {
+    f0();
+    for (i = 0; i < 9; i++)
+      f1();
+    for (i = 0; i < 99; i++)
+      f2();
+  } else {
+    memcpy((void *)dst, (void *)src, n);
+    for (i = 0; i < 6; i++)
+      memcpy((void *)(dst + 2), (void *)src, n + 1);
+    for (i = 0; i < 66; i++)
+      memcpy((void *)(dst + 9), (void *)src, n + 2);
+  }
+}
+
+// CHECK: Counters:
+// CHECK:   main:
+// CHECK:     Hash: 0x00030012a7ab6e87
+// CHECK:     Counters: 6
+// CHECK:     Indirect Call Site Count: 3
+// CHECK:     Number of Memory Intrinsics Calls: 3
+// CHECK:     Block counts: [27, 297, 12, 132, 3, 2]
+// CHECK:     Indirect Target Results:
+// CHECK:         [ 0, foo, 3 ]
+// CHECK:         [ 1, foo, 27 ]
+// CHECK:         [ 2, foo, 297 ]
+// CHECK:     Memory Intrinsic Size Results:
+// CHECK:         [ 0, 4, 2 ]
+// CHECK:         [ 1, 5, 12 ]
+// CHECK:         [ 2, 6, 132 ]
+// CHECK: Instrumentation level: IR
+// CHECK: Functions shown: 1
+// CHECK: Total functions: 3
+// CHECK: Maximum function count: 327
+// CHECK: Maximum internal block count: 297
+// CHECK: Statistics for indirect call sites profile:
+// CHECK:   Total number of sites: 3
+// CHECK:   Total number of sites with values: 3
+// CHECK:   Total number of profiled values: 3
+// CHECK:   Value sites histogram:
+// CHECK:         NumTargets, SiteCount
+// CHECK:         1, 3
+// CHECK: Statistics for memory intrinsic calls sizes profile:
+// CHECK:   Total number of sites: 3
+// CHECK:   Total number of sites with values: 3
+// CHECK:   Total number of profiled values: 3
+// CHECK:   Value sites histogram:
+// CHECK:         NumTargets, SiteCount
+// CHECK:         1, 3