<div dir="ltr">I feel that there might be some misunderstanding of 'weight' meta data here. A clarification: the absolute value of 'weight' does not matter much -- the weight value won't be used as 'frequency' or profile count. The only interpretation of the weight meta data is by forming the branch probability out of it.<div><br></div><div>In other words, if BFI does not have the limitation of handling '0' weight, it makes no difference annotating a branch with (1, 0) weights vs (1<<31, 0) --- as the resulting branch probability will be the same 100%.</div><div><br></div><div>My opinion is that we should make builtin_expect produce more biased BP than what we have today, but do not go extreme to consider other potential use cases. If user really wants extreme setting, he can also use the internal option to override it.</div><div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 26, 2016 at 11:11 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;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 class="HOEnZb"><div class="h5"><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:0 0 0 .8ex;border-left:1px #ccc solid;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: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></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: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></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: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></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:0 0 0 .8ex;border-left:1px #ccc solid;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: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></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>