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

[Amaury SECHET via llvm-commits]

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/5f8d7f7e/attachment.html>