<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 15, 2015 at 11:08 AM, Betul Buyukkurt <span dir="ltr"><<a href="mailto:betulb@codeaurora.org" target="_blank">betulb@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> On Wed, Oct 14, 2015 at 5:49 PM, Betul Buyukkurt <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>><br>
> wrote:<br>
>>> davidxl added inline comments.<br>
>>><br>
>>> ================<br>
>>> Comment at: include/llvm/ProfileData/InstrProfReader.h:144<br>
>>> @@ +143,3 @@<br>
>>> +    const IntPtrT FunctionPointer;<br>
>>> +    const uint32_t NumValueSites[IPVK_Last+1];<br>
>>> +    const IntPtrT Values;<br>
>>> ----------------<br>
>>> Make NumValueSites[..] the last field of ProfileData.  The number of<br>
>>> value<br>
>>> profile kind also need to be recorded in the file header. This is to<br>
>>> avoid<br>
>>> format version bump each time a new value profile kind is added.<br>
>>><br>
>><br>
>> I've added in my latest patch, a field to the raw header to record the<br>
>> last element of the value kind enum at run-time. However, I think the<br>
>> field ordering change will not help in avoiding a format version change.<br>
>><br>
><br>
> Why not?  ProfileData now becomes a variable length record. The reader<br>
> should be able to handle this without breaking format compatibility.<br>
<br>
</span>We may put the NumValueSites array as the last element but keeping it<br>
variable length means use of flexible arrays and that's not supported in<br>
C++. </blockquote><div><br></div><div>A common way of doing so is to make the array of length 1.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">How do you want to achieve keeping the ProfileData pointer in the<br>
RawInstrProfReader iterable (i.e. in readNextRecord)? Not possible as<br>
it's. We'd have to be removing NumValueSites from the ProfileData struct,<br></blockquote><div><br></div><div>no -- you can keep NumValueSites there.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
and casting a uint8_t* Data pointer to ProfileData followed by to<br>
NumValueSites[] is the solution. Is this how you'd like to proceed?<br></blockquote><div><br></div><div>Instead of doing Data++,  cast it do uint8_t * and advance with the actual number of bytes (using the number of ValueKinds recorded).</div><div><br></div><div>David</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Betul<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> David<br>
><br>
><br>
>> Justin, do you have any comments on the patch?<br>
>><br>
>> -Betul<br>
>><br>
>>> <a href="http://reviews.llvm.org/D13308" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13308</a><br>
>>><br>
>>><br>
>>><br>
>>><br>
>><br>
>><br>
><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>