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

[Jakob Stoklund Olesen]

On Oct 3, 2011, at 1:03 PM, Nick Lewycky wrote:

> On 3 October 2011 09:50, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>  
> +                    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.

Or just promote to 64-bit. ScalarEvolution::getConstant() already takes a uint64_t.

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

Fair enough.

/jakob

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