<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 26, 2016 at 10:04 AM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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" target="_blank"><span>D19299</span></a>.</p></div></blockquote><div>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).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">

<p>Since BFI is already handling its bug internally, we don't need to propagate knowledge of that bug up here.</p></div></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">

Anyone else want to register a vote on this?<br></div></blockquote><div><br></div><div>Let's discuss this with reasoning, not voting :)</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br></div><div class="HOEnZb"><div class="h5"><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><br>
================<br>
Comment at: lib/Transforms/Scalar/LowerExpectIntrinsic.cpp:42<br>
@@ -43,1 +41,3 @@<br>
</span><span>+    "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>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>
</div></div></blockquote></div><br></div></div>