[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.

This looks pretty close to done.

One high level issue I have is that you've interchangeably used "semantically unreachable" and "never executed".  I'd standardize to one or the other to reduce confusion.  I'd lean towards "never executed" or "known never executed", but if you find "semantically unreachable" better, using that would also be better than using both.



================
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
----------------
I thought we decided that backedges do exist in those cases?


================
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
----------------
Isn't the second kind of loop still a loop?  It just is a loop that is provably not reachable.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:39
+/// semantically unreachable (see `IsLoopNeverExecuted`).  We can be more
+/// aggressive in removing the semantically unreachable loops since they will
+/// not cause any difference to program behaviour (observable or unobservable).
----------------
Why not s/be more aggressive in removing the/always remove/ ?


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:40
+/// aggressive in removing the semantically unreachable loops since they will
+/// not cause any difference to program behaviour (observable or unobservable).
+///
----------------
s/program behaviour (observable or unobservable)/observable program behavior/

I don't think we care about changing unobservable program behavior. :)

I'd even drop the observable, btw, since "program behavior" means "observable program behavior".


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:135
+  // loop preheader as not-taken target.
+  return true;
+}
----------------
Is the former ("the preheader has no predecessors") possible since we have an llvm::Loop?  If not, I'd add an assert.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:156
+
+  // We can't remove loops that contain subloops.  If the subloops were dead,
+  // they would already have been removed in earlier executions of this pass.
----------------
Any reason why you reordered the preheader and the empty loop checks?  If not, I'd lean towards to keeping them the way they are to make the diff smaller.


================
Comment at: lib/Transforms/Scalar/LoopDeletion.cpp:254
     for (unsigned i = 1; i < ExitingBlocks.size(); ++i)
       P->removeIncomingValue(ExitingBlocks[i]);
     ++BI;
----------------
Not directly relevant to this patch, but is this bit correct with multiple edges from an exiting block to an exit block?


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


================
Comment at: test/Transforms/LoopDeletion/unreachable-loops.ll:1
+; RUN: opt < %s -loop-deletion -S | FileCheck %s
+; RUN: opt < %s -loop-deletion -verify-dom-info -S
----------------
Why not just add ` -verify-dom-info` to the first invocation (instead of having a separate one)?


https://reviews.llvm.org/D32494