[PATCH] D62563: Fix incorrect expand of non-linear addrecs

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Artur, nice find.  In terms of staging complexity, have you considered how impactful it would be to simply refuse to generate the value at the given iteration in this case?  evaluateAtIteration is allowed to return SCEVCouldNotCompute.  I'm tempted to first introduce a bailout for a quick correctness fix - maybe along side your assert to see if we're missing any other cases - and then spend more time considering your full fix.

One observation: I really don't think we want to be emitting an i9.  We probably want to be rounding up to the nearest legal type.  For i65, I doubt we want to be generating it at all.  It's probably best to simply bail out in that case.

p.s. Please go ahead and land your preparatory changes (1).  The context was helpful, but we should get those out of the way to simplify the rest of the review.  (This is mostly just factoring out CanonicalIVType right?)



================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1486
 
-  // Rewrite an AddRec in terms of the canonical induction variable, if
-  // its type is more narrow.
----------------
Removing this entirely seems like overkill.  Maybe either add a restriction to a) affine addrecs or b) the bitwidth of the existing canonical IV is sufficient?


================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1568
+           "Canonical IV can't be more narrow than the expr!");
+    return expand(SE.getTruncateExpr(SE.getUnknown(CanonicalIV), Ty));
   }
----------------
Not sure you actually want getUnknown?  Did you mean getSCEV?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62563/new/

https://reviews.llvm.org/D62563