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

[Chen Li via llvm-commits]

chenli added inline comments.

================
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)
----------------
sanjoy wrote:
> 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.
You are correct. For some reason I thought LCSSA PHIs were always single entry PHIs so I used the check to filter non LCSSA PHIs. I will fix it.

================
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))
----------------
sanjoy wrote:
> What if `Cond` is a `LoadInst` loading from an `i1 *`?
I think I'll just use Loop::isLoopInvariant(const Value *V) instead.

================
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));
----------------
sanjoy wrote:
> How do you know that the 0th incoming value to `PN` is an induction PHI node?
This is basically because of the previous assumption that we only have single entry PHI here. So if there is an induction PHI node, it should be 0th. The later 
```
InductionPHI->getBasicBlockIndex(LoopPreheader) != -1
```
is used to filter non-induction PHI node. 
The name InductionPHI is confusing because it's not necessary an indication PHI if it fails the check. 
I will refactor the code to make it cleaner.


http://reviews.llvm.org/D13974