<div dir="ltr">[repeating message as email because Phab comment doesn't seem to have come through]<br><br><p>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 <a href="http://reviews.llvm.org/D19299" class=""><span class="">D19299</span></a>.</p>

<p>Since BFI is already handling its bug internally, we don't need to propagate knowledge of that bug up here.</p>

Anyone else want to register a vote on this?<br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 26, 2016 at 10:22 AM, David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davidxl added inline comments.<br>
<span class=""><br>
================<br>
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:42<br>
@@ -43,1 +41,3 @@<br>
</span><span class="">+    "unlikely-branch-weight", cl::Hidden, cl::init(0),<br>
+    cl::desc("Weight of the branch unlikely to be taken (default = 0)"));<br>
<br>
</span>----------------<br>
<span class="">spatel wrote:<br>
> davidxl wrote:<br>
> > I suggest not using 0 weight which BFI can not handle well.  1 should be better.<br>
> 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.<br>
><br>
> 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.<br>
</span>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.<br>
<br>
The BFI with 0 weight problem is not a simple bug -- it is due to the limitation of the propagation algorithm. See BlockFrequencyInfoImplBase::adddToDist.<br>
<br>
Due to that, when computing BFI, 0 weight is changed to 1 anyway, so it might be better to make it an explicit 1.<br>
<br>
<br>
<a href="http://reviews.llvm.org/D19435" rel="noreferrer" target="_blank">http://reviews.llvm.org/D19435</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>