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

[Xinliang David Li via llvm-commits]

I feel that there might be some misunderstanding of 'weight' meta data
here.   A clarification: the absolute value of 'weight' does not matter
much -- the weight value won't be used as 'frequency' or profile count. The
only interpretation of the weight meta data is by forming the branch
probability out of it.

In other words, if BFI does not have the limitation of handling '0' weight,
it makes no difference annotating a branch with (1, 0) weights vs (1<<31,
0) --- as the resulting branch probability will be the same 100%.

My opinion is that we should make builtin_expect produce more biased BP
than what we have today, but do not go extreme to consider other potential
use cases. If user really wants extreme setting, he can also use the
internal option to override it.

David

On Tue, Apr 26, 2016 at 11:11 AM, 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/e148a0f6/attachment.html>