[PATCH] ScalarEvolution incorrectly assumes that the start of certain add recurrences don't overflow

[Andrew Trick]

Well, this is the usual problem of transferring nsw from one expression to another equivalent expression without checking control equivalence. I have no idea how you would break this without an early exit.

The purpose of this canonicalization step is to support LoopStrength reduce, which normalizes all of its induction variables so that pre and post-increment uses are associated with the same recurrence.

I agree there should be unit tests that are covering this if it is an important optimization. I think at the time there was no way to write stand-alone LSR tests.

You mentioned that you can replace this with a correct version. I would love to see that!

If you don't have a replacement for the broken code, then please just add a check that the "preincrement" recurrence must be incremented on every path that the "post-increment" recurrence will be used.

Something like: L->getExitBlock() == L->getLoopLatch()


================
Comment at: lib/Analysis/ScalarEvolution.cpp:1367
@@ +1366,3 @@
+  // ScalarEvolution may have concluded that PreAR does not overflow due to a
+  // pre-existing add recurrance that contains an `add nsw` at the IR level.
+  // There may be a path out of the loop without executing that `add nsw`, so we
----------------
recurrence

http://reviews.llvm.org/D7331

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/