[PATCH] Exit ScalarEvolution::getMulExpr Early when Choose Overflows
Andrew Trick
atrick at apple.com
Thu Aug 28 23:07:19 PDT 2014
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
More information about the llvm-commits
mailing list