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

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