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

[Anna Thomas via Phabricator via llvm-commits]

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


================
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
----------------
sanjoy wrote:
> Why is Loop-SimplifyCFG relevant here?  Won't it only simplify control flow //within// a loop?
Yes, that's right. The comment maybe confusing. My assumption is that these kind of constant conditional branches are generated by unswitch [1], i.e.  branch is within a loop. At a later point, deleting unreachable loop will be handled by Loop-SimplifyCFG: It will *start* at a constant branch condition, iterating forward through the blocks that will never be executed, and in the process if we reach a loop header which will never be executed, that loop will be deleted.

So, loop-deletion will just be a subcase of loop-simplifyCFG. At some point, we should just be able to call loop-simplifyCFG in the pass pipeline instead of loop-deletion. 

[1] If we have constant conditional branches outside of loops generated by some other non-loop passes, we could just use SimplifyCFG to eliminate the code? This brings me to the main reason for the patch: implementing the actual loop deletion, update dom tree etc for unreachable loops, and use that function for Loop-SimplifyCFG.



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


================
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
----------------
sanjoy wrote:
> I'm missing where you're checking for this second case?
That's the original and already existing case for loop deletion pass: see `isLoopDead`. I'll update this comment so that it does not look like this was an addition in the patch. 


================
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.
----------------
sanjoy wrote:
> 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?
> 
This comment is incorrect. There's no problem in deleting the edge. The only reason is to keep the code here similar in both cases: keep the edge from the source block to the exit block. If I special case the "loop never executes" scenario to delete the edge, we can remove the value from the phi as well. 
For now, I'll keep the code the same, and leave the edge as-is. 


https://reviews.llvm.org/D32494