[PATCH] D50558: [MustExecute] Fix algorithmic bug in isGuaranteedToExecute. PR38514

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Max convinced me that there's no algorithmic complexity change implied here.  Given the getLoopExits code walks all edges leaving blocks in the loop, both the old and new code is O(outbound edges of blocks in loop).

I've added a bunch of code comments, but I am anticipating approving this once those details are fixed.



================
Comment at: lib/Analysis/MustExecute.cpp:61
 /// be executed before the ExitBlock is executed in any dynamic execution trace.
-static bool CanProveNotTakenFirstIteration(BasicBlock *ExitBlock,
+static bool CanProveNotTakenFirstIteration(const BasicBlock *ExitBlock,
                                            const DominatorTree *DT,
----------------
Separate and land please.


================
Comment at: lib/Analysis/MustExecute.cpp:110
+
+  // Collect all transitive predecessors of BB in the same loop.
+  SmallPtrSet<const BasicBlock *, 4> Predecessors;
----------------
Add to the end of your comment: This set will be a subset of the blocks within the loop.


================
Comment at: lib/Analysis/MustExecute.cpp:113
+  SmallVector<const BasicBlock *, 4> WorkList;
+  for (auto *Pred : predecessors(BB)) {
+    Predecessors.insert(Pred);
----------------
What you have here is fine, but it might be cleaner to write this as:
Worklist.push_back(BB);
do {
} while (!Worklist.empty())


================
Comment at: lib/Analysis/MustExecute.cpp:115
+    Predecessors.insert(Pred);
+    WorkList.push_back(BB);
+  }
----------------
I think you meant push_back(Pred) here. 


================
Comment at: lib/Analysis/MustExecute.cpp:120
+    assert(CurLoop->contains(Pred) && "Should only reach loop blocks!");
+    // We are not interested in backedges.
+    if (Pred == CurLoop->getHeader())
----------------
Add to comment: and don't want to leave the loop.


================
Comment at: lib/Analysis/MustExecute.cpp:123
+      continue;
+    for (auto *PredPred : predecessors(Pred))
+      if (Predecessors.insert(PredPred).second)
----------------
Minor: One slightly non-obvious piece is that there can be cycles *within* a loop.  Either sub-loops or non-loop cycles.  The code does appear to handle that case, but might be worth a comment/test.


================
Comment at: lib/Analysis/MustExecute.cpp:144
+        if (CurLoop->contains(Succ) ||
+            !CanProveNotTakenFirstIteration(Succ, DT, CurLoop))
+          return false;
----------------
There's a subtlety here that may justify a comment or code structure change.  Specifically, we don't know that this particular path (out of the possible paths we identified) is taken on the first iteration.  So why is discharging the condition on that particular path on the first iterations sufficient?  Answer: because it implies that either a) this path is taken on the first iteration (and thus the exit isn't) or be another path is taken by the first iteration and by the time we execute this path, the instruction of interest has already executed.


https://reviews.llvm.org/D50558