[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 llvm-commits at lists.llvm.org
Mon May 13 12:55:40 PDT 2019


kbarton requested changes to this revision.
kbarton added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:416
+  /// polymorphism.
+  BranchInst *getGuard(ScalarEvolution &SE) const { return nullptr; }
+
----------------
Minor comment, but I think it's better to make the names as specific as possible. So, this could be getGuardBranch, instead of just getGuard, to distinguish between getGuardBlock for example. 


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:422
+  /// polymorphism.
+  PHINode *getInductionVariable(ScalarEvolution &SE) const { return nullptr; }
+
----------------
How is this method different from getCanonicalInductionVariable?
If these are fundamentally different, then the comments need to be clear what the differences are and when one should be used over the other. 


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:575
+  /// isAuxiliaryInductionVariable(x) --> true if x == i_1
+  /// isCanonical                   --> false
+  struct LoopBounds {
----------------
This is minor, but have you tried running this through doxygen to see what the output looks like?
It may come out looking much different in the doxygen pages then it does here. 


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:682
+  /// Note: auxiliary induction variable is not required to be used in the
+  ///       conditional branch in the loop latch. (but it can be)
+  bool isAuxiliaryInductionVariable(PHINode &AuxIndVar,
----------------
Minor comment -the parenthetical should be before the period, not after.  


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:606
+    /// - the first successor of the latch branch is the loop header
+    /// - the LHS of the latch comparison is StepInst
+    ICmpInst::Predicate getCanonicalPredicate() const;
----------------
Whitney wrote:
> jdoerfert wrote:
> > Whitney wrote:
> > > jdoerfert wrote:
> > > > kbarton wrote:
> > > > > Can you clarify this comment?
> > > > > Does this method assume:
> > > > >   1. The first successor of the latch branch is the loop header
> > > > >   2. The LHS of the latch comparison is the StepInst?
> > > > > and if both of these conditions are not met, then what does it return?
> > > > Doesn't this contradict the `getStepInst().getOperand(1);` in line 599?
> > > in line 599, we are looking at the instruction which update the induction variable, and getting the step. While here we are talking about the latch compare instruction, and checking if operand(1) is the step instruction.
> > OK. 
> It returns the predicate when the two points are satisfied. 
> e.g. if the second successor of the latch latch is the loop header instead of the first, then  it returns the inverse predicate.
> I will modify the comment to clarify that.
I think this comment needs to be clarified further as I find it confusing the way it is written. 
In particular, the mixing of itemization (before the example) and enumeration (after the example) . Can you rewrite it to make it more clear?


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:664
+  /// invariant.
+  PHINode *getInductionVariable(ScalarEvolution &SE,
+                                bool CheckStepInvariant = false) const;
----------------
Whitney wrote:
> kbarton wrote:
> > I find this very confusing.
> > I would have expected the getInductionVariable to return a Value, not an instruction. Strictly speaking, I don't think a PHINode is considered a variable.
> > 
> > What is the utility of returning the PHINode here instead of the actual Value?
> > 
> > 
> An existing function getCanonicalInductionVariable() also return PHINode *. Which value do you mean by the actual Value?
Yes, I saw that now. Perhaps I'm just being too pedantic on the terminology that is being used. 
I would have expected a method that returns a "Variable" to be a Value, not a PHINode (an instruction). However, you can get to the Value from the PHINode, so it's probably OK.

If others have strong feelings about this on way or another, please comment. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:226
+    if (D == Direction::Increasing)
+      return ICmpInst::ICMP_SLT;
+    if (D == Direction::Decreasing)
----------------
Whitney wrote:
> kbarton wrote:
> > kbarton wrote:
> > > How do you know to use the signed version, instead of the unsigned version?
> > Do you handle the ULE/SLE case anywhere?
> getCanonicalPredicate() is used by getGuard() which get the Signed Predicate for both the latch and the potential guard to compare, so it doesn't matter if SLE or ULE is returned.
Can you make it explicit in the comments for getCanonicalPredicate that it will always return the signed predicate, even if the latch uses the unsigned version?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:249
+
+  BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
+  assert(BI && BI->isConditional() && "Expecting conditional latch branch");
----------------
Should this be dyn_cast_or_null instead, to account for the possibility that getTerminator could returns nullptr?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:240
+  else if (SE.getSCEV(StepInstOp0) == Step)
+    StepValue = StepInstOp0;
+
----------------
If neither of these conditions are true, LoopBounds will be initialized with a null StepValue. I'm assuming this is intended behaviour.

Can you please add a comment to the getStepValue method that says it will return nullptr if the StepValue cannot be determined?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:348
+/// \endcode
+///
+/// - getPotentialGuard --> if (guardcmp) goto preheader; else goto afterloop
----------------
I don't understand the comments below, specifically what the --> is meant to indicate.
I think it should be sufficient to say the method returns a branch instruction that branches around the specified loop.
Also, the GuardBB, SkipIndex, SkipToBlock, etc., are all local variables that don't escape this method, so they are typically not documented in the function comments. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:355
+///    - exit dominates afterloop
+static BranchInst *getPotentialGuard(const Loop &L,
+                                     ICmpInst::Predicate &GuardPred) {
----------------
Can you rename this method to indicate you are returning a branch instruction (e.g., getPotentialGuardBranch). 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:418
+      return GuardBI;
+  }
+
----------------
In the comments you describe the guard in terms of dominance, but you don't use dominance anywhere in the function. 

I would suggest trying to refactor this as follows:

Change this method to getPotentialGuardBlock, that returns the basic block that (potentially) guards the loop body.
I think you can determine the guard block using dominators and post-dominators. Specifically, the guard block should be the closest predecessor that dominates the loop preheader. And the "other" successor of the potential guard block should post-dominate all of the blocks inside the loop. I believe that should be sufficient to guarantee the block is a potential guard.

Then you can get the branch from the guard block in the getGuard method, and do the necessary analysis on the branch itself to verify it conforms with the requirements for a guard branch. Does this make sense?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:423
+
+BranchInst *Loop::getGuard(ScalarEvolution &SE) const {
+  if (!isLoopSimplifyForm())
----------------
Again, try to make it clear in the name what you are specifically getting - in this case the guard branch. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:488
+  BasicBlock *Header = getHeader();
+  BasicBlock *Latch = getLoopLatch();
+  ICmpInst *CmpInst = getLatchCmpInst(*this);
----------------
It's probably reasonable here to assert that Header and Latch are not nullptr. 


================
Comment at: llvm/unittests/Analysis/LoopInfoTest.cpp:752
+      });
+}
----------------
These tests look good. I don't see any tests for the following - could you add some:

1. Loop Bounds and associated methods you have added
2. Unguarded loops (i.e., loops with constant upper bounds should not get a guard).
3. Loop nests
4. Functions with control flow that do not "guard" the loop.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565





More information about the llvm-commits mailing list