<div dir="ltr"><div>David's statement about my assumption is correct: I didn't think that programmers would use the builtin as a replacement for profile data, but only in extreme cases - effectively to mark the equivalent of an 'assert'. That's how I've used it as an application programmer, and that was always my interpretation of the GCC docs.<br><br>But the GCC docs are vague, so either we have to account for more liberal usage of the builtin or write our own docs.<br><br></div>I'll update the patch...<br><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 26, 2016 at 12:11 PM, Amaury SECHET <span dir="ltr"><<a href="mailto:deadalnix@gmail.com" target="_blank">deadalnix@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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.<br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">2016-04-26 10:46 GMT-07:00 Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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></span><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><span><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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></span><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><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">
Anyone else want to register a vote on this?<br></div></blockquote><div><br></div></span><div>Let's discuss this with reasoning, not voting :)</div><span><font color="#888888"><div><br></div><div>David</div></font></span><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br></div><div><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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div></div>