[PATCH] D11276: [LoopUnswitch] Code refactoring to separate trivial loop unswitch and non-trivial loop unswitch in processCurrentLoop()

[Philip Reames]

reames accepted this revision.
reames added a comment.

Really close to ready.  I'm not quite willing to give a conditional LGTM, but one more round should do it.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:466
@@ +465,3 @@
+  // Trivial unswitch condition can only occur at loop header basic block.
+  // Try trivial unswitch first before loop over other basic blocks in the loop.
+  if (TryTrivialLoopUnswitch(Changed)) {
----------------
Why?  (in the comment)

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:844
@@ +843,3 @@
+  // *would* execute. Check the header first.
+  for (BasicBlock::iterator I = Header->begin(), E = Header->end(); I != E; ++I)
+    if (I->mayHaveSideEffects())
----------------
range for loop

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:858
@@ +857,3 @@
+                                           currentLoop, Changed);
+    if (!LoopCond)
+      return false;
----------------
This doesn't look correct.  The old code doesn't unwitch unless the *condition* is itself the LIV condition.  The new code might split an and/or and unsplit one part.  Doing that as a separate change is debatable, but not in a refactor please.

In fact, if you could add a negative test case for this, that would be great.  


http://reviews.llvm.org/D11276