[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 marked 26 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:319
+    return IK == IK_FpInduction && InductionBinOp &&
+           !cast<FPMathOperator>(InductionBinOp)->isFast();
   }
----------------
jdoerfert wrote:
> Whitney wrote:
> > jdoerfert wrote:
> > > Why do we need this change? It seems either unrelated, unnecessary, or to complex for me. In the last case, please extend the comment.
> > I added the check `IK == IK_FpInduction` which is required in the comment `the induction type is FP`. Originally we don't need to check that, because InductionBinOp is not set for non FP inductions, my change below will set it for cases that are not FP.
> Is this unrelated to the main part as the changes wrt setting the InductionBinOp below?
Created https://reviews.llvm.org/D61329 for changes in IVDescriptors.


================
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;
----------------
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.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:652
+  /// - it is the closest conditional branch before the loop
+  /// - one successor dominates the loop preheader
+  /// - a loop exit block dominates the other successor
----------------
kbarton wrote:
> I would have expected the guard to dominate the preheader, and not have another block between the guard and the preheader. Or maybe I misunderstand this point. 
It is possible to have blocks in between the guard and the preheader, the simplest example would be empty blocks. `one successor dominates the loop preheader` ensures that the guard dominate the preheader.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:653
+  /// - one successor dominates the loop preheader
+  /// - a loop exit block dominates the other successor
+  /// - the condition is to verify loop initial value satisfy the latch
----------------
kbarton wrote:
> "a" loop exit block, or "the" loop exit block?
> Do these methods work on loops that have multiple exit blocks?
> If they do work on loops with multiple exit blocks, then I would think that **all** loop exit blocks need to dominate the other successor.
It should work on loops that have multiple exit blocks as well. Why do all loop exit blocks need to dominate the other successor? The guard should just skip the loop, and all loop exit blocks are outside of the loop, as long as one of them dominate the other successor, then it proves the guard skip the entire loop (given that one successor dominate the preheader which is the only entry point to the loop). Please correct me if I am wrong.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:655
+  /// - the condition is to verify loop initial value satisfy the latch
+  ///   condition
+  BranchInst *getGuard(ScalarEvolution &SE) const;
----------------
kbarton wrote:
> What do you mean the loop initial value? 
> 
> I think what you're trying to say is guard condition guarantees the loop body should be executed at least once. The only way I can think to say that using the current terminology is the lower bound of the loop is less than the upper bound (although that may not cover all possible conditions). 
The reason I use the term loop initial value instead of lower bound is the loop maybe decreasing.


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:664
+  /// invariant.
+  PHINode *getInductionVariable(ScalarEvolution &SE,
+                                bool CheckStepInvariant = false) const;
----------------
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?


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:676
+  /// - incremented by a loop invariant step for each loop iteration
+  /// - step instruction opcode should be add or sub
+  /// Note: auxiliary induction variable is not required to be used in the
----------------
kbarton wrote:
> This seems to be more restrictive then the check for induction variable.
> Specifically, not being used outside the loop and the step instruction being an add or sub. 
Right, that's the intensional behaviour


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:678
+  /// Note: auxiliary induction variable is not required to be used in the
+  ///       conditional branch in the loop latch.
+  bool isAuxiliaryInductionVariable(PHINode &AuxIndVar) const;
----------------
kbarton wrote:
> The auxiliary induction variable is not required to be used or cannot be used?
> What happens if it is used in the conditional branch? Can that happen?
It can be used, it is not *required* to be used.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:226
+    if (D == Direction::Increasing)
+      return ICmpInst::ICMP_SLT;
+    if (D == Direction::Decreasing)
----------------
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.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:289
+/// Return the closest conditional branch before loop L, where
+/// one successor dominates the loop preheader,
+/// and a loop exit block dominates the other successor.
----------------
kbarton wrote:
> I'm confused why one successor of the guard should dominate the preheader, not be the preheader itself. 
because it may not be the case, simplest example would be empty blocks


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:331
+    Succ = GuardBB;
+    GuardBB = GuardBB->getSinglePredecessor();
+    if (!GuardBB)
----------------
kbarton wrote:
> This will miss the case where a basic block has multiple predecessors, but all of the predecessors are to the same block.
> I don't think this is a big deal, just pointing it out. I think if you've run simplifycfg prior to this, that should get cleaned up. 
hmmm....in that case, maybe I should use getUniquePredecessor() instead of getSinglePredecessor().


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:370
+    SmallPtrSet<BasicBlock *, 4> Visited;
+    while (ExitBlock && ExitBlock != SkipToBlock &&
+           Visited.count(ExitBlock) == 0) {
----------------
kbarton wrote:
> Is it possible for getUniqueExitBlocks to add null pointers into the ExitBlocks?
> I would have thought this is not possible (i.e., something is wrong if that's happening). Maybe it would be better to assert(ExitBlock && "Expecting valid ExitBlock") before the while loop to catch this if it happens. 
ExitBlock is overriden in the loop `ExitBlock = ExitBlock->getSingleSuccessor();`, so ExitBlock can be nullptr.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:375
+    }
+    if (ExitBlock == SkipToBlock)
+      return GuardBI;
----------------
jdoerfert wrote:
> kbarton wrote:
> > This is just checking that one of the exit blocks goes to the "other" successor of the guard.
> > Do you not need to check that **all** of the exit blocks go to that block?
> That is a good point, it should for sure be stated clearly what is computed here.
The purpose of the guard should just skip the entire loop, given that one successor dominates the loop preheader, which is the only entry point to the loop, then any loop exit block dominates other successor should be enough to prove that it skips the loop, as loop exit blocks are not part of the loop. Please correct me if I am wrong.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:471
+    Value *IncomingValue = PN->getIncomingValueForBlock(Latch);
+    assert(IncomingValue && "Expecting valid incoming value from latch");
+    if (StepInst == IncomingValue) {
----------------
kbarton wrote:
> Doesn't this assert mean the only phis you expect to find in the header are from the latch block?
> Is that always going to be true?
this assert means any phis in the header should have an incoming value from the latch block. This should always be true, because by definition the exists a branch from the latch to the header if latch exists.


================
Comment at: llvm/unittests/Analysis/LoopInfoTest.cpp:33
+/// Build the loop info and scalar evolution for the function and run the Test.
+static void runWithLoopInfoAndSE(
+    Module &M, StringRef FuncName,
----------------
kbarton wrote:
> Did clang-format format this for you?
> The formatting is different then the formatting above. 
That's from clang-format, I guess is because of the longer function name.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565