[llvm-commits] patch: chrec * chrec = chrec.

[Jakob Stoklund Olesen]

On Oct 2, 2011, at 4:50 PM, Nick Lewycky wrote:

> 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.

Hi Nick,

I can't really comment on the SCEV stuff, but

+static unsigned Choose(unsigned n, unsigned k, bool &Overflow) {
+  // This method can overflow internally even if the final result would fit.
+
+  if (n == 0) return 1;
+  if (k >= n) return 1;
+
+  if (k > n/2)
+    k = n-k;
+
+  unsigned r = 1;
+  for (unsigned i = 1; i <= k; ++i) {
+    r = umul_ov(r, n-(i-1), Overflow);
+    r /= i;
+  }
+  return r;
+}

Please add a comment that this function is computing a binomial coefficient, and how it does it.

In particular, it wasn't immediately clear to me that the division never truncates.

Are binomial coefficients defined for k > n? If so, shouldn't the value be 0?

+                    const SCEV *CoeffTerm = getConstant(Ty, Coeff1*Coeff2);

Can this multiplication overflow?

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.

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.

/jakob