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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:319
+    return IK == IK_FpInduction && InductionBinOp &&
+           !cast<FPMathOperator>(InductionBinOp)->isFast();
   }
----------------
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?


================
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,
----------------
Whitney wrote:
> 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?
No, keep it.

However, later, e.g., in `setStepInst`, you `assert(isInductionPHI(IndVar))` or something similar. The comment states this assertion in words but it could mention the `isInductionPHI` function which allows to verify the precondition outside of the `LoopBounds`.


================
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); }
+
----------------
Whitney wrote:
> jdoerfert wrote:
> > kbarton wrote:
> > > Same - this is assuming a very specific format for the compare. Is it guaranteed to be true in all cases?
> > 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().
`getCmpInst()` looks for an instruction on the LHS, that does not mean anything on its own.



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


================
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);
----------------
Whitney wrote:
> 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.
> 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.

You are right, it should work, no need to check for nullptr but you can.

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

Thanks.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:194
+
+void Loop::LoopBounds::setStepInst(PHINode &IndVar) {
+  InductionDescriptor IndDesc;
----------------
Whitney wrote:
> 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.
If that is the only reason I'd merge them into an initialize method or sth.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:220
+  // StepInst
+  if (CmpInst.getOperand(0) != &getStepInst()) {
+    // Cannot flip strictness of NE and EQ
----------------
Whitney wrote:
> 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.
Probably/Hopefully, correct.


================
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());
----------------
Whitney wrote:
> 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)
You are obviously right here, ... 

I should not do code reviews at night...


================
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 &&
----------------
Whitney wrote:
> 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.
No strong feeling, keep it.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:375
+    }
+    if (ExitBlock == SkipToBlock)
+      return GuardBI;
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565