[PATCH] D26185: [ARM] Loop Strength Reduction crashes when targeting ARM or Thumb if the LLVM-IR is not in LCSSA form.

[Eli Friedman via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:329
+    if (!AR->getStart()->isZero() &&
+        SE.isLoopInvariant(AR->getStepRecurrence(SE), AR->getLoop())) {
       DoInitialMatch(AR->getStart(), L, Good, Bad, SE);
----------------
efriedma wrote:
> "SE.isLoopInvariant(AR->getStepRecurrence(SE), AR->getLoop())" is always true; if you try to build an AddRec with non-invariant operands, you'll hit the assertion you pointed out.
Ah, right, my previous comment was wrong.  "AR->isAffine()" would be a more clear way to write this check.  (A non-affine AddRec always has a step which is not invariant, and an affine AddRec always has a step which is invariant.)


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:582
       // FIXME: AR->getNoWrapFlags(SCEV::FlagNW)
-      return SE.getAddRecExpr(Start, Step, AR->getLoop(), SCEV::FlagAnyWrap);
+      if (SE.isLoopInvariant(Step, AR->getLoop()))
+        return SE.getAddRecExpr(Start, Step, AR->getLoop(), SCEV::FlagAnyWrap);
----------------
efriedma wrote:
> It would be more clear to check "SE.isLoopInvariant(RHS, AR->getLoop())".
(Ignore my previous comment here.)


================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:3208
+    if (Remainder != AR->getStart() &&
+        SE.isLoopInvariant(AR->getStepRecurrence(SE), AR->getLoop())) {
       if (!Remainder)
----------------
efriedma wrote:
> Again, useless check.
(Ignore my previous comment here.)


https://reviews.llvm.org/D26185