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

[Anna Thomas via Phabricator via llvm-commits]

anna marked an inline comment as done.
anna added a comment.

In this change, I've added the preheader requirement to never executed loops as well. The main reason for adding this requirement
is that in all testcases I've added, `opt -loop-deletion` always generates a preheader for the loop before running this pass. 
Not sure how to test for non-preheader cases. 
So, the core logic in `deleteDeadLoop` is now very similar to initial code before the patch.



================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:105
+/// In LLVM, a loop can never be unreachable i.e. it's header would
+/// always have atleast one predecessor. However, in cases where the loop
+/// header would never be executed at runtime, we consider such loops as
----------------
sanjoy wrote:
> Is "not unreachable" == "i.e. it's header would always have atleast one predecessor" or == "its header must have a path from the entry block"?
the latter. i've updated the comment.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:134
+  // of constant conditional branch.
+  auto *BI = dyn_cast<BranchInst>(Pred->getTerminator());
+  if (!BI || BI->isUnconditional())
----------------
sanjoy wrote:
> Perhaps this can be made more concise using PatternMatch?
> 
> ```
> BasicBlock *Taken, *NotTaken;
> ConstantInst *Cond;
> if (!match(Pred->getTerminator(), m_Br(m_ConstantInt(Cond), Taken, NotTaken)))
>   return false;
> if (!Cond->getZExtValue())
>   std::swap(Taken, NotTaken);
> ```
> 
> edit: actually, I think this can be combined with the previous check too to make the entire function body be:
> 
> ```
> auto *Pred = L->getLoopPredecessor();
> auto *FirstLoopBlock = L->getHeader();
> 
> for (int i = 0; i < 2 && Pred; i++) {
>   BasicBlock *Taken, *NotTaken;
>   ConstantInst *Cond;
>   if (match(Pred->getTerminator(), m_Br(m_ConstantInt(Cond), Taken, NotTaken))) {
>     if (!Cond->getZExtValue())
>       std::swap(Taken, NotTaken);
>     return NotTaken == FirstLoopBlock && Taken != FirstLoopBlock;
>     // Alternatively:
>     // if (NotTaken == FirstLoopBlock && Taken != FirstLoopBlock)
>     //   return true;
>   }
>   FirstLoopBlock = Pred;
>   Pred = Pred->getSinglePredecessor();
> }
> ```
> 
I've changed this function to consider all immediate predecessors of the preheader. Preheader is now a requirement for this function. Pls see the updated function.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:235
+  // loop.
+  if (LoopIsNeverExecuted)
+    BlockCausingLoopUnreachable =
----------------
anna wrote:
> sanjoy wrote:
> > I would just pass in the block that we decided was the last executed block instead of re-deriving it here.  Otherwise we risk the logic here and `isLoopNeverExecuted` diverging.
> > 
> > [edit: the comment above about splitting the preheader supersedes this comment]
> Actually, that's what I did initially (during local modification). However, it required passing that last executed block between 3 functions. I personally didnt like that design either. 
> 
>  I like your first comment on splitting the edge from the loop predecessor to the header. I'll try to get that working and keep the core logic as similar as possible.
This logic is now removed because of the preheader requirement. 


https://reviews.llvm.org/D32494