[PATCH] D60565: [LOOPINFO] Extend Loop object to add utilities to get the loop bounds, step, induction variable, and guard branch.

[Kit Barton via Phabricator via llvm-commits]

kbarton marked an inline comment as done.
kbarton added a comment.

I discussed this with @Whitney and I think at this point it makes sense to remove the getGuard and getPotentialGuard methods as there still seems to be some discussions about the right approach for that.
Aside from that, I think everything else looks pretty good, aside from some specific style-related comments.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:574
+    /// Return the LoopBounds object if
+    /// - the given \p IndVar is a induction variable
+    /// - the initial value of the induction variable can be found
----------------
a -> an


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:673
+  /// 2. one successor dominates the loop preheader
+  /// 3. a loop exit block dominates the other successor
+  /// 4.  the condition is to verify the loop initial induction variable value
----------------
I think this condition needs to be reworded to:
The other successor post-dominates all nodes in the loop. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:249
+
+  BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
+  assert(BI && BI->isConditional() && "Expecting conditional latch branch");
----------------
Whitney wrote:
> kbarton wrote:
> > Should this be dyn_cast_or_null instead, to account for the possibility that getTerminator could returns nullptr?
> Can a basic block not have a terminator? I thought it must have one.
From: http://llvm.org/doxygen/classllvm_1_1BasicBlock.html

> A well formed basic block is formed of a list of non-terminating instructions followed by a single terminator instruction. Terminator instructions may not occur in the middle of basic blocks, and must terminate the blocks. The BasicBlock class allows malformed basic blocks to occur because it may be useful in the intermediate stage of constructing or modifying a program. However, the verifier will ensure that basic blocks are "well formed".

So, while I agree it is unlikely to happen, it is still possible, depending on when this is called wrt other optimizations. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:294
+
+Direction Loop::LoopBounds::getDirection() const {
+  const SCEVAddRecExpr *StepAddRecExpr =
----------------
>From what I have seen, a more common way to write this method in LLVM-style would be:

  if (SCEVAddRecExpr *StepAddRecExpr = dyn_cast<SCEVAddRecExpr>(SE.getSCEV(&getStepInst()))) 
    if (const SCEV *StepRecur = StepAddRecExpr->getStepRecurrence(SE)) {
      if (SE.isKnownPositive(StepRecur))
        return Direction::Increasing;
      if (SE.isKnownNegative(StepRecur))
        return Direction::Decreasing;
    }
  return Direction::Unknown;


That said, I don't know if this is explicitly documented anywhere. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:311
+
+Optional<Loop::LoopBounds> Loop::getBounds(ScalarEvolution &SE) const {
+  PHINode *IndVar = getInductionVariable(SE);
----------------
Similarly, this is typically written as:

  if (PHINode *IndVar = getInductionVariable(SE))
    return LoopBounds::getBounds(*this, *IndVar, SE);
  return None;


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:528
+bool Loop::getInductionDescriptor(ScalarEvolution &SE,
+                                  InductionDescriptor &IndDesc) const {
+  PHINode *IndVar = getInductionVariable(SE);
----------------
Same as above. This can be rewritten by initializing IndVar in the if condition and then calling isInductionPHI.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565