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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D63885#1563469 <https://reviews.llvm.org/D63885#1563469>, @etiotto wrote:

> My opinion is that we should extend the proposed API to identify 'proper' loop guards in addition to weak loop guards.


I like your distinction between weak and proper guards.  We probably need better terminology, but both are useful.  For the moment, I'd suggest we start with the stronger form and add the weaker one later if needed.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:732
+  /// dominator tree.
+  BranchInst *getLoopGuardBranch(const DominatorTree &DT,
+                                 const PostDominatorTree &PDT) const;
----------------
Whitney wrote:
> reames wrote:
> > PostDom is not widely available in the optimizer.  I strongly suspect that a narrower, but more widely usable, API would be better in practice.
> I heard that there is an intension to make PostDom more widely available, and I think that's the right thing to do. What do you think?
It has "been the intention" for years.  Unless someone has committed to doing the work in the next six months, I'd advice not relying on it.  


================
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,
----------------
Whitney wrote:
> reames wrote:
> > Your definition would seem to disallow:
> > if (C) return;
> > for (...) {}
> > 
> > That is C wouldn't be considered a guard condition.  Intentional?  
> I think this is fine, in most cases, LLVM tries to common the return blocks, so `if (C)` will jump to the common exit block, and this algorithm will work. Although we can intensionally write LLVMIR with multiple return blocks, I don't think it will be the common case. 
Put two calls on the return paths to different targets and we don't merge them.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885