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

[Whitney via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:732
+  /// dominator tree.
+  BranchInst *getLoopGuardBranch(const DominatorTree &DT,
+                                 const PostDominatorTree &PDT) const;
----------------
Meinersbur wrote:
> 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.
I asked in the last loop group meeting. People mentioned that with the `DomTreeUpdater`, making `PostDom` more widely available should be easier than before. 

Loop-guard normal form may not be the direction we want to go, we are still trying to explore what makes the most sense. 


================
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
----------------
Meinersbur wrote:
> 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? 
> Format each enumeration item on its own line?
Will do.

> Now the other is the loop guard?
Yes, getLoopGuardBranch() returns the 'closest' loop guard. But both branches are consider loop guards to begin with, isLoopGuardBranch() returns true for both branches if they both satisfy the requirements. 

> Also, if was the loop guard of another loop, now it is the loops guard of both?
hmm...only if there is no unsafe instructions guarded by that branch and outside of the loop in consideration. 

> What is an 'unsafe' instruction and why does it stop a block being a loop guard?
For example, instructions that may throw. In general, I would like to make sure if entered the guard region, then the loop is definitely going to be executed. 

> Do you intend to refine the conditions of a loop guards over time?
Yes, depending on the use cases. We will like to refine the conditions to benefit more users.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:400-401
+    SmallVector<const BasicBlock *, 4> WorkList;
+    for (const BasicBlock *Succ : successors(&BeginBB))
+      WorkList.push_back(Succ);
+
----------------
Meinersbur wrote:
> 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.
I should assert that `BeginBB` dominate `Header`, and `EndBB` post dominate `Header`. And right, I was only thinking of the case when there is a preheader, I should also consider when there is no preheader, which means `BeginBB` can be `Header`.


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


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:415
+
+      if (!isGuaranteedToTransferExecutionToSuccessor(Succ))
+        return true;
----------------
Meinersbur wrote:
> 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).
I chose `isGuaranteedToTransferExecutionToSuccessor` instead of `->mayThrow` is because it consider more cases, for example it checks if memory operation trap. `isGuaranteedToTransferExecutionToSuccessor` is a superset of `->mayThrow`. 

What is dynamically endless loops? 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63885