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

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Mostly minor stylistic comments inline.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:715
@@ +714,3 @@
+/// exits. If so, we know that if the exit path is taken, it is at the first
+/// loop iteration. This lets us predict values of well behaved induction
+/// variables.
----------------
Now that we're not longer special casing induction variable, please change this comment to be something like "lets us predict values of PHI nodes that live on the loop header".

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:720
@@ +719,3 @@
+/// incoming block as loop preheader and others as back edges. But there's
+/// no restrictions to handle other cases as well.
+void IndVarSimplify::RewriteFirstIterationLoopExitValues(Loop *L) {
----------------
I think this is a false promise :) There are restrictions around other
cases, e.g. (using a terrible LLVM and C mashup):

```
 for (init; cond; incr) {
  ..
  if (c) {
   ..
   br merge;
  } else {
   ..
   br merge;
  }
 merge:
  %val = phi [ 0, %if ], [ 1, %else ]
  if (loop_invariant_cond)
    br exit
  ...
}

exit:
  %lcssa = phi [ %val ]
```

You cannot predict the value of `%val` here.

I'd just remove this `NOTE` for now.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:733
@@ +732,3 @@
+    while (auto *PN = dyn_cast<PHINode>(begin++)) {
+      for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) {
+        // Get condition that leads to the exit path.
----------------
I'd suggest doing one of the following

 - rename `i` to something more descriptive (like `incomingValIdx`) 

OR

 - extract out the loop body to a lambda called `ProcessIncomingValue` or something like that.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:751
@@ +750,3 @@
+
+        auto *ExitVal =
+            dyn_cast<PHINode>(PN->getIncomingValue(i));
----------------
Super minor: do we actually need a line break here?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:754
@@ +753,3 @@
+
+        // Currently only deal with PHIs.
+        if (!ExitVal)
----------------
I'd remove the `Currently` -- this optimization is fundamentally intended to work with PHI nodes.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:760
@@ +759,3 @@
+        // preheader (if the exit value is in a joint block of the
+        // loop then it will not be qualified).
+        auto *LoopPreheader = L->getLoopPreheader();
----------------
I think ` (if the exit value is in a joint block of the oop then it will not be qualified)` bit is somewhat redundant.

I'd just say "If ExitVal is a PHI on the loop header, then we know its value along this exit because the exit can only be taken on the first iteration" and leave it at that.  No point in discussing the invalid cases.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:763
@@ +762,3 @@
+        if (ExitVal->getBasicBlockIndex(LoopPreheader) != -1) {
+          PN->setIncomingValue(i,
+              ExitVal->getIncomingValueForBlock(LoopPreheader));
----------------
Add an assert here `ExitVal` lives on the loop header.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2128
@@ -2063,1 +2127,3 @@
 
+  // Further clean up loop exit values.
+  RewriteFirstIterationLoopExitValues(L);
----------------
//This// may be a good place to state that `RewriteFirstIterationLoopExitValues` does not depend on us being able to compute the trip count of `L`, and therefore can get cases that `RewriteLoopExitValues` does not.  You can also put this "factoid" in the declaration of `RewriteFirstIterationLoopExitValues`


http://reviews.llvm.org/D13974