[llvm-commits] patch: chrec * chrec = chrec.
Jakob Stoklund Olesen
stoklund at 2pi.dk
Mon Oct 3 09:50:42 PDT 2011
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
More information about the llvm-commits
mailing list