[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:137
@@ -136,2 +136,3 @@
   void RewriteLoopExitValues(Loop *L, SCEVExpander &Rewriter);
+  void RewriteLoopExitValuesWithoutBECount(Loop *L);
 
----------------
I'm not sure that `RewriteLoopExitValuesWithoutBECount` is the best name for this function.  The transform is only incidentally being done over a loop for which we have no backedge count.

How about `RewriteFirstIterationLoopExitValues`, since we're really using the fact that the exit, if taken, will only be taken in the first ever loop iteration?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:715
@@ +714,3 @@
+/// loop exits. If so, we know that if the exit path is taken, it is at the
+/// first loop iteration. If the induction variable used in the exit path
+/// has not been updated before the exit condition, it will keep its initial
----------------
I think "updated" is misleading when talking about SSA.  Why not keep the comment brief and simple, something like:

```
/// Check to see if this loop has exits conditional on loop invariant values.  If so, we know that if the exit path is taken, it is taken on the first iteration.  This lets us predict the value of well behaved induction variables.
```


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:718
@@ +717,3 @@
+/// value from loop preheader. Therefore, we can rewrite the exit value with
+/// its initial value. As a side effect, the phi node generated by LCSSA is
+/// no longer needed.
----------------
I'm not sure if "As a side effect, the phi node generated by LCSSA is
/// no longer needed." is accurate -- can't LCSSA phis have multiple incoming values?

[edit] See longer discussion below.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:727
@@ +726,3 @@
+
+  SmallVector<BasicBlock*, 8> ExitBlocks;
+  L->getUniqueExitBlocks(ExitBlocks);
----------------
Nit: spacing.

I'll suggest just running the diff through clang-format before submitting.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:734
@@ +733,3 @@
+    // values defined inside the loop are used on this path.
+    while (PHINode *PN = dyn_cast<PHINode>(begin++)) {
+      // We only deal with LCSSA phis.
----------------
Use `auto *` here.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:735
@@ +734,3 @@
+    while (PHINode *PN = dyn_cast<PHINode>(begin++)) {
+      // We only deal with LCSSA phis.
+      if (PN->getNumIncomingValues() != 1)
----------------
I think this is misleading -- can't LCSSA phis have multiple incoming values?  I'm thinking of cases like these:


```
for (...) {
  t = ...;
  if (..) goto x;
  ..
  if (..) goto x;
}
...
x:
  print(t)
```

`t` dominates `x` so in "normal" code you won't need a PHI, but if you //do// want to insert a PHI for `t` then you'll need a `phi [x, x]`.  Actually I'm not even sure if an "LCSSA PHI" is a well defined concept.


In any case, I don't think there is any need to special case LCSSA PHI's here.  You //could// special case single entry PHIs if you really wanted to, but I doubt that restriction will make your change any simpler.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:742
@@ +741,3 @@
+
+      Value *Cond = nullptr;
+      if (BranchInst *BI = dyn_cast<BranchInst>(TermInst)) {
----------------
This is mostly stylistic, but prefer using `auto *` on the LHS of a `dyn_cast`; since the type is obvious.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:752
@@ +751,3 @@
+      // All non-instructions are loop-invariant.
+      if (auto *CondInst = dyn_cast<Instruction>(Cond))
+        if (!L->hasLoopInvariantOperands(CondInst))
----------------
What if `Cond` is a `LoadInst` loading from an `i1 *`?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:758
@@ +757,3 @@
+      // incoming block as loop preheader, and others as back edges.
+      auto *InductionPHI =
+          dyn_cast<PHINode>(PN->getIncomingValue(0));
----------------
How do you know that the 0th incoming value to `PN` is an induction PHI node?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:770
@@ +769,3 @@
+      auto *LoopPreheader = L->getLoopPreheader();
+      if (InductionPHI->getBasicBlockIndex(LoopPreheader) != -1) {
+        PN->setIncomingValue(0,
----------------
I don't quite follow the logic of when `InductionPHI->getBasicBlockIndex(LoopPreheader)` can be `-1`.  The preheader is the only block outside the loop branching into the header, and an induction variable (which will be a PHI node on the loop header) should have to have one incoming value for the "first" iteration which will have to be the value incoming from the preheader.

If you see failures without this check, I suspect this is because, as I mentioned in the previous comment, `PN->getIncomingValue(0)` need not be an induction PHI node.


http://reviews.llvm.org/D13974