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

[Chen Li]

chenli added a comment.

In http://reviews.llvm.org/D11276#207564, @reames wrote:

> Comments inline.
>
> p.s. What's the motivation for this?  Is it a step towards a transform or just general code cleanup?


It's actually both. This separation is needed for further improvement on trivial loop unswitch. However, I think it is still a good cleanup if no future patch is landed based on this.


================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:834
@@ +833,3 @@
+// TryTrivialLoopUnswitch - Check if loop header block's terminator is a trivial
+// unswitch condition (loop header block is the only possible block to have a
+// trivial unswitch condition). If it is, always unswitch becayse there is no
----------------
reames wrote:
> why is the header the only possible trivial unswitch?  (I know the answer, but it's not obvious from the comment.)
More comments will be added to address this.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:841
@@ +840,3 @@
+
+  Constant *CondVal = nullptr;
+  BasicBlock *ExitBlock = nullptr;
----------------
reames wrote:
> This code can be greatly simplified by inlining the definition of FindLIVLoopCondition and IsTrivialUnswitchCondition.  If I'm reading the code right, you'd basically end up with the existing code in isTrivialUnswitchCondition plus getting the condition from the branch and checking that it's loop invariant.  
> 
> To put it differently, the generality of this code is actively confusing, not helping.  
I will fix this.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:855
@@ +854,3 @@
+        UnswitchTrivialCondition(currentLoop, LoopCond, CondVal, ExitBlock, HeaderTerm);
+        return true;
+      }
----------------
reames wrote:
> You're missing the NumBranches and NumSwitches statistic updates here.  You might be better off introducing new statistics for the trivial cases for reporting though.
I have actually updated NumBranches in the caller of this function. I forgot to separate NumBranches and NumSwitches though. I will redo correctly. Trivial cases are already updated inside UnswitchTrivialCondition function.


http://reviews.llvm.org/D11276