<div class="gmail_quote">On 3 October 2011 09:50, Jakob Stoklund Olesen <span dir="ltr"><<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<div><br>
On Oct 2, 2011, at 4:50 PM, Nick Lewycky wrote:<br>
<br>
> This patch is a follow-up to r139133 which corrected SCEV's getMulExpr when given two chrec arguments. Here I extend it to correctly multiply chrecs of any length.<br>
<br>
</div>Hi Nick,<br>
<br>
I can't really comment on the SCEV stuff, but<br>
<br>
+static unsigned Choose(unsigned n, unsigned k, bool &Overflow) {<br>
+  // This method can overflow internally even if the final result would fit.<br>
+<br>
+  if (n == 0) return 1;<br>
+  if (k >= n) return 1;<br>
+<br>
+  if (k > n/2)<br>
+    k = n-k;<br>
+<br>
+  unsigned r = 1;<br>
+  for (unsigned i = 1; i <= k; ++i) {<br>
+    r = umul_ov(r, n-(i-1), Overflow);<br>
+    r /= i;<br>
+  }<br>
+  return r;<br>
+}<br>
<br>
Please add a comment that this function is computing a binomial coefficient, and how it does it.<br></blockquote><div><br></div><div>Sure. I was relying on the reader understanding "n choose k".</div><div><br></div>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">In particular, it wasn't immediately clear to me that the division never truncates.<br>
<br>
Are binomial coefficients defined for k > n? If so, shouldn't the value be 0?<br></blockquote><div><br></div><div>Yes, k > n should be zero.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


+                    const SCEV *CoeffTerm = getConstant(Ty, Coeff1*Coeff2);<br>
<br>
Can this multiplication overflow?<br></blockquote><div><br></div><div>Oh good point. I spent all this time thinking about what if Ty is an i3 and Coeff1*Coeff2 doesn't fit there, but not the case where Ty is larger than unsigned. The most straight-forward thing to do is reuse umul_ov.</div>


<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Finally, your x,y,z loop is O(N^4) in the number of SCEV operands. I don't know if this is a problem in practice.<br>



<br>
The Choose() calls in that loop are computing the same intermediate results over and over. You could get an O(N^3) computation by reusing those binomials.<br></blockquote><div><br></div><div>Both true, but not yet observed to be problems in practice. One alternative approach would be to allocate a binomial-coefficient scratch space and then use the recursive algorithm. I was really hoping to avoid putting more complex implementation for Choose into this patch, there's enough gnarly math in there as it is.</div>


<div><br></div><div>Nick</div><div><br></div></div>