<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 1, 2015 at 9:22 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="">> Hi Betul,<br>
><br>
> I've finally gotten around to going over this in detail - sorry for the<br>
> delay, and thanks for working on this.<br>
><br>
> I think that the general approach is a good one, and that this will end<br>
> up working well. I have a couple of points on a high level, and I'll<br>
> also send some review for the patches you've sent out.<br>
><br>
> - The raw profile format does not need to be backwards compatible,<br>
>   that's only important for the indexed one. Instead of adding<br>
>   RawInstrValueProfReader, just change RawInstrProfReader and reject the<br>
>   data if the version is 1. Similarly, don't change the raw profile<br>
>   magic - just bump the version to 2.<br>
<br>
</span>Thanks. Not remaining backwards compatible in the RawInstrProfReader<br>
removed much of the added complexity from the implementation. I'll upload<br>
my changes soon to the phabricator later today.<br>
<span class=""><br>
> - We don't need to store the value profiling kind in the data at all.<br>
>   The frontend knows which invocations of the intrinsic are for each kind<br>
>   implicitly, much like it knows the difference between a counter for an<br>
>   "if" and a "for" apart implicitly. Just store one set of profiling data<br>
>   and let the frontend sort it out.<br>
<br>
</span>I think so too. However, value_kind should stay around until the kinds of<br>
expressions whose values are computed are included in the hash<br>
computation. Otherwise, if a value-profiled expression is to be removed<br>
from source, then all the rest of the values would get attached to the MD<br>
fields of wrong types. Currently hash checking verifies if the counter<br>
assigned regions in the source has been changed across profile consumption<br>
runs.<br></blockquote><div><br></div><div>yes -- separating counter arrays from different value kind is still very useful, for instance profile-generate and profile-use compilations can turn on different set of value profile options and can still work.</div><div><br></div><div>However, by organizing the in-memory and persistent profile data structure properly, we don't need to record the value kind, nor counter index information in the value profile entries. See my most recent review comments.</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> - Given that, the instrprof libraries needn't know about kinds at all,<br>
>   that can be dealt with locally in clang's CodeGenPGO, where we need to<br>
>   know what to do with the data. I'd just drop the kind completely for<br>
>   now. It'll be easy to add later without affecting much.<br>
<br>
</span>I've not reflected this request in my latest changes that I'll upload<br>
later today. If we can come up with a mechanism to make sure the source<br>
changes on values profiled can be detected at function level, then I may<br>
bring in this change in my CLs.<br>
<span class=""><br>
> - We should be able to statically allocate the first entry for each<br>
>   callsite for __llvm_profile_value_data. It will always be there, and<br>
>   for call sites that always call the same target it's nice to avoid the<br>
>   allocation.<br>
<br>
</span>I'm not in favor of this as dynamic allocation seem to be doing fine.<br>
<span class=""><br>
> - The changes in the llvm and compiler-rt repos need their own tests.<br>
<br>
</span>I'll add them in.<br>
<span class=""><br>
> - Please use clang-format on your changes. Several of the patches have<br>
>   unusual or non-llvm-style whitespace/wrapping/etc.<br>
<br>
</span>I'll do this as well.<br>
<span class=""><br>
> Finally, the LLVM patch is quite large. We can probably make it a bit<br>
> easier to stage and review by splitting out a couple of things and doing<br>
> them first:<br>
><br>
> 1. Changing the __llvm_profile_data variables so they're no longer<br>
>    const.<br>
><br>
> 2. Adding the InstrProfRecord type and updating the unittests. Also,<br>
>    this type should just go in the InstrProf.h header, no need for its<br>
>    own.<br>
><br>
> 3. Arguably, we could update the raw format and implement the<br>
>    RawInstrProfReader changes (but ignoring the new data) first. If you<br>
>    think this is worthwhile go ahead, but if it'll lead to too much<br>
>    extra busywork it probably isn't worth it.<br>
><br>
> I'm sending some review on the patches themselves as well, but I expect<br>
> those to mostly be on the mechanical aspects since the high level points<br>
> are already written down here.<br>
<br>
</span>I'll first update current CL's responding to the review comments and then<br>
start uploading the CL's in the above recommended order.<br>
<br>
-Betul<br>
<div><div class="h5"><br>
> Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> writes:<br>
>> I have sent my review comments. I think most of my high level concerns<br>
>> have been addressed (except for last few minor fix ups).<br>
>><br>
>> Justin, do you have a chance to take a look?<br>
>><br>
>> thanks,<br>
>><br>
>> David<br>
>><br>
>> On Wed, May 13, 2015 at 10:49 AM, Betul Buyukkurt<br>
>> <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> wrote:<br>
>>>> Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> writes:<br>
>>>>>> From: <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>><br>
>>>>>> Date: Tue, Apr 7, 2015 at 12:44 PM<br>
>>>>>> Subject: [LLVMdev] IC profiling infrastructure<br>
>>>>>> To: <a href="mailto:llvmdev@cs.uiuc.edu">llvmdev@cs.uiuc.edu</a><br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> Hi All,<br>
>>>>>><br>
>>>>>> We had sent out an RFC in October on indirect call target profiling.<br>
>>>>>> The<br>
>>>>>> proposal was about profiling target addresses seen at indirect call<br>
>>>>>> sites.<br>
>>>>>> Using the profile data we're seeing up to %8 performance<br>
>>>>>> improvements<br>
>>>>>> on<br>
>>>>>> individual spec benchmarks where indirect call sites are present.<br>
>>>>>> We've<br>
>>>>>> already started uploading our patches to the phabricator. I'm<br>
>>>>>> looking<br>
>>>>>> forward to your reviews and comments on the code and ready to<br>
>>>>>> respond<br>
>>>>>> to<br>
>>>>>> your design related queries.<br>
>>>>>><br>
>>>>>> There were few questions posted on the RFC that were not responded.<br>
>>>>>> Here<br>
>>>>>> are the much delayed comments.<br>
>>>>>><br>
>>>>><br>
>>>>> Hi Betul, thank you for your patience.  I have completed initial<br>
>>>>> comparison with a few alternative value profile designs. My<br>
>>>>> conclusion<br>
>>>>> is that your proposed approach should well in practice. The study can<br>
>>>>> be found here:<br>
>>>>> <a href="https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.google.com_document_u_1_d_1k-2D-5Fk-5FDLFBh8h3XMnPAi6za-2DXpmjOIPHX-5Fx6UB6PULfw_pub&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=Mfk2qtn1LTDThVkh6-oGglNfMADXfJdty4_bhmuhMHA&m=X1YyGdEXhcbVu9VleZjmIQFqfEvAGl7bQB6FAwf2loc&s=lmQocq0Ltj3saguy6CzYwxhA1vT6RqaMdsbttyrfgrE&e=" target="_blank">https://docs.google.com/document/u/1/d/1k-_k_DLFBh8h3XMnPAi6za-XpmjOIPHX_x6UB6PULfw/pub</a><br>
>>>><br>
>>>> Thanks for looking at this David.<br>
>>>><br>
>>>> Betul: I also have some thoughts on the approach and implementation of<br>
>>>> this, but haven't had a chance to go over it in detail. I hope to have<br>
>>>> some feedback for you on all of this sometime next week, and I'll<br>
>>>> start<br>
>>>> reviewing the individual patches after that.<br>
>>><br>
>>> Hi All,<br>
>>><br>
>>> I've posted three more patches yesterday. They might be missing some<br>
>>> cosmetic fixes, but the support for profiling multiple value kinds have<br>
>>> been added to the readers, writers and runtime. I'd appreciate your<br>
>>> comments on the CL's.<br>
>>><br>
>>> Thanks,<br>
>>> -Betul<br>
>>><br>
>>>><br>
>>>>>> 1) Added dependencies: Our implementation adds dependency on<br>
>>>>>> calloc/free<br>
</div></div>>>>>>> as we’re generating/maintaining a linked list at run time.<br>
<div class="HOEnZb"><div class="h5">>>>>><br>
>>>>> If it becomes a problem for some, there is a way to handle that --<br>
>>>>> but<br>
>>>>> at a cost of more memory required (to be conservative). One of the<br>
>>>>> good feature of using dynamic memory is that it allows counter array<br>
>>>>> allocation on the fly which eliminates the need to allocate memory<br>
>>>>> for<br>
>>>>> lots of cold/unexecuted functions.<br>
>>>>><br>
>>>>>> We also added<br>
>>>>>> dependency on the usage of mutexes to prevent memory leaks in the<br>
>>>>>> case<br>
>>>>>> multiple threads trying to insert a new target address for the same<br>
>>>>>> IC<br>
>>>>>> site into the linked list. To least impact the performance we only<br>
>>>>>> added<br>
>>>>>> mutexes around the pointer assignment and kept any dynamic memory<br>
>>>>>> allocation/free operations outside of the mutexed code.<br>
>>>>><br>
>>>>> This (using mutexes) should be and can be avoided -- see the above<br>
>>>>> report.<br>
>>>>><br>
>>>>>><br>
>>>>>> 2) Indirect call data being present in sampling profile output: This<br>
>>>>>> is<br>
>>>>>> unfortunately not helping in our case due to perf depending on lbr<br>
>>>>>> support. To our knowledge lbr support is not present on ARM<br>
>>>>>> platforms.<br>
>>>>>><br>
>>>>><br>
>>>>> yes.<br>
>>>>><br>
>>>>>> 3) Losing profiling support on targets not supporting<br>
>>>>>> malloc/mutexes:<br>
>>>>>> The<br>
>>>>>> added dependency on calloc/free/mutexes may perhaps be eliminated<br>
>>>>>> (although our current solution does not handle this) through having<br>
>>>>>> a<br>
>>>>>> separate run time library for value profiling purposes.<br>
>>>>>> Instrumentation<br>
>>>>>> can link in two run time libraries when value profiling (an instance<br>
>>>>>> of<br>
>>>>>> it<br>
>>>>>> being indirect call target profiling) is enabled on the command<br>
>>>>>> line.<br>
>>>>><br>
>>>>> See above.<br>
>>>>><br>
>>>>>><br>
>>>>>> 4) Performance of the instrumented code: Instrumentation with IC<br>
>>>>>> profiling<br>
>>>>>> patches resulted in 7% degradation across spec benchmarks at -O2.<br>
>>>>>> For<br>
>>>>>> the<br>
>>>>>> benchmarks that did not have any IC sites, no performance<br>
>>>>>> degradation<br>
>>>>>> was<br>
>>>>>> observed. This data is gathered using the ref data set for spec.<br>
>>>>>><br>
>>>>><br>
>>>>> I'd like to make the runtime part of the change to be shared and used<br>
>>>>> as a general purpose value profiler (not just indirect call<br>
>>>>> promotion), but this can be done as a follow up.<br>
>>>>><br>
>>>>> I will start with some reviews. Hopefully others will help with<br>
>>>>> reviews<br>
>>>>> too.<br>
>>>>><br>
>>>>> thanks,<br>
>>>>><br>
>>>>> David<br>
>>>>><br>
>>>>><br>
>>>>><br>
>>>>>> Thanks,<br>
>>>>>> -Betul Buyukkurt<br>
>>>>>><br>
>>>>>> Qualcomm Innovation Center, Inc.<br>
>>>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a<br>
>>>>>> Linux<br>
>>>>>> Foundation Collaborative Project<br>
>>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> _______________________________________________<br>
>>>>>> LLVM Developers mailing list<br>
>>>>>> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>>>>>><br>
>>>>><br>
>>>>> _______________________________________________<br>
>>>>> LLVM Developers mailing list<br>
>>>>> <a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
>>>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
>>>><br>
>>><br>
>>><br>
><br>
<br>
<br>
<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
</div></div></blockquote></div><br></div></div>