[PATCH] D15057: [PGO] Enable common VP format in profile runtime

[Betul Buyukkurt via llvm-commits]

betulb added a comment.

One side comment on the commit message: Using TotalValueDataSize would only cause a drop of the last N function(s)' value data if/when file writing is to be triggered through means other than atexit() and calls to __llvm_profile_instrument_target are allowed to execute concurrently w/ file write. Neither is happening at this time.


================
Comment at: lib/profile/InstrProfiling.c:70
@@ +69,3 @@
+                                   uint32_t ValueKind, uint16_t NumValueSites) {
+  *((uint16_t *)&Data->NumValueSites[ValueKind]) = NumValueSites;
+}
----------------
Is the casting really needed? What's passed in is a __llvm_profile_data pointer, so 
shouldn't Data->NumValueSites[ValueKind] be OK?

================
Comment at: lib/profile/InstrProfiling.c:188
@@ +187,3 @@
+  if (!Records)
+    goto Return;
+  /*
----------------
Nothing to free. So, just "return 0;" ?

================
Comment at: lib/profile/InstrProfiling.c:224
@@ +223,3 @@
+    if (!I->Values)
+      continue;
+    ValueProfRuntimeRecord *R = &Records[I - DataBegin];
----------------
Shouldn't line 223 check for !getValueProfDataSizeRT(R) and not for !I->Values? The new check also should happen after line 225.


http://reviews.llvm.org/D15057