[PATCH] D22630: Loop rotation

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin requested changes to this revision.
mzolotukhin added a comment.
This revision now requires changes to proceed.

Hi,

Please find some comments inline. Also, please reinspect the tests - in many of them the comments don't correspond to test bodies, they are not minimal, and some tests duplicate others.

Thanks,
Michael



================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:149
+  const BasicBlock *OrigHeader = L->getHeader();
   if (!L->isLoopExiting(OrigHeader))
     return false;
----------------
mzolotukhin wrote:
> hiraditya wrote:
> > eli.friedman wrote:
> > > Did you mean to change this check? Given the way you've implemented cloning, you should be able to do something more aggressive.
> > The new implementation can rotate the loop as far as user wants. This check is here because currently we copy until we find a basic block which is not exiting. I guess if there are use cases where a very general copying is helpful, we can remove this check.
> I think this check is not needed now. What we want to look at is the loop edge (and we do it just before this check) - loop rotation should make it exiting. But even if you want to keep this check, the comment needs to be updated as it was written for the limitations of the old implementation.
Why do we need this check?


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:19
+// - The loop is an SEME (Single Entry Multiple Exit) region.
+// - The loop has a header and its terminator conditionally exits the loop.
+// - The loop has a latch and its terminator does not exit the loop.
----------------
Why do we need this requirement?


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:24
+// Post conditions:
+// The dominator tree is updated during loop rotation
+// If the loop was already in loop-simplify form then that property is preserved
----------------
Please end sentences with dots.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:108-110
+static bool isSingleEntrySingleExit(const BasicBlock *Entry,
+                                    const BasicBlock *Exit,
+                                    DominatorTree *DT, DominatorTree *PDT) {
----------------
This function seems to be unused.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:175-176
 
-        // Users in the OrigPreHeader need to use the value to which the
-        // original definitions are mapped.
-        if (UserBB == OrigPreheader) {
-          U = OrigPreHeaderVal;
-          continue;
-        }
-      }
+  bool isLoopSizeWithinLimits(const SmallVecBB &Blocks,
+                            const SmallPtrSetBB &Exits) const;
 
----------------
Formatting is slightly off here.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:205-208
+  // The header may have invoke instruction to BBs within the loop.
+  const BranchInst *BI = dyn_cast<const BranchInst>(OrigH->getTerminator());
+  if (!BI || !BI->isConditional())
+    return false;
----------------
I don't understand how this comment corresponds to the code.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:270-273
+PHINode *LoopRotate::getOrCreatePHI(Instruction *Inst, BasicBlock *NewHeader,
+                                    BasicBlock *NewPreheader,
+                                    BasicBlock *NewLatch,
+                                    ValueToValueMapTy &VMap) const {
----------------
Please add a comment about what this function is doing and what it expects.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:319
+          // Handle uses in the loop-closed-phi.
+          PHINode *ClosePhi = cast<PHINode>(UserInst);
+          BasicBlock *Pred = ClosePhi->getIncomingBlock(U.getOperandNo());
----------------
Why is UserInst guaranteed to be a PHINode?


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:356-357
+// (bounded by \p OrigH and \p OrigLatch), which are exiting the loop in
+// \p Blocks and collect all the exits from the region in \p Exits. Returns the
+// first basic block which was not copied.
+BasicBlock *LoopRotate::collectSEMEBlocks(BasicBlock *OrigH,
----------------
I find it a bit confusing that you're saying about blocks being copied while the function in fact just populates the list.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:369
 
-        // If the dominator changed, this may have an effect on other
-        // predecessors, continue until we reach a fixpoint.
-      } while (Changed);
+    // Copy until any BB where the branch does not exit loop, or the loop-latch.
+    if (OrigLatch == *BB || !L->isLoopExiting(*BB) ||
----------------
I think this check is the reason you kept that `OrigH->isExiting()` check. I think that what we rather should do here is to copy all blocks up to an exiting block that we're going to make a new latch. Does it make sense?

In other words, I'd like the following case to be rotated:
Entry -> {A}
A -> {B}
B -> {Exit, C}
C -> {A}

I see that current implementation is generic enough to handle such cases, and the only reasons why it won't do it is that (IMHO unnecessary) limitations.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:410
+        else
+          // When no mapping is available it must be a constant.
+          PN->addIncoming(Op, CopyBB);
----------------
...a constant, a function argument, or a global.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:444-445
+    Loop *BBParentLoop = BBLoop->getParentLoop();
+    if (BBParentLoop)
+      BBParentLoop->addBasicBlockToLoop(NewBB, *LI);
+    VMap[BB] = NewBB;
----------------
Why don't we need to update LI if there is no parent loop?


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:494
 
-    if (isa<DbgInfoIntrinsic>(I))
-      continue;
+typedef SmallPtrSetImpl<BasicBlock *> SmallPtrSetBB;
+void dump(const SmallPtrSetBB& e) {
----------------
You have the same typedef inside LoopRotate.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:495-498
+void dump(const SmallPtrSetBB& e) {
+  for (auto BB: e)
+    dbgs() << *BB << "\n";
+}
----------------
If it's not used anywhere, please remove it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:604
+        DEBUG(dbgs() << "\nChanging IDom of " << *ExitBB << "to" << *NewIDom);
+        I = DTBB->begin();
+      } else
----------------
Can we avoid quadratic complexity here?


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:73
+
+; Check that the loop with indirect branches is rotated.
+; CHECK-LABEL: @foo3
----------------
This function doesn't have any indirect branches.


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:149-153
+bb10:
+  switch i32 %arg3, label %bb11 [
+    i32 46, label %bb19
+    i32 32, label %bb19
+  ]
----------------
Why do we need two switches in this test?


https://reviews.llvm.org/D22630