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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

I'm not sure an API this general is the most relevant.  I would have expected something more tightly tied to the loop itself instead of the region of code which contains the loop.  Thinking about how transforms might want to use this, I suspect a much tighter definition would be useful.

Consider a transform which figures out a condition C under which loop L is a noop.  Modifying the guard (as defined here) would not be valid, but a guard which controls the preheader directly w/no side effects possible inside the guarded region, but outside the loop would be exactly what was needed.  Not sure what other transforms need off the top of my head, but I think that needs to be explored.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:732
+  /// dominator tree.
+  BranchInst *getLoopGuardBranch(const DominatorTree &DT,
+                                 const PostDominatorTree &PDT) const;
----------------
PostDom is not widely available in the optimizer.  I strongly suspect that a narrower, but more widely usable, API would be better in practice.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:737
+  /// dominates the loop header, and the other successor postdominates the loop
+  /// header, i.e. the branch instruction \p BI is a loop guard.
+  bool isLoopGuardBranch(const BranchInst &BI, const DominatorTree &DT,
----------------
Your definition would seem to disallow:
if (C) return;
for (...) {}

That is C wouldn't be considered a guard condition.  Intentional?  


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:372
+    BasicBlock *IDomBB = IDomNode->getBlock();
+    if (BranchInst *BI = dyn_cast_or_null<BranchInst>(IDomBB->getTerminator()))
+      if (isLoopGuardBranch(*BI, DT, PDT))
----------------
Your skipping over arbitrary throws here.  Is that intended?
if (C) {
  foo(); //may throw
  for (...) {}
}


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:392
+  BasicBlock *Header = getHeader();
+  return (DT.dominates(Succ0, Header) && PDT.dominates(Succ1, Header)) ||
+         (DT.dominates(Succ1, Header) && PDT.dominates(Succ0, Header));
----------------
A quick version of this which doesn't need PDT would be to check if the second successor was one of the loops exit blocks.  


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885