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

[• chenli]

Summary

The current code in LoopUnswtich::processCurrentLoop() mixes trivial loop unswitch and non-trivial loop unswitch together. It goes over all basic blocks in the loop and checks if a condition is trivial or non-trivial unswitch condition. However, trivial unswitch condition can only occur in the loop header basic block (where it controls whether or not the loop does something at all). This refactoring separate trivial loop unswitch and non-trivial loop unswitch. Before going over all basic blocks in the loop, it checks if the loop header contains a trivial unswitch condition. If so, unswitch it. Otherwise, go over all blocks like before but don't check trivial condition any more since they are not possible to be in the other blocks. This code has no functionality change.

[reames]

Comments inline.

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

lib/Transforms/Scalar/LoopUnswitch.cpp

834

why is the header the only possible trivial unswitch? (I know the answer, but it's not obvious from the comment.)

835

spelling

841

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.

855

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.

[• chenli]

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

lib/Transforms/Scalar/LoopUnswitch.cpp

834

More comments will be added to address this.

841

I will fix this.

855

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.

[• chenli]

Update w.r.t Philip's comments.

After inlining isTrivialUnswitchCondition() into TryTrivialLoopUnswitch(), isTrivialUnswitchCondition() is basically dead code (no use anywhere). A following patch will be submitted to clean it up.

[broune]

I'd wait to see what reames thinks, but this looks good to me.

[reames]

After inlining isTrivialUnswitchCondition() into TryTrivialLoopUnswitch(), isTrivialUnswitchCondition() is basically dead code (no use anywhere). A following patch will be submitted to clean it up.

Please delete it in this commit. It'll generate a warning and some bots treat warnings as errors.

[reames]

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

lib/Transforms/Scalar/LoopUnswitch.cpp

466

Why? (in the comment)

845

range for loop

859

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.

[• chenli]

Delete IsTrivialUnswitchCondition() and move related comments to TryTrivialLoopUnswitch()

[• chenli]

Philip, sorry I didn't see your inlined comments (I was preparing deleting IsTrivialUnswitchCondition() the same time). I will address it with a new update.

[• chenli]

Fixed issue pointed by Philip and added a test case for that.

[reames]

LGTM

lib/Transforms/Scalar/LoopUnswitch.cpp

void initLoopData() {
       loopHeader = currentLoop->getHeader();
       loopPreheader = currentLoop->getLoopPreheader();
     }
 
     /// Split all of the edges from inside the loop to their exit blocks.
     /// Update the appropriate Phi nodes as we do so.
     void SplitExitEdges(Loop *L, const SmallVectorImpl<BasicBlock *> &ExitBlocks);
 
+    bool TryTrivialLoopUnswitch(bool &Changed);
+
     bool UnswitchIfProfitable(Value *LoopCond, Constant *Val,
                               TerminatorInst *TI = nullptr);
     void UnswitchTrivialCondition(Loop *L, Value *Cond, Constant *Val,
                                   BasicBlock *ExitBlock, TerminatorInst *TI);
     void UnswitchNontrivialCondition(Value *LIC, Constant *OnVal, Loop *L,
                                      TerminatorInst *TI);
 
     void RewriteLoopBodyWithConditionConstant(Loop *L, Value *LIC,
bool LoopUnswitch::processCurrentLoop() {
   // Probably we reach the quota of branches for this loop. If so
   // stop unswitching.
   if (!BranchesInfo.countLoop(
           currentLoop, getAnalysis<TargetTransformInfoWrapperPass>().getTTI(
                            *currentLoop->getHeader()->getParent()),
           AC))
     return false;
 
+  // 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)) {
+    ++NumBranches;
+    return true;
+  }
+
   // Loop over all of the basic blocks in the loop.  If we find an interior
   // block that is branching on a loop-invariant condition, we can unswitch this
   // loop.
   for (Loop::block_iterator I = currentLoop->block_begin(),
          E = currentLoop->block_end(); I != E; ++I) {
     TerminatorInst *TI = (*I)->getTerminator();
     if (BranchInst *BI = dyn_cast<BranchInst>(TI)) {
       // If this isn't branching on an invariant condition, we can't unswitch

 }
 
 /// UnswitchIfProfitable - We have found that we can unswitch currentLoop when
 /// LoopCond == Val to simplify the loop.  If we decide that this is profitable,
 /// unswitch the loop, reprocess the pieces, then return true.
 bool LoopUnswitch::UnswitchIfProfitable(Value *LoopCond, Constant *Val,
                                         TerminatorInst *TI) {
   Function *F = loopHeader->getParent();
-  Constant *CondVal = nullptr;
-  BasicBlock *ExitBlock = nullptr;
-
-  if (IsTrivialUnswitchCondition(LoopCond, &CondVal, &ExitBlock)) {
-    // If the condition is trivial, always unswitch. There is no code growth
-    // for this case.
-    UnswitchTrivialCondition(currentLoop, LoopCond, CondVal, ExitBlock, TI);
-    return true;
-  }
 
   // Check to see if it would be profitable to unswitch current loop.
   if (!BranchesInfo.CostAllowsUnswitching()) {
     DEBUG(dbgs() << "NOT unswitching loop %"
                  << currentLoop->getHeader()->getName()
                  << " at non-trivial condition '" << *Val
                  << "' == " << *LoopCond << "\n"
                  << ". Cost too high.\n");
void LoopUnswitch::UnswitchTrivialCondition(Loop *L, Value *Cond, Constant *Val,
 
   // Now that we know that the loop is never entered when this condition is a
   // particular value, rewrite the loop with this info.  We know that this will
   // at least eliminate the old branch.
   RewriteLoopBodyWithConditionConstant(L, Cond, Val, false);
   ++NumTrivial;
 }
 
+// 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
+// code growth for this case.
+bool LoopUnswitch::TryTrivialLoopUnswitch(bool &Changed) {
+  BasicBlock *Header = currentLoop->getHeader();
+  TerminatorInst *HeaderTerm = Header->getTerminator();
+
+  Constant *CondVal = nullptr;
+  BasicBlock *ExitBlock = nullptr;
+
+  if (BranchInst *BI = dyn_cast<BranchInst>(HeaderTerm)) {
+    // If this isn't branching on an invariant condition, we can't unswitch
+    // it.
+    if (BI->isConditional()) {
+      // See if this, or some part of it, is loop invariant.  If so, we can
+      // unswitch on it if we desire.
+      Value *LoopCond = FindLIVLoopCondition(BI->getCondition(),
+                                             currentLoop, Changed);
+      if (LoopCond &&
+          IsTrivialUnswitchCondition(LoopCond, &CondVal, &ExitBlock)) {
+        UnswitchTrivialCondition(currentLoop, LoopCond, CondVal, ExitBlock, HeaderTerm);
+        return true;
+      }
+    }
+  } else if (SwitchInst *SI = dyn_cast<SwitchInst>(HeaderTerm)) {
+    Value *LoopCond = FindLIVLoopCondition(SI->getCondition(),
+                                           currentLoop, Changed);
+    unsigned NumCases = SI->getNumCases();
+    if (LoopCond && NumCases) {
+      if (IsTrivialUnswitchCondition(LoopCond, &CondVal, &ExitBlock)) {
+        UnswitchTrivialCondition(currentLoop, LoopCond, CondVal, ExitBlock, nullptr);
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 /// SplitExitEdges - Split all of the edges from inside the loop to their exit
 /// blocks.  Update the appropriate Phi nodes as we do so.
 void LoopUnswitch::SplitExitEdges(Loop *L,
                                const SmallVectorImpl<BasicBlock *> &ExitBlocks){
 
   for (unsigned i = 0, e = ExitBlocks.size(); i != e; ++i) {
     BasicBlock *ExitBlock = ExitBlocks[i];
     SmallVector<BasicBlock *, 4> Preds(pred_begin(ExitBlock),