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

[Whitney via Phabricator via llvm-commits]

Whitney marked 4 inline comments as done.
Whitney added a comment.

@reames Thanks for the review! I agree that there are still much more to consider for how we want to define a loop guard, to make the most use of it.

What's others opinion on disallow instructions with side effects in between the loop guard block and the loop header?



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:732
+  /// dominator tree.
+  BranchInst *getLoopGuardBranch(const DominatorTree &DT,
+                                 const PostDominatorTree &PDT) const;
----------------
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?


================
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,
----------------
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. 


================
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))
----------------
reames wrote:
> Your skipping over arbitrary throws here.  Is that intended?
> if (C) {
>   foo(); //may throw
>   for (...) {}
> }
This is a good point to discuss, what basic blocks are allowed between loop guard block and the loop header...If we decide to disallow basic blocks that may throw, we can use `isGuaranteedToTransferExecutionToSuccessor()` to check.


================
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));
----------------
reames wrote:
> 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.  
This will almost never work if the loop is in simplified form, as it needs to have dedicated exits. We could check if the second successor one of the loops exit blocks's successor, but this will only work for the most simple cases, e.g. doesn't allow any basic block between the exit block to the 'second successor'. I think instead of not using PDT, we should consider making PDT more widely available. What do you think?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885