[PATCH] D32494: [Loop Deletion] Delete loops that are semantically unreachable

[Sanjoy Das via Phabricator via llvm-commits]

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

I made a few first pass comments.  I think the term "semantically unreachable" sounds more complicated than it actually is. :)  How about "never executed", as in `isLoopNeverExecuted`?



================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:48
+                          LoopInfo &LI,
+                          bool LoopIsSemanticallyUnreachable,
+                          LPMUpdater *Updater = nullptr);
----------------
Indent seems off - clang-format?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:119
+
+  auto *Preheader = L->getLoopPreheader();
+
----------------
I think this distinction between predecessor and preheader is making the logic a bit more complex than it needs to be.  I'd instead do:

```
auto *Pred = L->getLoopPredecessor();
auto *Succ = L->getHeader();
if (Pred && Pred->getTerminator() is not conditional branch) {
  Succ = Pred;
  Pred = Pred->getSinglePredecessor();
}
if (!Pred || Pred->getTerminator() is not conditional branch)
  return false;
```



================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:134
+  // of constant conditional branch. We avoid doing this now, because
+  // Loop-SimplifyCFG will be enhanced to change branches in loop from constant
+  // conditional to unconditional, thereby iterating forward across the straight
----------------
Why is Loop-SimplifyCFG relevant here?  Won't it only simplify control flow //within// a loop?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:137
+  // line code.
+  TerminatorInst *T = HeaderPred->getTerminator();
+  auto *BI = dyn_cast<BranchInst>(T);
----------------
The `T` seems a bit superfluous here -- why not just `auto *BI = dyn_cast<BranchInst>(HeaderPred->getTerminator())`?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:145
+  BasicBlock *NotTaken =
+      Cond->getZExtValue() ? BI->getSuccessor(1) : BI->getSuccessor(0);
+  return (NotTaken == FirstLoopBlock);
----------------
What if both the branches branch to the header?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:146
+      Cond->getZExtValue() ? BI->getSuccessor(1) : BI->getSuccessor(0);
+  return (NotTaken == FirstLoopBlock);
+}
----------------
This can be just `return NotTaken == FirstLoopBlock;`


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:189
+  // The loop is not semantically unreachable. Check for the second case of a
+  // dead loop, i.e.  where the loop has no backedge, and all statements in the
+  // loop are invariant. This is a non-loop if we can move all statements within
----------------
I'm missing where you're checking for this second case?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:279
+    // We set it to the undef value instead of removing the incoming value to
+    // avoid dealing with dead phi's, which may make the corresponding block
+    // dead.
----------------
How do dead phis make a block dead?

If you're talking about keeping the edge from SourceBB to ExitBlock even though it will never be taken, then I see what you mean, but that comment should be on `SourceBB->getTerminator()->replaceUsesOfWith(L->getHeader(), ExitBlock);`.  Btw, what is the problem with deleting that edge?  Is it with updating LoopInfo or something else?



https://reviews.llvm.org/D32494