[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 accepted this revision.
kbarton added a comment.
This revision is now accepted and ready to land.

Aside from a few minor comments, this LGTM.



================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:535
 
+  /// Below are some utilities to get loop bounds, guard branch, and induction
+  /// variable, and check if a given phinode is an auxiliary induction variable,
----------------
Need to remove guard branch from the comments, as that is not included in this patch anymore. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:294
+
+Direction Loop::LoopBounds::getDirection() const {
+  const SCEVAddRecExpr *StepAddRecExpr =
----------------
Whitney wrote:
> kbarton wrote:
> > 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. 
> I thought they prefer early exit. https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
Yes, that's true. But that is dealing more with large bodies of code and/or complicated conditionals where many things need to be satisfied. I think the purpose of that is it is very hard to keep all of the logic straight in your head while reading the code, so separating out common conditions or early exits makes the code easier to read.  These functions, on the other hand, are very small and do not have a lot of conditions or context that needs to be remembered. 

I agree this is somewhat subjective, and I don't have a strong preference either way. However, after looking at a lot of code in LLVM, I find the more compact style easier to read for small functions. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:197
+
+  BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
+  if (!BI || !BI->isConditional())
----------------
This should also be dyn_cast_or_null, since Latch->getTerminator() can return nullptr in some cases. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565