[PATCH] D20408: [profile] entry eviction support in value profiler

[Vedant Kumar via llvm-commits]

vsk added a comment.

This looks good overall. I have a few nits and one corner case I'm concerned about.


================
Comment at: lib/profile/InstrProfilingValue.c:102
@@ -96,2 +101,3 @@
   ValueProfNode *CurrentVNode = ValueCounters[CounterIndex];
+  uint64_t MinCount = (uint64_t)(-1);
 
----------------
nit, UINT64_MAX?

================
Comment at: lib/profile/InstrProfilingValue.c:108
@@ -101,3 +107,3 @@
       CurrentVNode->Count++;
       return;
     }
----------------
If `CurrentVNode == MinCountVNode` before this increment, should we check if `MinCountVNode` needs to be replaced before we return? It's not a common case, but I'm concerned about doing `--MinCountVNode->Count` on the wrong node in subsequent calls.

================
Comment at: lib/profile/InstrProfilingValue.c:111
@@ +110,3 @@
+    if (CurrentVNode->Count < MinCount) {
+        MinCount = CurrentVNode->Count;
+        MinCountVNode = CurrentVNode;
----------------
nit, clang-format

================
Comment at: lib/profile/InstrProfilingValue.c:120
@@ +119,3 @@
+  if (VDataCount >= VPMaxNumValsPerSite) {
+    /* Bump down the min count node's value. If it reaches 0,
+     * evict it. This eviction/replacement policy makes hot
----------------
nit, "min count node's count"?

================
Comment at: test/profile/Inputs/instrprof-value-prof-evict.c:49
@@ +48,3 @@
+
+#define INDIRECT_CALLSITE(Sequence, NumValsTracked)                            \
+  lprofSetMaxValsPerSite(NumValsTracked);                                      \
----------------
nit, could you pull this macro into main()? That makes it a bit clearer where the `I` comes from.

================
Comment at: test/profile/Inputs/instrprof-value-prof-real.c:312
@@ -311,3 +311,3 @@
 // CHECK-NEXT:	[ 0, foo_1_2_2_2_2_2_2_1_2, 747 ]
-// CHECK-NEXT:	[ 0, foo_1_2_2_2_2_2_2_2_1, 746 ]
+// CHECK-NEXT:	[ 0, foo
 // CHECK-NEXT:	[ 1, foo_2_2_2_2_2_2_2_2_2, 2000 ]
----------------
I don't follow why `[0, foo_..., N]` changes to `[0, foo`. Could you explain why this happens?


http://reviews.llvm.org/D20408