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

[Sanjay Patel via llvm-commits]

[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>.

Since BFI is already handling its bug internally, we don't need to
propagate knowledge of that bug up here.
Anyone else want to register a vote on this?


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/81ca317e/attachment.html>