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

[Whitney via Phabricator via llvm-commits]

Whitney added inline comments.


================
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;
----------------
kbarton wrote:
> 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?
I updated the comment again, hopefully is more clear this time.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:664
+  /// invariant.
+  PHINode *getInductionVariable(ScalarEvolution &SE,
+                                bool CheckStepInvariant = false) const;
----------------
kbarton wrote:
> 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. 
an instruction is a value as well.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:416
+  /// polymorphism.
+  BranchInst *getGuard(ScalarEvolution &SE) const { return nullptr; }
+
----------------
kbarton wrote:
> 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. 
changed.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:422
+  /// polymorphism.
+  PHINode *getInductionVariable(ScalarEvolution &SE) const { return nullptr; }
+
----------------
kbarton wrote:
> 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. 
Added comment to clarify that.


================
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,
----------------
kbarton wrote:
> Minor comment -the parenthetical should be before the period, not after.  
moved.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:240
+  else if (SE.getSCEV(StepInstOp0) == Step)
+    StepValue = StepInstOp0;
+
----------------
kbarton wrote:
> 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?
added.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:348
+/// \endcode
+///
+/// - getPotentialGuard --> if (guardcmp) goto preheader; else goto afterloop
----------------
kbarton wrote:
> 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. 
Updated the comment.


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


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:418
+      return GuardBI;
+  }
+
----------------
jdoerfert wrote:
> kbarton wrote:
> > 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?
> > And the "other" successor of the potential guard block should post-dominate all of the blocks inside the loop. 
> 
> Not all blocks but just the header I think. That should allow multi-exit loops (if it matters).
> 
> > I believe that should be sufficient to guarantee the block is a potential guard.
> 
> Agreed.
I changed the code to use the post dominator, but there is no need to use the dominator, as by finding the closest conditional branch, we know it dominates the loop.


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


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:226
+    if (D == Direction::Increasing)
+      return ICmpInst::ICMP_SLT;
+    if (D == Direction::Decreasing)
----------------
kbarton wrote:
> 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?
That's not totally true, it returns the signed predicate for ne or eq which doesn't have a sign. Updated the comment to reflect that.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565