[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 14 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:
> 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.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:327
+      return nullptr;
+
     if (!InductionBinOp || cast<FPMathOperator>(InductionBinOp)->isFast())
----------------
jdoerfert wrote:
> Same as above
same as above


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:577
+    /// LoopBounds can only be created if an induction variable can be
+    /// identified.
+    LoopBounds(const Loop &Loop, PHINode &IndVar, ICmpInst &C,
----------------
jdoerfert wrote:
> Maybe mention how one is supposed to determine this precondition.
As LoopBounds take a reference of IndVar, meaning that LoopBounds can only be created if an induction variable. Is it better if I remove the comment?


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:602
+    /// Get the final value of the loop induction variable.
+    Value &getFinalIVValue() const { return *CmpInst.getOperand(1); }
+
----------------
jdoerfert wrote:
> Do you check, or enforce, that the ICmpInst is "canonical", e.g., the bound is on the right?
> I tried to find this below but I couldn't.
> 
It is enforced by getCmpInst().


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


================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:654
+  /// - a loop exit block dominates the other successor
+  /// - the condition is to verify loop initial value satisfy the latch
+  ///   condition
----------------
jdoerfert wrote:
> Do you really test that? The initial loop fusion patch did not.
I may not have understand the question correctly. getGuard() checks if the condition satisfy the latch condition by comparing the operands and the predicate of the branch instruction with the bounds of the loop.



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:1057
+  BinaryOperator *BOp = dyn_cast_or_null<BinaryOperator>(
+      Phi->getIncomingValueForBlock(AR->getLoop()->getLoopLatch()));
   const SCEV *Step = AR->getStepRecurrence(*SE);
----------------
jdoerfert wrote:
> Does this work if there is no loop latch? I have the feeling `Phi->getIncomingValueForBlock` might choke on a nullptr. I might be wrong though.
> 
> I feel the changes in this file are unrelated and can go before the rest. If you agree, open a new patch for them, maybe with a test, and I'll accept it as it is pretty straight forward.
> 
> 
I was trying to follow the similar semantic as the line above. However, I agree that it is a good idea to check if the latch is nullptr first.

I will open a new patch with the changes on IVDescriptors.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:194
+
+void Loop::LoopBounds::setStepInst(PHINode &IndVar) {
+  InductionDescriptor IndDesc;
----------------
jdoerfert wrote:
> [I don't care too much but I'm interested:] Any reason you separated `setIniitalIVValue` and `setStepInst`?
The reason is there are getInitialIVValue() and getStepInst(). I don't have strong feeling either ways.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:220
+  // StepInst
+  if (CmpInst.getOperand(0) != &getStepInst()) {
+    // Cannot flip strictness of NE and EQ
----------------
jdoerfert wrote:
> I am not sure this is in sync with the constant `getOperand(1)` in one of the headers.
I think you are talking about the getOperand(1) in getStepInst(), which is looking at the StepInst, and here we are looking at the LatchCmpInst.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:224
+      return ICmpInst::getFlippedStrictnessPredicate(Pred);
+    Direction D = getDirection();
+    if (D == Direction::Increasing)
----------------
jdoerfert wrote:
> Why do you have the non-SE path above for certain special cases e.g.,. non-equality conditions, before you have a, what seems to be, general SE based handling?
when the predicate is not ne and not eq, then there is no need to use SE, we can simply flip the strictness of the predicate. 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:356
+  assert((SkipIndex == 1 || GuardBI->getSuccessor(1) == Succ) &&
+         "Expecting one of GuardBI successor to be Succ");
+  ICmpInst *Cond = dyn_cast<ICmpInst>(GuardBI->getCondition());
----------------
jdoerfert wrote:
> Why do we "compute" the index of the Successor if we assert it is 1 anyway? This seems like a specialization of a more general scheme.
There is not just asserting the index of the successor is 1, it is asserting either the successor which skip the loop is 1 or `Succ` is index 1 (i.e. the successor which skip the loop is 0)


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:360
+    return nullptr;
+  GuardPred =
+      (SkipIndex == 1) ? Cond->getPredicate() : Cond->getInversePredicate();
----------------
jdoerfert wrote:
> This is the general version (see above) again.
see above reply.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:369
+    // infinite loop when ExitBlock single successor loops back to itself.
+    SmallPtrSet<BasicBlock *, 4> Visited;
+    while (ExitBlock && ExitBlock != SkipToBlock &&
----------------
jdoerfert wrote:
> You could instead call `Visited.clear()` here but I don't mind a new object.
I prefer a new object, because we don't need to keep track if the live range of the other `Visited`. If you don't have a strong feeling, I will keep it as is.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565