[PATCH] D13974: [IndVarSimplify] Rewrite loop exit values with their initial values from loop preheader

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:729
@@ +728,3 @@
+    while (auto *PN = dyn_cast<PHINode>(begin++)) {
+      for (unsigned incomingValIdx = 0, e = PN->getNumIncomingValues();
+          incomingValIdx != e; ++incomingValIdx) {
----------------
I know I suggested this, but LLVM style is `IncomingValIdx`.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:732
@@ +731,3 @@
+        // Get condition that leads to the exit path.
+        auto *TermInst = PN->getIncomingBlock(incomingValIdx)->getTerminator();
+
----------------
Do you not need to check that ` PN->getIncomingBlock(incomingValIdx)` is inside the loop?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:736
@@ +735,3 @@
+        if (auto *BI = dyn_cast<BranchInst>(TermInst)) {
+          if (!BI->isConditional())
+            continue;
----------------
Note: this transform is also valid for unconditional branches (since `br label %foo` is equivalent to `br true label %foo label %foo`).

Actually, thinking about it, I think we can pretty much `assert(BI->isConditional())` (IOW you don't need the check since `BI->getCondition()` will have the same assert it in).  (Please verify this) I think you cannot have an unconditional branch from a block `B` in the loop to a loop exit since then `B` will not be connected to the header (i.e. there won't be a path from `B` to the loop header) and hence `B` won't be part of the loop leading to a contradiction.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:759
@@ +758,3 @@
+        auto *LoopPreheader = L->getLoopPreheader();
+        if (ExitVal->getBasicBlockIndex(LoopPreheader) != -1) {
+          assert(ExitVal->getParent() == L->getHeader() &&
----------------
I'd put an `assert(LoopPreaheader)` here.


http://reviews.llvm.org/D13974