[PATCH] D17203: [LICM] Hoist and sink entire inner loops.

[Philip Reames via llvm-commits]

reames added a comment.

More random comments.  Note that this is likely fairly far from submission in it's current form.  You might want to think about options for splitting this up so that we don't get caught in a long review cycle.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:120
@@ -119,3 +120,2 @@
     void getAnalysisUsage(AnalysisUsage &AU) const override {
-      AU.setPreservesCFG();
       AU.addRequired<DominatorTreeWrapperPass>();
----------------
You'll need to enumerate which passes are preserved without this.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:351
@@ +350,3 @@
+  // Only loops at level n+1.
+  if (SubLoop->getParentLoop() != CurLoop)
+    return false;
----------------
This should be checked by the caller.  Possible in that helper function: isHeaderOfImmediateSubLoop?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:354
@@ +353,3 @@
+
+  // Don't hoist if it has multiple exit or entry blocks.
+  if (!SubLoop->getExitBlock() || !SubLoop->getLoopPreheader()) {
----------------
Given these two sets of checks are repeated, a helper function would be good.  Alternatively, is isLoopSimplifyForm sufficient?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:361
@@ +360,3 @@
+
+  if (!DT->dominates(SubLoop->getHeader(), CurLoop->getExitBlock())) {
+    DEBUG(dbgs() << "Not hoisting subloop: " << SubLoop->getHeader()->getName()
----------------
Do we have any guarantee at this point that CurLoop has only one exit?  If so, assert it.  

================
Comment at: lib/Transforms/Scalar/LICM.cpp:379
@@ +378,3 @@
+        return false;
+      } else if (isa<PHINode>(&I) || isa<BranchInst>(&I)) {
+        continue;
----------------
Everywhere you have isa<BranchInst> you probably want to introduce handle all terminators except invokes.  

================
Comment at: lib/Transforms/Scalar/LICM.cpp:384
@@ +383,3 @@
+        return false;
+      } else if (!isSafeToExecuteUnconditionally(I, DT, TLI, CurLoop, SafetyInfo,
+                                 CurLoop->getLoopPreheader()->getTerminator())) {
----------------
This is a *really* expensive way to phrase this query.  It'll be general, but slow.  Can you look for an alternate way to express this for the entire loop in one go?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:609
@@ +608,3 @@
+  if (inSubLoop(BB, CurLoop, LI)) {
+    BasicBlock *BB = N->getBlock();
+    Loop *SubLoop = LI->getLoopFor(BB);
----------------
BB is already available in this scope.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:613
@@ +612,3 @@
+    // Make sure this is the dominating block.
+    if (SubLoop->getHeader() == BB) {
+      if (canSinkSubLoop(SubLoop, AA, LI, DT, TLI, CurLoop, CurAST,
----------------
Introducing a helper function isHeaderOfSubLoop would make this far easier to follow.  

================
Comment at: lib/Transforms/Scalar/LICM.cpp:622
@@ +621,3 @@
+        Changed = true;
+        // runOnLoop uses the subloop list to find out what to free. Because
+        // SubLoop is no longer in that list, free it here instead.
----------------
I don't see that your updating DT here.  This is problematic since we'll be walking a stale tree.





http://reviews.llvm.org/D17203