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

Mon Apr 29 21:00:17 PDT 2019

jdoerfert added a comment.

I reviewed up to line 380 in LoopInfo.cpp.

================
Comment at: llvm/include/llvm/Analysis/IVDescriptors.h:319
+    return IK == IK_FpInduction && InductionBinOp &&
+           !cast<FPMathOperator>(InductionBinOp)->isFast();
   }
----------------
Why do we need this change? It seems either unrelated, unnecessary, or to complex for me. In the last case, please extend the comment.

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

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:413
+  /// Derived classes can override this method using static template
+  /// polymorphism.
+  BranchInst *getGuard(ScalarEvolution &SE) const { return nullptr; }
----------------
Could you first describe the "desired" function/semantic of this method before you note that it can be overriden?

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:417
+  /// Derived classes can override this method using static template
+  /// polymorphism.
+  PHINode *getInductionVariable(ScalarEvolution &SE,
----------------
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,
----------------
Maybe mention how one is supposed to determine this precondition.

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:599
+    /// iteration.
+    Value &getStepValue() const { return *getStepInst().getOperand(1); }
+
----------------
Do you check, or enforce, that the Step is "canonical", e.g., the step is on the right?
I tried to find this below but I couldn't.

================
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); }
+
----------------
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.

================
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;
----------------
Doesn't this contradict the `getStepInst().getOperand(1);` in line 599?

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:633
+
+    // the initial value of the loop induction variable
+    Value *Init;
----------------
I'd start comments with an upper case letter, again below. (sorry for the nitpicking).

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:640
+    // the latch condition instruction
+    ICmpInst &CmpInst;
+
----------------
Please rename if it is "the latch condition" to sth like "LatchCmpInst".

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:646
+  /// Return the struct LoopBounds collected if all struct members are found,
+  /// else return nullptr.
+  Optional<LoopBounds> getBounds(ScalarEvolution &SE) const;
----------------
else "None".

================
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
----------------
Do you really test that? The initial loop fusion patch did not.

================
Comment at: llvm/include/llvm/Analysis/LoopInfo.h:673
+  /// Return true if the given PHINode \p AuxIndVar is
+  /// - in loop header
+  /// - not used outside of the loop
----------------
"in a" or "in the", but I might be wrong here.

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

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:194
+
+void Loop::LoopBounds::setStepInst(PHINode &IndVar) {
+  InductionDescriptor IndDesc;
----------------
[I don't care too much but I'm interested:] Any reason you separated `setIniitalIVValue` and `setStepInst`?

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:220
+  // StepInst
+  if (CmpInst.getOperand(0) != &getStepInst()) {
+    // Cannot flip strictness of NE and EQ
----------------
I am not sure this is in sync with the constant `getOperand(1)` in one of the headers.

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:224
+      return ICmpInst::getFlippedStrictnessPredicate(Pred);
+    Direction D = getDirection();
+    if (D == Direction::Increasing)
----------------
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?

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:257
+  BasicBlock *Latch = L.getLoopLatch();
+  assert(Latch && "Expecting valid latch");
+
----------------
Couldn't we return nullptr here as well? It seems nullptr is a valid return value here anyway.

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:267
+
+  // First operand of CI should be the StepInst
+  if (isa<Instruction>(CI->getOperand(0)))
----------------
[IMPORTANT] How do we know or enforce this?

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:288
+
+/// Return the closest conditional branch before loop L, where
+/// one successor dominates the loop preheader,
----------------
-"where"
+"if"

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:290
+/// one successor dominates the loop preheader,
+/// and a loop exit block dominates the other successor.
+/// Example:
----------------
"and a definite successor of a loop exit block (trivially) post-dominates the other successor.
The second condition ensures the branch is a control dependence of the loop."

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:306
+/// afterloop:
+/// \endcode
+///
----------------
Maybe we need a more general picture here as the example shows the simple case. I can imagine some "ascii-art" or a more elaborate description.

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:318
+  // the loop preheader.
+  BasicBlock *GuardBB = L.getLoopPreheader();
+  BranchInst *GuardBI = dyn_cast<BranchInst>(GuardBB->getTerminator());
----------------
`if (!GuardBB) return nullptr`

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:319
+  BasicBlock *GuardBB = L.getLoopPreheader();
+  BranchInst *GuardBI = dyn_cast<BranchInst>(GuardBB->getTerminator());
+  if (!GuardBB || !GuardBI)
----------------
GuardBI is overwritten before used.

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:336
+    // A Visited set is needed to records blocks already visited, to avoid
+    // infinite loop when ExitBlock single successor loops back to itself.
+    if (Visited.count(GuardBB) != 0)
----------------
The comment is outdated (ExitBlock)

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:340
+
+    Visited.insert(GuardBB);
+    GuardBI = dyn_cast<BranchInst>(GuardBB->getTerminator());
----------------
We often use the pattern:
```
if (!Visited.insert(GuardBB).second) 
  return nullptr;
```

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

================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:359
+  if (!Cond)
+    return nullptr;
+  GuardPred =
----------------
I'm always for newlines to mark control condition changes

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

================
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 &&
----------------
You could instead call `Visited.clear()` here but I don't mind a new object.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60565