[PATCH] D63885: [LOOPINFO] Introduce the loop guard API.

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Please, please, please, reduce the complexity of this patch.  Start with the simplest possible implementation which handles one trivial case.  Get that in with the interface, then incrementally build on top of that.  You are stuck in a review cycle where there's enough different aspects of this patch that we will keep finding problems for many more iterations.  You best (and possibly only) way out of that is to reduce scope until there's nothing left that anyone can possibly object to, then build incrementally off the agreed baseline!



================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:409
+
+      WorkList.append(successors(Succ).begin(), successors(Succ).end());
+    }
----------------
This code is overly general and confusingly so.  Walking arbitrary successors is exploring an overly large portion of the CFG, even for the PDT case!

It's possible there's something about the way it's called which makes this not so, but a quick skim of the code doesn't make that obvious, so there's a readability problem here at least.  


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:417
+  for (BasicBlock *Pred : predecessors(Header)) {
+    if (contains(Pred))
+      continue;
----------------
This appears to be trying to handle the case where a loop has multiple predecessors outside of a loop.  DONT.  Use getPredecessor and let this all simplify.  


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:432
+    BasicBlock *Succ1 = BI->getSuccessor(1);
+    BasicBlock *ExitBlock = getExitBlock();
+    BasicBlock *ExitSucc =
----------------
Loops frequently have more than one exit block, at least an early return is needed here.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63885/new/

https://reviews.llvm.org/D63885