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

[Nick Lewycky]

Andrew Trick wrote:
> 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).

Guilty here, reporting as ordered.

  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.

My best guess is that this is a buggy attempt to revisit Ops and 
continue folding when the inner loop modified Ops, before going back 
through getMulExpr. What we actually do is through getMulExpr 
immediately as soon as a modification is made, so the outer loop is 
completely pointless.

Removing it passes tests, I'll remove it.

Nick

>
> http://reviews.llvm.org/D5113
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

http://reviews.llvm.org/D5113