[PATCH] D106580: [LoopFlatten] Use SCEV and Loop APIs to identify increment and trip count

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:74
 
 struct FlattenInfo {
   Loop *OuterLoop = nullptr;
----------------
Perhaps we can document some assumptions and limitations here.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:77
   Loop *InnerLoop = nullptr;
   PHINode *InnerInductionPHI = nullptr;
   PHINode *OuterInductionPHI = nullptr;
----------------
This corresponds to a loop induction variable, which we expect to have a start value of 0.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:158
+  // latch value is the increment variable.
   Increment = nullptr;
+  if (L->isCanonical(*SE))
----------------
Perhaps I have a slight preference here for:

  f (!L->isCanonical(*SE))
    return false;

so we don't need the `Increment = nullptr;` here.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:159
   Increment = nullptr;
-  if (match(Compare->getOperand(0),
-            m_c_Add(m_Specific(InductionPHI), m_ConstantInt<1>()))) {
-    Increment = dyn_cast<BinaryOperator>(Compare->getOperand(0));
-    Limit = Compare->getOperand(1);
-  } else if (Compare->getUnsignedPredicate() == CmpInst::ICMP_NE &&
-             match(Compare->getOperand(1),
-                   m_c_Add(m_Specific(InductionPHI), m_ConstantInt<1>()))) {
-    Increment = dyn_cast<BinaryOperator>(Compare->getOperand(1));
-    Limit = Compare->getOperand(0);
-  }
+  if (L->isCanonical(*SE))
+    Increment =
----------------
Ah, nice one, `isCanonical` checks exactly what we were assuming and pattern matching: it checks that the loop starts at 0 and increments by 1. Do we need the `isZero()` check above?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:162
+        dyn_cast<BinaryOperator>(InductionPHI->getIncomingValueForBlock(Latch));
   if (!Increment || Increment->hasNUsesOrMore(3)) {
+    LLVM_DEBUG(dbgs() << "Could not find valid increment\n");
----------------
Then, this can just be:

  if (Increment->hasNUsesOrMore(3))


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

https://reviews.llvm.org/D106580