[PATCH] D114917: [LoopInterchange] Enable loop interchange with multiple inner loop indvars

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:338
+  bool isLoopStructureUnderstood(
+      const SmallVectorImpl<PHINode *> &InnerInductionVar);
 
----------------
[nit] InnerInductionVar -> InnerInductionVars


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:647
 bool LoopInterchangeLegality::isLoopStructureUnderstood(
-    PHINode *InnerInduction) {
-  unsigned Num = InnerInduction->getNumOperands();
+    const SmallVectorImpl<PHINode *> &Inductions) {
   BasicBlock *InnerLoopPreheader = InnerLoop->getLoopPreheader();
----------------
suggestion: Inductions ->InnerInductions


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:696
     std::function<bool(Value *)> IsPathToIndVar;
-    IsPathToIndVar = [&InnerInduction, &IsPathToIndVar](Value *V) -> bool {
-      if (V == InnerInduction)
+    IsPathToIndVar = [&Inductions, &IsPathToIndVar](Value *V) -> bool {
+      if (std::find(Inductions.begin(), Inductions.end(), V) !=
----------------
suggestion: `Value *V` -> `const Value *V`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:697-698
+    IsPathToIndVar = [&Inductions, &IsPathToIndVar](Value *V) -> bool {
+      if (std::find(Inductions.begin(), Inductions.end(), V) !=
+          Inductions.end())
         return true;
----------------
suggestion: use `llvm::is_contained()`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:716
+    if (IsPathToIndVar(Op0) && IsPathToIndVar(Op1))
+      return true;
+
----------------
What if we have a loop condition like this:
```
for (...)
  for (; i < j; i++, j--)
```

I'm not sure if this would be legal to interchange (need to think about it more). I would have expected we return false if both left and right are functions of inner-ivs, to be conservatively correct.

Should have a test for this case also.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:892
 
-  // TODO: Current limitation: Since we split the inner loop latch at the point
-  // were induction variable is incremented (induction.next); We cannot have
-  // more than 1 user of induction.next since it would result in broken code
-  // after split.
-  // e.g.
-  // for(i=0;i<N;i++) {
-  //    for(j = 0;j<M;j++) {
-  //      A[j+1][i+2] = A[j][i]+k;
-  //  }
-  // }
-  Instruction *InnerIndexVarInc = nullptr;
-  if (InnerInductionVar->getIncomingBlock(0) == InnerLoopPreHeader)
-    InnerIndexVarInc =
-        dyn_cast<Instruction>(InnerInductionVar->getIncomingValue(1));
-  else
-    InnerIndexVarInc =
-        dyn_cast<Instruction>(InnerInductionVar->getIncomingValue(0));
-
-  if (!InnerIndexVarInc) {
-    LLVM_DEBUG(
-        dbgs() << "Did not find an instruction to increment the induction "
-               << "variable.\n");
-    ORE->emit([&]() {
-      return OptimizationRemarkMissed(DEBUG_TYPE, "NoIncrementInInner",
-                                      InnerLoop->getStartLoc(),
-                                      InnerLoop->getHeader())
-             << "The inner loop does not increment the induction variable.";
-    });
-    return true;
+  InnerLoopInductions = Inductions;
+  SmallPtrSet<Instruction *, 8> InnerIndexVarIncs;
----------------
why do we need this copy?


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1401
+              UserI->getParent() == NewLatch ||
+              std::find(InductionPHIs.begin(), InductionPHIs.end(), UserI) !=
+                   InductionPHIs.end())
----------------
use `llvm::is_contained()`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1411
               this->LI->getLoopFor(OpI->getParent()) != this->InnerLoop ||
-              OpI == InductionPHI)
+              std::find(InductionPHIs.begin(), InductionPHIs.end(), OpI) !=
+                  InductionPHIs.end())
----------------
use `llvm::is_contained()`


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:1729
+      continue;
+    InnerLoopPHIs.push_back(cast<PHINode>(&PHI));
+  }
----------------
Why did we need to check `OuterInnerReductions` before and not anymore? 


================
Comment at: llvm/test/Transforms/LoopInterchange/interchangeable-innerloop-multiple-indvars.ll:16
+;;     e = 5;
+;;     for (; d, e; d--, e--)
+;;       a |= b[d][c + 9];
----------------
The loop latch in the IR only uses one IV anyway, so can we simplify this to only use one IV for the condition? 
Please also add a test where more than one IV is used in the latch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114917