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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.
Herald added a subscriber: wuzish.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:732
+  /// dominator tree.
+  BranchInst *getLoopGuardBranch(const DominatorTree &DT,
+                                 const PostDominatorTree &PDT) const;
----------------
Whitney wrote:
> reames wrote:
> > 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.  
> @Meinersbur @jdoerfert @kbarton Do you know if there is anyone working / plan to work on it?
I don't know anyone currently working on it.

However, if you intend to go into the direction of a loop-guard normal form, why not assuming that the loop guard can only be the block branching to the loop header. Not (Post-)dominator tree needed.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:735-737
+  /// Return true iff 1. one successor of the given branch instruction \p BI
+  /// dominates the loop header, 2. the other successor postdominates the loop
+  /// header, 3. and there doesn't exist unsafe instructions guarded by the
----------------
Format each enumeration item on its own line?

Apart from the conditions tested here, what is your informal definition/purpose of a loop guard?
With these conditions, anything that skips the loop without executing any additional code could be a loop guard. Imagine two blocks dominating the header; this definition would make the 'closest' the loop guard. Let's say the compiler is smart and optimizes it away. Now the other is the loop guard? Also, if was the loop guard of another loop, now it is the loops guard of both?

What is an 'unsafe' instruction and why does it stop a block being a loop guard?

Do you intend to refine the conditions of a loop guards over time? 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:400-401
+    SmallVector<const BasicBlock *, 4> WorkList;
+    for (const BasicBlock *Succ : successors(&BeginBB))
+      WorkList.push_back(Succ);
+
----------------
What if `BeginBB` is (not) the header? If not, it's never added to the worklist. If it is, we are exploring the blocks inside the loop.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:405-406
+      const BasicBlock *Succ = WorkList.pop_back_val();
+      if (Succ == &EndBB)
+        return false;
+
----------------
Early exit? Couldn't it be possible that there are unsafe instructions in the blocks still in the worklist?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:415
+
+      if (!isGuaranteedToTransferExecutionToSuccessor(Succ))
+        return true;
----------------
If you want to disallow throwing instructions, how did  you conclude that `isGuaranteedToTransferExecutionToSuccessor` is what you want and not `->mayThrow`?

There might be dynamically endless loops as well that isGuaranteedToTransferExecutionToSuccessor does not check (that is, only if the endless loop was inside a call).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885