[PATCH] D13308: Value profiling - remaining LLVM changes

[Xinliang David Li via llvm-commits]

On Thu, Oct 15, 2015 at 11:08 AM, Betul Buyukkurt <betulb at codeaurora.org>
wrote:

> > On Wed, Oct 14, 2015 at 5:49 PM, Betul Buyukkurt <betulb at codeaurora.org>
> > wrote:
> >>> davidxl added inline comments.
> >>>
> >>> ================
> >>> Comment at: include/llvm/ProfileData/InstrProfReader.h:144
> >>> @@ +143,3 @@
> >>> +    const IntPtrT FunctionPointer;
> >>> +    const uint32_t NumValueSites[IPVK_Last+1];
> >>> +    const IntPtrT Values;
> >>> ----------------
> >>> Make NumValueSites[..] the last field of ProfileData.  The number of
> >>> value
> >>> profile kind also need to be recorded in the file header. This is to
> >>> avoid
> >>> format version bump each time a new value profile kind is added.
> >>>
> >>
> >> I've added in my latest patch, a field to the raw header to record the
> >> last element of the value kind enum at run-time. However, I think the
> >> field ordering change will not help in avoiding a format version change.
> >>
> >
> > Why not?  ProfileData now becomes a variable length record. The reader
> > should be able to handle this without breaking format compatibility.
>
> We may put the NumValueSites array as the last element but keeping it
> variable length means use of flexible arrays and that's not supported in
> C++.


A common way of doing so is to make the array of length 1.


> How do you want to achieve keeping the ProfileData pointer in the
> RawInstrProfReader iterable (i.e. in readNextRecord)? Not possible as
> it's. We'd have to be removing NumValueSites from the ProfileData struct,
>

no -- you can keep NumValueSites there.


> and casting a uint8_t* Data pointer to ProfileData followed by to
> NumValueSites[] is the solution. Is this how you'd like to proceed?
>

Instead of doing Data++,  cast it do uint8_t * and advance with the actual
number of bytes (using the number of ValueKinds recorded).

David

>
> -Betul
>
> > David
> >
> >
> >> Justin, do you have any comments on the patch?
> >>
> >> -Betul
> >>
> >>> http://reviews.llvm.org/D13308
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151015/25bda53c/attachment.html>