<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 19, 2015 at 1:08 PM, Cong Hou <span dir="ltr"><<a href="mailto:congh@google.com" target="_blank">congh@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">congh added a comment.<br>
<span class=""><br>
In <a href="http://reviews.llvm.org/D13745#268627" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13745#268627</a>, @davidxl wrote:<br>
<br>
> Can you proceed with what Manman suggested (split it into smaller patches)? Here is my suggestion:<br>
><br>
> Patch-1: new interfaces -- no functional changes<br>
<br>
<br>
</span>This may increase the storage space of some classes like MBB which will store both weights and probabilities. Is that OK?<br></blockquote><div><br></div><div>How much memory overhead do you see? I expect it to be low.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Patch-2.1: SelectionDAG builder change to use new interfaces<br>
<br>
<br>
</span>So for other users we just treat probabilities (which is built in SelectionDAGBuilder by using the new interfaces) as weights, right?<br></blockquote><div><br></div><div>yes -- something in that line.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> Patch-2.2: rest of the clients<br>
<br>
> Patch-3: remove old weight based interfaces (no functional change expected)<br>
<br>
<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Analysis/BranchProbabilityInfo.h:93<br>
@@ -89,3 +92,3 @@<br>
/// are updating the analysis by later calling setEdgeWeight.<br>
uint32_t getEdgeWeight(const BasicBlock *Src,<br>
unsigned IndexInSuccessors) const;<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> congh wrote:<br>
> > davidxl wrote:<br>
> > > Why is this interface still kept? As in MBB, this one should also be deprecated.<br>
> > This is needed in BlockFrequencyInfoImpl. Note that in this patch I only changed edge weights into probabilities in MBB but not in BranchProbabilityInfo. Do you think if we should also do this in BranchProbabilityInfo?<br>
> yes, this should also be done for BPI -- but in another patch after this one gets in and stablizes.<br>
</span>OK.<br>
<span class=""><br>
================<br>
Comment at: include/llvm/Support/BranchProbability.h:41<br>
@@ -40,3 +40,3 @@<br>
BranchProbability() : N(0) {}<br>
BranchProbability(uint32_t Numerator, uint32_t Denominator);<br>
<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> congh wrote:<br>
> > davidxl wrote:<br>
> > > Any reason why Numerator and Denominator can not be uint64_t?<br>
> > If we add an overload constructor with type uint64_t, there will be many compilation errors of ambiguous overloads when we pass arguments of int types.<br>
> How about removing the interface with 32bit parameter and just keep the 64bit one?<br>
</span>At this momenta it seems that we only need 64-bit as input in one place. So is it worthwhile to do this? This may bring overhead when constructing probabilities.</blockquote><div><br></div><div>Actually current 32bit based interface does unnecessary casting that can be eliminated, so I think it is worth while. A side note, I think that constructor needs to be an inline function defined in the header.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Another solution is creating a static builder function that takes 64-bit as inputs.<br></blockquote><div><br></div><div>Probably not worth it.</div><div><br></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">
<span class=""><br>
================<br>
Comment at: include/llvm/Support/BranchProbability.h:83<br>
@@ -82,4 +82,3 @@<br>
BranchProbability &operator+=(BranchProbability RHS) {<br>
- assert(N <= D - RHS.N &&<br>
- "The sum of branch probabilities should not exceed one!");<br>
- N += RHS.N;<br>
+ // Saturate the result in case of overflow.<br>
+ N = (N + RHS.N < N) ? D : N + RHS.N;<br>
----------------<br>
</span><span class="">davidxl wrote:<br>
> congh wrote:<br>
> > davidxl wrote:<br>
> > > when can overflow happen? Also should the check really be:<br>
> > ><br>
> > > (uint64_t)N + RHS.H > D ?<br>
> > When we construct a branch probability we do rounding to get the numerator. If the denominator is odd, then 1/2 + 1/2 will exceed 1 using BranchProbability representation. That is when overflow happens.<br>
> ><br>
> > I forgot to update this part as when it is written we were using UINT32_MAX as denominator. I will update it.<br>
> ok -- but the check does not handle the case when the resulting Prob > 1.<br>
</span>We don't allow a probability to be greater than 1, so here we use the saturating operation:<br>
<br>
N = ((uint64_t)N + RHS.N > D) ? D : N + RHS.N;<br>
<br>
<br>
<a href="http://reviews.llvm.org/D13745" rel="noreferrer" target="_blank">http://reviews.llvm.org/D13745</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>