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

[Nick Lewycky]

On 3 October 2011 09:50, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:

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

Sure. I was relying on the reader understanding "n choose k".

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

Yes, k > n should be zero.


> +                    const SCEV *CoeffTerm = getConstant(Ty,
> Coeff1*Coeff2);
>
> Can this multiplication overflow?
>

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.

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

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.

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111003/c9acdbae/attachment.html>