[PATCH] Exit ScalarEvolution::getMulExpr Early when Choose Overflows

[Andrew Trick]

Good idea! Your change looks good, but I'm not sure I understand the math.

Instead of:
  
  Choose(AddRec->getNumOperands() - 2,
         AddRec->getNumOperands() / 2 - 1,

Why not:

  // max(choose(n, k)) = choose(max(n), max(k)/2)
  // max(n) = max(x)
  //   = AddRec->getNumOperands() + OtherAddRec->getNumOperands() - 2
  //   where OtherAddRec->getNumOperands >= 2
  //
  // max(k)/2 = max(2x - y)/2 = max(x)/2 = AddRec->getNumOperands()/2
  Choose(AddRec->getNumOperands(),
         AddRec->getNumOperands() / 2,

Regardless of whether I'm right, please add similar comments.

Instead of modifying the loop condition, it might me more clear just to write:

> if (EarlyOut)
>   continue

(otherwise it looks like EarlyOut is loop variant).

On more thing. Can you please explain the state of the existing code? (note, I didn't write this, I only reformatted it at one point). Why do we need two attempts to loop over OtherIdx?

    for (unsigned OtherIdx = Idx+1;
         OtherIdx < Ops.size() && isa<SCEVAddRecExpr>(Ops[OtherIdx]);
         ++OtherIdx) {

That makes the getMulExpr reduction cubic! (Probably n^4 considering the cost of Choose.)

I stared at this for a long time and just don't get it. We only iterate over the OuterIdx loop when all previous attempts to multiply AddRec * OtherAddRec failed with Overflow. This seems to me to guarantee that all subsequent attempts will fail.

http://reviews.llvm.org/D5113