[PATCH] Make ScalarEvolution::ComputeExitLimit more agressive

[Sanjoy Das]

Replies inline.


================
Comment at: include/llvm/IR/BasicBlock.h:214
@@ +213,3 @@
+  ///
+  /// Note that unique successir doesn't mean single edge, there can be
+  /// multiple edges from this block to the unique successor (for example a
----------------
artagnon wrote:
> s/successir/successor/
Will fix.

================
Comment at: include/llvm/IR/BasicBlock.h:217
@@ +216,3 @@
+  /// switch statement with multiple cases having the same destination).
+  BasicBlock *getUniqueSuccessor();
+  const BasicBlock *getUniqueSuccessor() const {
----------------
artagnon wrote:
> Slightly tangential: is a hypothetical getSingleSucessor() not useful yet?
Even if it is, that's not relevant to this patch.  But honestly, I don't know.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4836
@@ +4835,3 @@
+    while (true) {
+      if (std::next(pred_begin(UniqueSucc)) != pred_end(UniqueSucc))
+        return false;
----------------
artagnon wrote:
> Um, pred_size()?
I can't find a `pred_size`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4840
@@ +4839,3 @@
+      BasicBlock *Next = UniqueSucc->getUniqueSuccessor();
+      if (!Next || Next == L->getHeader())
+        break;
----------------
artagnon wrote:
> Are you worried that `L->getHeader()` might be `nullptr`? Why check `Next` separately then?
`L->getHeader()` cannot be `nullptr`.  Given that, I don't follow this comment -- I want to `break` if `Next` is `nullptr` OR `Next` is `L->getHeader()`.  How do I do that with a single check?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4866
@@ -4835,3 +4865,3 @@
       Exit = *SI;
-    } else if (*SI != L->getHeader()) {
-      MustExecuteLoopHeader = false;
+    } else if (*SI != L->getHeader() && MustExecuteLoopHeader) {
+      // TODO: DominatesHeaderIgnoringLoopEntry cannot be true for two different
----------------
artagnon wrote:
> Redundant `&& MustExecuteLoopHeader` -- you do an `&=` anyway.
`DominatesHeaderIgnoringLoopEntry` is not free, and I don't want to call it if `MustExecuteLoopHeader` is `false`.

================
Comment at: lib/IR/BasicBlock.cpp:249
@@ +248,3 @@
+  succ_iterator SI = succ_begin(this), E = succ_end(this);
+  if (SI == E) return nullptr; // No preds.
+  BasicBlock *SuccBB = *SI;
----------------
artagnon wrote:
> succ_empty()?
Again, I cannot find a `succ_empty`.  But I did notice that `// No preds.` should be `// No successors.`.

================
Comment at: lib/IR/BasicBlock.cpp:252
@@ +251,3 @@
+  ++SI;
+  for (;SI != E; ++SI) {
+    if (*SI != SuccBB)
----------------
artagnon wrote:
> Probably nicer with a range-based loop or std::all_of.
`std::all_of` is a good idea.  I don't want to make that change in this patch because I want `getUniqueSuccessor` to be an exact dual of `getUniquePredecessor`.  But I can refactor both in a cleanup change.

================
Comment at: test/Analysis/ScalarEvolution/compute-exit-limit-aggressively.ll:72
@@ +71,3 @@
+; CHECK: Determining loop execution counts for: @h
+; CHECK-NEXT: Loop %loop: Unpredictable backedge-taken count.
+; CHECK-NEXT: Loop %loop: Unpredictable max backedge-taken count.
----------------
artagnon wrote:
> Why have you shown two of these examples with the unknown parameter passed?
These are negative tests that demonstrate that the transform does not fire incorrectly.

http://reviews.llvm.org/D6830

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