[Profile] Fix bad interaction of profile counter reset and value profile eviction

[davidxl]

Summary

The problem is that value profile evict-or assumes value profile counts are non zero. The assumption is wrong once the counters are cleared (for value profile data, the counts are reset, but the target entries are still kept). When counter resetting and eviction is combined, the zero count entries will end up with large profile count due to wrapping. When all entries are wrapped, adding new target will also lead to seg fault.

[vsk]

This makes sense. Would you be opposed to combining the C file and the test file? I think it'd improve readability to have the run lines and the check lines in the same place.

test/profile/Inputs/instrprof-value-prof-reset.c

27

Since max-vals-per-site = 2, I think we'd also want CHECK-NOT: callee_0?

27

nit, could you fix the capitalization here?

[davidxl]

Updated the test case according to Vedant's feedback.

[vsk]

LGTM.

I don't see any issue with having the whole test live in test/profile/*.c, and would prefer that, but it's no big deal.

[xur]

lgtm.

lib/profile/InstrProfilingValue.c

      * 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)) {
+    if (!MinCountVNode->Count || !(--MinCountVNode->Count)) {
       CurVNode = MinCountVNode;
       CurVNode->Value = TargetValue;
       CurVNode->Count++;
     }
     return;
   }
 
   CurVNode = allocateOneNode(PData, CounterIndex, TargetValue);

test/profile/Inputs/instrprof-value-prof-reset.c

+void callee_0() {}
+void callee_1() {}
+void callee_2() {}
+
+void *CalleeAddrs[] = {callee_0, callee_1, callee_2, callee_2, callee_2};
+extern void lprofSetMaxValsPerSite(unsigned);
+extern void __llvm_profile_reset_counters();
+
+typedef void (*FPT)(void);
+
+
+// Testing value profiling eviction algorithm.
+FPT getCalleeFunc(int I) { return CalleeAddrs[I]; }
+
+int main() {
+  int I;
+
+  // First fill up two value profile entries with two targets
+  lprofSetMaxValsPerSite(2);
+
+  for (I = 0; I < 5; I++) {
+    if (I == 2) {
+      __llvm_profile_reset_counters();
+    }
+    // CHECK:  callee_2, 3
+    // CHECK-NEXT: callee_1, 0
+    // CHECK-NOT: callee_0,
+    FPT FP = getCalleeFunc(I);
+    FP();
+  }
+}

test/profile/instrprof-value-prof-reset.test

+// RUN: %clang_profgen -O2 -mllvm -enable-value-profiling=true -mllvm -vp-static-alloc=true -mllvm -vp-counters-per-site=3 -o %t %S/Inputs/instrprof-value-prof-reset.c
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t
+// RUN: llvm-profdata merge -o %t.profdata %t.profraw
+// RUN: llvm-profdata show --all-functions -ic-targets  %t.profdata | FileCheck  %S/Inputs/instrprof-value-prof-reset.c
+
+// IR level instrumentation
+// RUN: %clang_pgogen -O2 -mllvm -disable-vp=false -mllvm -vp-static-alloc=true  -mllvm -vp-counters-per-site=3 -o %t.ir  %S/Inputs/instrprof-value-prof-reset.c
+// RUN: env LLVM_PROFILE_FILE=%t.ir.profraw %run %t.ir
+// RUN: llvm-profdata merge -o %t.ir.profdata %t.ir.profraw
+// RUN: llvm-profdata show --all-functions -ic-targets  %t.ir.profdata | FileCheck  %S/Inputs/instrprof-value-prof-reset.c
+
+// IR level instrumentation, dynamic allocation
+// RUN: %clang_pgogen -O2 -mllvm -disable-vp=false -mllvm -vp-static-alloc=false -o %t.ir.dyn  %S/Inputs/instrprof-value-prof-reset.c
+// RUN: env LLVM_PROFILE_FILE=%t.ir.dyn.profraw %run %t.ir.dyn
+// RUN: llvm-profdata merge -o %t.ir.dyn.profdata %t.ir.dyn.profraw
+// RUN: llvm-profdata show --all-functions -ic-targets  %t.ir.dyn.profdata | FileCheck  %S/Inputs/instrprof-value-prof-reset.c