[PATCH] D9009: Value profiling compiler-rt changes

[Xinliang David Li via llvm-commits]

I read previous discussions on VP and buffer API. The buffer API is used in
a constraint environment, for instance when libc is not available. In that
environment, VP can not be enabled anyway. Because of this, I am ok leaving
the implementation as it is in your current patch -- but please do add a
comment in there about missing VP data for buffer API.

thanks,

David


On Wed, Nov 11, 2015 at 8:28 PM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Wed, Nov 11, 2015 at 6:51 PM, Betul Buyukkurt <betulb at codeaurora.org>
> wrote:
>
>> > davidxl added inline comments.
>> >
>> > ================
>> > Comment at: lib/profile/InstrProfiling.c:175
>> > @@ +174,3 @@
>> > +
>> > +    for (uint32_t i = 0; i < NumVSites; ++i) {
>> > +
>> > ----------------
>> > Looks like the structure is very close to ValueProfData structure used
>> in
>> > indexed format (except that the later is organized per-value kind, while
>> > here it is combined).  It will get all the interfaces for reading etc.
>>
>> So, is this another file format change cycle? We've already changed the
>> format three times during the life time of this CL.
>>
>>
> The two formats are basically the same, so there is no much change
> essentially. See my comment in another patch, it is more about code sharing
> and reuse. Having said that, I am fine with changing this as a follow up.
>
> > ================
>> > Comment at: lib/profile/InstrProfilingBuffer.c:45
>> > @@ -41,3 +44,3 @@
>> >        PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) +
>> > -      NamesSize + Padding;
>> > +      NamesSize + Padding2;
>> >  }
>> > ----------------
>> > Value Profile Data size is not counted here.
>>
>> We do not make use of these API's, thus we first need input from the
>> community (specifically the users of these API's) to change them.
>> Currently, VP is just not supported w/ the Buffer API's. In case buffer
>> API's are used to retrieve profile data during an instrumented executable
>> continues it's execution (jit like scenarios?), then updating the
>> ValueCounters member of __llvm_profile_data would possibly break the
>> profiler.
>>
>>
> This does not sound right -- buffer API and file API now behaves
> differently.  The right way to do is:
>
> 1) introduce a lock for dumping. When the dumping lock is help, the
> profiler will simply return without updating
> 2) when dumping is finished, the valueCounters can be cleared to be null
> (and edge profile count needs to be cleared too) and the lock can be
> released.
>
> David
>
>
>> -Betul
>>
>> >
>> > ================
>> > Comment at: lib/profile/InstrProfilingBuffer.c:110
>> > @@ -101,2 +109,3 @@
>> > +  UPDATE_memcpy(Zeroes,        Padding2      * sizeof(char));
>> >  #undef UPDATE_memcpy
>> >
>> > ----------------
>> > Value profile data is missing here.
>> >
>> >
>> > http://reviews.llvm.org/D9009
>> >
>> >
>> >
>> >
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151111/5b4ea313/attachment.html>