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

Betul Buyukkurt via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 2 07:08:30 PST 2015


betulb added a subscriber: betulb.
betulb added a comment.

> 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?


I see. It's needed to remove the const qualifier.

> ================

>  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



http://reviews.llvm.org/D15057





More information about the llvm-commits mailing list