<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 15, 2016 at 4:53 PM, Michael Kuperstein <span dir="ltr"><<a href="mailto:mkuper@google.com" target="_blank">mkuper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mkuper added a subscriber: bogner.<br>
mkuper added a comment.<br>
<span class=""><br>
In <a href="https://reviews.llvm.org/D25963#596720" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25963#596720</a>, @anemet wrote:<br>
<br>
> In <a href="https://reviews.llvm.org/D25963#596582" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25963#596582</a>, @mkuper wrote:<br>
><br>
> > Thanks, Adam!<br>
> >  I'll fix the cosmetics, but I'm still not sure what to do about the branch weight adjustments.<br>
><br>
><br>
> Wasn't @davidxl working on fixing them?<br>
<br>
<br>
</span>Not that I know of. @davidxl, did I miss something?<br>
<br>
Also, phab seems to have swallowed a long comment I wrote about that. :-\<br>
<br>
Basically, the underlying issue is that clang doesn't actually record the branch weights it gets from the profile. Rather, it records the branch weights it thinks are better for the purpose of branch probability estimation.  From CodeGenPGO.cpp:<br>
/// According to Laplace's Rule of Succession, it is better to compute the<br>
/// weight based on the count plus 1, so universally add 1 to the value.<br>
While this may be good from a branch-probability standpoint, it throws off using the weights for trip count estimates, when the exit weight is small.<br>
<br>
I see 3 options:<br>
<br>
1. Keep what I have here as a band-aid.<br></blockquote><div><br></div><div>Probably not.  it makes IR based PGO wrong, and also defeats the purpose of the original adjustment in the first place. In other words, 1) is strictly worse than 3) below.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2. Don't do the -1 adjustment here - hopefully, in reality, we don't care about the cases where it makes a difference. I'll need to benchmark this, though.<br></blockquote><div><br></div><div><br></div><div>See my reply. We should go for 2.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3. Remove the +1 adjustment in clang, and instead do it where we actually use the branch weights to estimate probabilities, that is, in BPI. Of course, there may be other places that read the weights but really want the adjusted weights (and, say, assume weights are never 0). I asked @bogner about this on IRC, and he seemed ambivalent.<br></blockquote><div><br></div><div>This has been discussed many times in the past.  The conclusion is that we will keep FE profile handling as it is today.  </div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What do you think?<br>
<br>
<br>
<a href="https://reviews.llvm.org/D25963" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25963</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>