[PATCH] D28964: [PGO] Value profile support for value ranges

[Rong Xu via llvm-commits]

On Tue, Jan 24, 2017 at 11:38 AM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Tue, Jan 24, 2017 at 11:20 AM, Rong Xu <xur at google.com> wrote:
>
>>
>>
>> On Tue, Jan 24, 2017 at 11:15 AM, Xinliang David Li <davidxl at google.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Jan 24, 2017 at 11:13 AM, Rong Xu <xur at google.com> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Jan 24, 2017 at 11:02 AM, Xinliang David Li <davidxl at google.com
>>>> > wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, Jan 24, 2017 at 10:57 AM, Rong Xu <xur at google.com> wrote:
>>>>>
>>>>>> Yes. that's another way to do this. I considered that when I
>>>>>> implemented the code. I choose not do because:
>>>>>> (1) it makes it the instrumentation code complex and increase the
>>>>>> code size as basically you inline the logic to every instrumentation site.
>>>>>>
>>>>>
>>>>> We can filter a lot of memcpy sites with constant sizes, so the number
>>>>> of sites remaining should not be large (compared to virtual calls).
>>>>>
>>>> in real applications, the static call site for memcpy and memset is not
>>>> small.
>>>>
>>>>>
>>>>>
>>>>>> (2) it affects the static counter allocation. This range profile
>>>>>> potentially uses a lot more counters that current indirectly calls.
>>>>>>
>>>>>
>>>>> Why is that?
>>>>>
>>>> we collect the precise size for small sized memcpy and memset. Most of
>>>> the callsite has multiple size values.
>>>>
>>>
>>> My question is how is this affected by the way it is implemented: 1)
>>> using API call ; 2) generating inline sequence of interval profiling.
>>>
>>
>> using API, I have the exact count of the range instrumentations (and the
>> range) so I can adjust the counter allocation in lowering (different from
>> indirect calls)
>> On the other hand, if we do the inline sequence in instrumentation, we
>> treat them the same as current indirect calls instrumentation.
>>
>>
>
> Did I miss anything? I do not see any special handling of static counter
> allocation in the patches. The default counters per-site is still unchanged?
>

in InstrProfiling.cpp: function
void InstrProfiling::emitVNodes()
TotalNS is adjusted based on value profile kind.

>
> David
>
>
>
>>
>>> Anyway, see my previous reply -- I think having a minimal wrapper API in
>>> compiler rt is fine.
>>>
>>> David
>>>
>>>
>>>>
>>>>> David
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> The advantage of not change compiler_rt is that less information is
>>>>>> needed to pass b/w runtime and instrumentation. This especially true is we
>>>>>> want to have profile client to control it's own ranges.
>>>>>>
>>>>>> But I still think the change in compiler_rt is straight-forward way.
>>>>>>
>>>>>> On Tue, Jan 24, 2017 at 10:33 AM, David Li via Phabricator <
>>>>>> reviews at reviews.llvm.org> wrote:
>>>>>>
>>>>>>> davidxl added a comment.
>>>>>>>
>>>>>>> In fact, I don't there is a need for compiler_rt change at all for
>>>>>>> the interval/range profiling. The compiler_rt API should be kept simple.
>>>>>>> In other words, interval profiling should already be 'lowered' into regular
>>>>>>> value profiling in the instrumentation lowering phase. The instrumentation
>>>>>>> lowering will general inline sequence that collapse ranges into single
>>>>>>> value depending on the value profile kind.  Only the
>>>>>>> __llvm_profile_instrument_target low level interface is needed.
>>>>>>>
>>>>>>> The only downside of this approach that we can not override the
>>>>>>> range definitions at runtime -- but I don't think that is worth the
>>>>>>> complexity introduced.
>>>>>>>
>>>>>>>
>>>>>>> Repository:
>>>>>>>   rL LLVM
>>>>>>>
>>>>>>> https://reviews.llvm.org/D28964
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170124/e7c5e180/attachment.html>