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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/profile/InstrProfiling.c:224
@@ +223,3 @@
+    if (!I->Values)
+      continue;
+    ValueProfRuntimeRecord *R = &Records[I - DataBegin];
----------------
betulb wrote:
> davidxl wrote:
> > betulb wrote:
> > > Shouldn't line 223 check for !getValueProfDataSizeRT(R) and not for !I->Values? The new check also should happen after line 225.
> > You are right. I->Values is lazily initialized -- so I->Values == NULL does not mean we can skip it (only when there is zero value site statically). I need to enhance the test case to cover it.
> I'm not sure if I'm following your concern about lazy initialization for I->Values.  
> The earlier loop initializes the ValueProfRuntimeRecord. 
> 
> It did seemed to me that you're concerned about value data being dropped at file write time. If there is any scenario which can cause dropping of the value data in the previous implementation, then this new implementation could cause a buffer overwrite under the same circumstances, if a NULL entry for I->Values gets updated after the null check on line 194 and before line 223. But, I do not see any allowance for that at the moment.
No -- I don't have a concern about I->Values being lazily initialized -- it is perfectly fine to do it -- just the writer code needs to handle it properly -- and your suggestion of not checking !I->Values is correct (also avoid the overwrite problem you mentioned here).

The value dropping problem as you mentioned can happen for the last 'N' function -- the main problem is that it can potentially lead to hot values gets truncated.




http://reviews.llvm.org/D15057