[PATCH] D19435: [LowerExpectIntrinsic] pin default likely/unlikely weights to min/max values

[Sanjay Patel via llvm-commits]

David's statement about my assumption is correct: I didn't think that
programmers would use the builtin as a replacement for profile data, but
only in extreme cases - effectively to mark the equivalent of an 'assert'.
That's how I've used it as an application programmer, and that was always
my interpretation of the GCC docs.

But the GCC docs are vague, so either we have to account for more liberal
usage of the builtin or write our own docs.

I'll update the patch...

On Tue, Apr 26, 2016 at 12:11 PM, Amaury SECHET <deadalnix at gmail.com> wrote:

> I think it is reasonable to interpret expect as "make this the fast path".
> For what it is worth, I've been using 65536/0 in D front end for a while
> with good results. The 64/4 was definitively too "mild" to get very
> unlikely branches out of the way, which lead to the customization.
>
> 2016-04-26 10:46 GMT-07:00 Xinliang David Li <davidxl at google.com>:
>
>>
>>
>> On Tue, Apr 26, 2016 at 10:04 AM, Sanjay Patel <spatel at rotateright.com>
>> wrote:
>>
>>> [repeating message as email because Phab comment doesn't seem to have
>>> come through]
>>>
>>> The problem I foresee - known unknown? :) - is that some heuristic-based
>>> transform will not trigger because we chose unwisely here. Why be imperfect
>>> when we can be perfect? It's the same argument/implementation that we said
>>> was correct in D19299 <http://reviews.llvm.org/D19299>.
>>>
>> You are making assumption that all users of __builtin_expect assume
>> 'extreme' branch probabilities, which I think is not the case. I am pretty
>> sure people will happy to apply the builtin even when the BP is 80% (e.g,
>> just for better layout).
>>
>>
>>
>>> Since BFI is already handling its bug internally, we don't need to
>>> propagate knowledge of that bug up here.
>>>
>>
>> It is not just about BFI's issue but with making builtin_expect too
>> extreme.  Also with current BFI, you essentially get weight of 1, but if
>> BFI is fixed in someway in the future, the behavior of builtin_expect will
>> also change silently (also to the extreme 0% prob which does not sound
>> right).  You need to consider other use cases of builtin_expect (see above).
>>
>>
>>> Anyone else want to register a vote on this?
>>>
>>
>> Let's discuss this with reasoning, not voting :)
>>
>> David
>>
>>
>>>
>>>
>>> On Tue, Apr 26, 2016 at 10:22 AM, David Li <davidxl at google.com> wrote:
>>>
>>>> davidxl added inline comments.
>>>>
>>>> ================
>>>> Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:42
>>>> @@ -43,1 +41,3 @@
>>>> +    "unlikely-branch-weight", cl::Hidden, cl::init(0),
>>>> +    cl::desc("Weight of the branch unlikely to be taken (default =
>>>> 0)"));
>>>>
>>>> ----------------
>>>> spatel wrote:
>>>> > davidxl wrote:
>>>> > > I suggest not using 0 weight which BFI can not handle well.  1
>>>> should be better.
>>>> > I thought this over, and I really don't want to compromise on either
>>>> value here. The meaning of builtin_expect() is clear, and we should honor
>>>> that programmer directive. Picking semi-random values from the air can only
>>>> lead to unseen problems down the road.
>>>> >
>>>> > Can you explain why/where BFI has a problem with a '0' weight? If
>>>> it's a simple bug, I will try to fix that before this patch.
>>>> I am not sure what problem you can foresee down the road with 99.95%
>>>> probability proposed.  2000 is actually not something coming out of thin
>>>> air -- GCC also uses this -- not that it is perfect, but I would guess lots
>>>> of apps are tuned based on that setting.
>>>>
>>>> The BFI with 0 weight problem is not a simple bug -- it is due to the
>>>> limitation of the propagation algorithm. See
>>>> BlockFrequencyInfoImplBase::adddToDist.
>>>>
>>>> Due to that, when computing BFI, 0 weight is changed to 1 anyway, so it
>>>> might be better to make it an explicit 1.
>>>>
>>>>
>>>> http://reviews.llvm.org/D19435
>>>>
>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160426/534e2f5a/attachment.html>