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

[Sanjoy Das via Phabricator via llvm-commits]

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

Comments inline.



================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:34
+/// 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.
----------------
> 80 chars line?


================
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
----------------
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"?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:107
+/// header would never be executed at runtime, we consider such loops as
+/// semantically unreachable. This function returns true when loop `L` is
+/// semantically unreachable. For example, loop L2 is unreachable in 
----------------
I'd rephrase this a bit: However, if the loop header would never be executed at runtime, the loop is considered semantically unreachable.

But generally, I'd be in favor of not adding so much detail here, but instead just saying something brief like: "This function returns true if there is no viable path from the entry block to the header of \p L.  Right now it only does a local search to save compile time(?)"



================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:134
+  // of constant conditional branch.
+  auto *BI = dyn_cast<BranchInst>(Pred->getTerminator());
+  if (!BI || BI->isUnconditional())
----------------
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();
}
```



================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:188
+  // backedge, and all statements in the loop are invariant. This is a non-loop
+  // if we can move all statements within the loop to the preheader.
+  // We can only remove the loop if there is a preheader that we can
----------------
I didn't understand the "has no backedge" part -- if it does not have a backedge how is it a loop?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:224
+static void deleteDeadLoop(Loop *L, DominatorTree &DT, ScalarEvolution &SE,
+                           LoopInfo &LI, bool LoopIsNeverExecuted,
+                           LPMUpdater *Updater) {
----------------
Why not insert a preheader if there wasn't one (by splitting the edge from the loop predecessor to the header), and keep the rest of the CFG modifying logic the same?  IIUC, the only other place that needs to change is the `P->setIncomingValue(j, UndefValue::get(P->getType()))`, and even that can be done as a pre-pass before `deleteDeadLoop` keeping the core logic here the same in both the situations.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:235
+  // loop.
+  if (LoopIsNeverExecuted)
+    BlockCausingLoopUnreachable =
----------------
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]


https://reviews.llvm.org/D32494