<div dir="ltr">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.<div><br></div><div>thanks,</div><div><br></div><div>David</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 11, 2015 at 8:28 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Nov 11, 2015 at 6:51 PM, 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>> davidxl added inline comments.<br>
><br>
> ================<br>
> Comment at: lib/profile/InstrProfiling.c:175<br>
> @@ +174,3 @@<br>
> +<br>
> +    for (uint32_t i = 0; i < NumVSites; ++i) {<br>
> +<br>
> ----------------<br>
> Looks like the structure is very close to ValueProfData structure used in<br>
> indexed format (except that the later is organized per-value kind, while<br>
> here it is combined).  It will get all the interfaces for reading etc.<br>
<br>
</span>So, is this another file format change cycle? We've already changed the<br>
format three times during the life time of this CL.<br>
<span><br></span></blockquote><div><br></div></span><div>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.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>
> ================<br>
> Comment at: lib/profile/InstrProfilingBuffer.c:45<br>
> @@ -41,3 +44,3 @@<br>
>        PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) +<br>
> -      NamesSize + Padding;<br>
> +      NamesSize + Padding2;<br>
>  }<br>
> ----------------<br>
> Value Profile Data size is not counted here.<br>
<br>
</span>We do not make use of these API's, thus we first need input from the<br>
community (specifically the users of these API's) to change them.<br>
Currently, VP is just not supported w/ the Buffer API's. In case buffer<br>
API's are used to retrieve profile data during an instrumented executable<br>
continues it's execution (jit like scenarios?), then updating the<br>
ValueCounters member of __llvm_profile_data would possibly break the<br>
profiler.<br>
<span><font color="#888888"><br></font></span></blockquote><div><br></div></span><div>This does not sound right -- buffer API and file API now behaves differently.  The right way to do is:</div><div><br></div><div>1) introduce a lock for dumping. When the dumping lock is help, the profiler will simply return without updating</div><div>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.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>David</div></font></span><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><font color="#888888">
-Betul<br>
</font></span><div><div><br>
><br>
> ================<br>
> Comment at: lib/profile/InstrProfilingBuffer.c:110<br>
> @@ -101,2 +109,3 @@<br>
> +  UPDATE_memcpy(Zeroes,        Padding2      * sizeof(char));<br>
>  #undef UPDATE_memcpy<br>
><br>
> ----------------<br>
> Value profile data is missing here.<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D9009" rel="noreferrer" target="_blank">http://reviews.llvm.org/D9009</a><br>
><br>
><br>
><br>
><br>
<br>
<br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div>