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

[Xinliang David Li via llvm-commits]

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/34bebe0d/attachment.html>