<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Aug 18, 2015 at 5:58 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2015-Aug-18, at 11:47, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
><br>
> Hi Duncan<br>
><br>
</span><span class="">> Could you please take a look at this patch again? I updated this patch by converting all zero weights into ones in BFI. You can check my previous message below for more details.<br>
><br>
> Thank you very much!<br>
><br>
<br>
<br>
</span>> Index: test/Transforms/SampleProfile/branch.ll<br>
> ===================================================================<br>
> --- test/Transforms/SampleProfile/branch.ll<br>
> +++ test/Transforms/SampleProfile/branch.ll<br>
> @@ -36,18 +36,18 @@<br>
>    tail call void @llvm.dbg.value(metadata i8** %argv, i64 0, metadata !14, metadata !DIExpression()), !dbg !27<br>
>    %cmp = icmp slt i32 %argc, 2, !dbg !28<br>
>    br i1 %cmp, label %return, label %if.end, !dbg !28<br>
> -; CHECK: edge entry -> return probability is 0 / 1 = 0%<br>
> -; CHECK: edge entry -> if.end probability is 1 / 1 = 100%<br>
> +; CHECK: edge entry -> return probability is 1 / 2 = 50%<br>
> +; CHECK: edge entry -> if.end probability is 1 / 2 = 50%<br>
<br>
This doesn't look like the right result to me.<br>
<br>
In the frontend instrumentation, we used Laplace's Rule of Succession<br>
when creating branch weights.  Should you do the same thing with the<br>
sampling profile?  Or something similar?<br></blockquote><div><br></div><div>You mean converting 0 and 1 into 1 and 2? I am not sure if it is reasonable to do this for branch weights that are indicated in .ll files: clients may not be aware of this change and they may use 0 and 1 weights intentionally to represent a very cold branch and a very hot branch. The change in this patch is quite temporary and it is borrowed from BFI (explained below).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Also, what's the point of supporting edge weights of 0 if they're<br>
ignored in branch probability?<br></blockquote><div><br></div><div>Sorry, I don't get it here. Where zero edge weights are supported? And how they are ignored in branch probability?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
More below...<br>
<span class=""><br>
> thanks,<br>
> Cong<br>
><br>
> On Tue, Aug 11, 2015 at 4:59 PM, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
> Hi Duncan<br>
><br>
> I committed this patch in the past after your approval, but it broke some others tests (see PR24377) that are not in the tree and I then reverted it. Later I investigated the bug and found the problem is that we could get zero edge weights from BPI (usually the zero weight is from test cases). When those zero weights come to MBPI, they will be turned into 16, which will lead to overflow when summing up weights. For example, if we have two edge weights 0 and UINT32_MAX, in the weight normalization function they are OK as the sum of them is not greater than UINT32_MAX. But when summing them up in MBPI we are adding 16 + UINT32_MAX instead of 0 + UINT32_MAX. This will lead to assertion failures.<br>
<br>
</span>Is this a problem in MBPI that should be fixed?  Maybe we should<br>
have another mechanism for specifying "unknown"?  Or maybe we<br>
should deal gracefully with 0 weights?<br></blockquote><div><br></div><div>I am considering to propose an improvement on MBB::addSuccessor(). Specifically, we can discard the use of default parameter but split it into two interfaces: the one without the weight parameter will use default weight, and the one with the weight parameter will accept any weight passed in, including 0. In this way, we can still allow zero weights indicated by clients without converting it into default weight later (then MBPI won't convert zero into default weight anymore). What do you think?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> As when BFI meets zero weights it will turn them into ones,<br>
<br>
</span>I thought BFI only did this when it was normalizing.  Am I wrong?<br></blockquote><div><br></div><div>In BlockFrequencyInfoImplBase::addToDist(), the first statement is:</div><div><br></div><div><div>  if (!Weight)</div><div>    Weight = 1;</div></div><div><br></div><div>This is where BFI turns zero weights into ones. That is why I did the same thing in this patch. Another way to normalize zero weights is to let other weights very big and turn zero into one. For example, we can normalize 0 and 1 into 1 and UINT32_MAX - 1. But I am not sure this is the right way.</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 class="HOEnZb"><div class="h5"><br>
> I did the same trick in BPI by turning zero edge weights into ones. Now those test failures have disappeared.<br>
><br>
> Could you please take a look again at this patch? The additional update is in lib/Analysis/BranchProbabilityInfo.cpp and some test cases. Thank you very much!<br>
><br>
><br>
> thanks,<br>
> Cong<br>
><br>
> On Tue, Aug 11, 2015 at 4:43 PM, Cong Hou <<a href="mailto:congh@google.com">congh@google.com</a>> wrote:<br>
> congh removed rL LLVM as the repository for this revision.<br>
> congh updated this revision to Diff 31882.<br>
> congh added a comment.<br>
><br>
> Update the patch by turning zero edge weights into one in BPI so that we won't get zero weights in BPI anymore.<br>
><br>
><br>
> <a href="http://reviews.llvm.org/D11442" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11442</a><br>
><br>
> Files:<br>
>   include/llvm/CodeGen/MachineBasicBlock.h<br>
>   include/llvm/CodeGen/MachineBranchProbabilityInfo.h<br>
>   lib/Analysis/BranchProbabilityInfo.cpp<br>
>   lib/CodeGen/IfConversion.cpp<br>
>   lib/CodeGen/MachineBasicBlock.cpp<br>
>   lib/CodeGen/MachineBlockPlacement.cpp<br>
>   lib/CodeGen/MachineBranchProbabilityInfo.cpp<br>
>   test/CodeGen/X86/pr24377.ll<br>
>   test/Transforms/SampleProfile/branch.ll<br>
>   test/Transforms/SampleProfile/calls.ll<br>
>   test/Transforms/SampleProfile/propagate.ll<br>
><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>