[PATCH] [LoopInterchange] Add support to interchange loops with reductions.

[James Molloy]

Hi Karthik,

See my inline comments.

Cheers,

James


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:258
@@ -255,3 +257,3 @@
 // Checks if it is legal to interchange 2 loops.
 // [Theorm] A permutation of the loops in a perfect nest is legal if and only if
 // the direction matrix, after the same permutation is applied to its columns,
----------------
s/Theorm/Theorem

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:590
@@ -588,1 +589,3 @@
 } // end of namespace
+bool LoopInterchangeLegality::checkAllUsesAreReductions(Instruction *Ins,
+                                                        Loop *L) {
----------------
Can you rename this "areAllUsesReductions"? it sounds more like a boolean query, which is what this is. "Check" implies some action.

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:595
@@ +594,3 @@
+    ReductionDescriptor RD;
+    if (!UserIns)
+      return false;
----------------
You can merge these two if's:

    if (!UserIns || !ReductionDescriptor::isReductionPHI(UserIns, L, RD))
      return false;

If you wanted to be *really* cool...

    return !std::any(Ins->user_begin(), Ins->user_end(), [](User *U) {
      PHINode *UserIns = dyn_cast<PHINode>(*I);
      ReductionDescriptor RD;
      return UserIns && ReductionDescriptor::isReductionPHI(UserIns, L, RD);
    });

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:696
@@ +695,3 @@
+bool LoopInterchangeLegality::populateInductionAndReductions(Loop *L) {
+  if (L->getLoopLatch() == nullptr || L->getLoopPredecessor() == nullptr)
+    return false;
----------------
just:

    if (!L->getLoopLatch() || !L->getLoopPredecessor())
      return false;


================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:707
@@ +706,3 @@
+    else
+      return false;
+  }
----------------
Probably a good idea to have some debugging output here saying what PHI failed to be recognized?

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:732
@@ +731,3 @@
+  if (BranchInst *BI = dyn_cast<BranchInst>(LatchBlock->getTerminator())) {
+    unsigned Num = BI->getNumOperands();
+    for (unsigned i = 0; i < Num; ++i) {
----------------
Assert that this is 2?

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:757
@@ +756,3 @@
+  // TODO: Currently we handle only loops with 1 induction variable.
+  if (Inductions.size() != 1)
+    return true;
----------------
Some debugging output saying why it failed would be nice

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:760
@@ +759,3 @@
+  InnerInductionVar = Inductions.pop_back_val();
+  Reductions.clear();
+  if (!populateInductionAndReductions(OuterLoop))
----------------
I don't like this, you're mutating the content of the class for no good reason. It would be better to explicitly give Inductions and Reductions (a stack local variable) to populateInductionAndReductions(), and rename it to findInductionAndReductions()

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1063
@@ -983,1 +1062,3 @@
 
+bool LoopInterchangeTransform::hasReductionPHI(Loop *L) {
+  BasicBlock *LoopHeader = L->getHeader();
----------------
You found this out in LoopInterchangeLegality, but threw away the result. Why recalculate it here?

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1081
@@ +1080,3 @@
+  if (hasReductionPHI(InnerLoop)) {
+    // TODO: Check if the induction PHI will always be the first PHI.
+    BasicBlock *New = InnerLoopHeader->splitBasicBlock(
----------------
s/TODO/FIXME

================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1093
@@ +1092,3 @@
+      Value *V = PHI->getIncomingValueForBlock(InnerLoopPreHeader);
+      for (auto UI = PHI->user_begin(), UE = PHI->user_end(); UI != UE; ++UI) {
+        PHI->replaceAllUsesWith(V);
----------------
Just:

    for (auto U : PHI->users())



================
Comment at: lib/Transforms/Scalar/LoopInterchange.cpp:1138
@@ +1137,3 @@
+  for (auto I = CurrBlock->begin(); isa<PHINode>(I); ++I) {
+    PHINode *PHI = dyn_cast<PHINode>(I);
+    unsigned Num = PHI->getNumIncomingValues();
----------------
Needs a bailout if I is not a PHINode.

http://reviews.llvm.org/D8314

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/