[PATCH] D32494: [Loop Deletion] Delete loops that are never executed

[Anna Thomas via Phabricator via llvm-commits]

anna marked 7 inline comments as done.
anna added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:35
+/// guarantee that the loop is infact dead.  Here we handle two kinds of dead
+/// loop. The first kind is where no backedges exist, and the loop variant
+/// statements are made invariant by moving to the preheader.  The second kind
----------------
sanjoy wrote:
> I thought we decided that backedges do exist in those cases?
oops, forgot to change the comment.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:37
+/// statements are made invariant by moving to the preheader.  The second kind
+/// is where the loop is no longer a loop in LLVM notion, since the loop is
+/// semantically unreachable (see `IsLoopNeverExecuted`).  We can be more
----------------
sanjoy wrote:
> Isn't the second kind of loop still a loop?  It just is a loop that is provably not reachable.
It is, but I think it's no longer a loop according to how llvm identifies loops. See LoopInfoImpl.h `analyze`: We always check that the backedges are reachable from entry (which auto implies the header that dominates these backedges are also reachable from entry). I've made the description of this function more concise though. 


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:135
+  // loop preheader as not-taken target.
+  return true;
+}
----------------
sanjoy wrote:
> Is the former ("the preheader has no predecessors") possible since we have an llvm::Loop?  If not, I'd add an assert.
I think the only case is when the preheader is the function entry block. That's already been handled above. So, per llvm notion, the preheader should have predecessors at this point. Added the assert.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:254
     for (unsigned i = 1; i < ExitingBlocks.size(); ++i)
       P->removeIncomingValue(ExitingBlocks[i]);
     ++BI;
----------------
sanjoy wrote:
> Not directly relevant to this patch, but is this bit correct with multiple edges from an exiting block to an exit block?
I think this is correct because the `ExitingBlocks` are generated by `getExitingBlocks`, so it handles multiple edges from an exiting block to an exit block. If it was `getUniqueExitingBlocks`, this would be incorrect.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:336
     return false;
-
+errs() << "Loop Deletion for function: " << *L->getHeader()->getParent() << "\n";
   DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
----------------
sanjoy wrote:
> Debugging code?
thanks :)


https://reviews.llvm.org/D32494