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

[Whitney via Phabricator via llvm-commits]

Whitney marked 9 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:409
+
+      WorkList.append(successors(Succ).begin(), successors(Succ).end());
+    }
----------------
reames wrote:
> 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.  
I will think about how to simplified this further at least for the first patch.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:417
+  for (BasicBlock *Pred : predecessors(Header)) {
+    if (contains(Pred))
+      continue;
----------------
reames wrote:
> 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.  
I am trying to handle the case where there doesn't exists a preheader. I can modified the function to only allow loop with preheader.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:432
+    BasicBlock *Succ1 = BI->getSuccessor(1);
+    BasicBlock *ExitBlock = getExitBlock();
+    BasicBlock *ExitSucc =
----------------
reames wrote:
> Loops frequently have more than one exit block, at least an early return is needed here.
I thought nullptr is allowed in comparison. I still want to continue when ExitBlock is nullptr, as it maybe able to use PDT (if given) for multi exit blocks loop.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885