Apply review feedback

JosephTremoulet · JosephTremoulet · commit 00ea5afde1f1 · 2017-08-10T16:59:13.000-04:00
- Rename lastExitBlock -&gt; lastNonLoopBlock
 - Bail out of loop search when MAX_LOOP_NUM hit
 - Define `moveBefore` and clarify comments around `moveAfter` and
   `next`
 - Check to see if the back-edge is in the loop by testing against top,
   rather than bottom, since the loop discovery walks to preds.
diff --git a/src/jit/compiler.h b/src/jit/compiler.h
@@ -5357,7 +5357,7 @@ class Compiler
                                 GenTreePtr* ppTest,
                                 GenTreePtr* ppIncr);
 
-    void optRecordLoop(BasicBlock*   head,
+    bool optRecordLoop(BasicBlock*   head,
                        BasicBlock*   first,
                        BasicBlock*   top,
                        BasicBlock*   entry,
diff --git a/src/jit/optimizer.cpp b/src/jit/optimizer.cpp
@@ -1119,10 +1119,11 @@ bool Compiler::optExtractInitTestIncr(
 
 /*****************************************************************************
  *
- *  Record the loop in the loop table.
+ *  Record the loop in the loop table.  Return true if successful, false if
+ *  out of entries in loop table.
  */
 
-void Compiler::optRecordLoop(BasicBlock*   head,
+bool Compiler::optRecordLoop(BasicBlock*   head,
                              BasicBlock*   first,
                              BasicBlock*   top,
                              BasicBlock*   entry,
@@ -1138,7 +1139,7 @@ void Compiler::optRecordLoop(BasicBlock*   head,
 #if COUNT_LOOPS
         loopOverflowThisMethod = true;
 #endif
-        return;
+        return false;
     }
 
     // Assumed preconditions on the loop we're adding.
@@ -1318,6 +1319,7 @@ void Compiler::optRecordLoop(BasicBlock*   head,
 DONE_LOOP:
     DBEXEC(verbose, optPrintLoopRecording(loopInd));
     optLoopCount++;
+    return true;
 }
 
 #ifdef DEBUG
@@ -1725,7 +1727,7 @@ void Compiler::optFindNaturalLoops()
                     goto NO_LOOP;
                 }
 
-                if (!loopBlocks.isMember(bottom->bbNum))
+                if (!loopBlocks.isMember(top->bbNum))
                 {
                     // The "back-edge" we identified isn't actually part of the flow cycle containing ENTRY
                     goto NO_LOOP;
@@ -1786,12 +1788,12 @@ void Compiler::optFindNaturalLoops()
                         // This code is lexically between TOP and BOTTOM, but it does not
                         // participate in the flow cycle.  Check for a run of consecutive
                         // such blocks.
-                        BasicBlock* lastExitBlock = block;
-                        BasicBlock* nextLoopBlock = block->bbNext;
+                        BasicBlock* lastNonLoopBlock = block;
+                        BasicBlock* nextLoopBlock    = block->bbNext;
                         while (!loopBlocks.isMember(nextLoopBlock->bbNum))
                         {
-                            lastExitBlock = nextLoopBlock;
-                            nextLoopBlock = nextLoopBlock->bbNext;
+                            lastNonLoopBlock = nextLoopBlock;
+                            nextLoopBlock    = nextLoopBlock->bbNext;
                             // This loop must terminate because we know BOTTOM is in loopBlocks.
                         }
 
@@ -1802,7 +1804,8 @@ void Compiler::optFindNaturalLoops()
                             moveAfter = bottom;
                             while (moveAfter->bbFallsThrough())
                             {
-                                // Avoid splitting up this fall-through if we can.
+                                // Avoid splitting up this fall-through if we can; see if we can legally
+                                // change `moveAfter` to the next block after it.
                                 BasicBlock* next = moveAfter->bbNext;
 
                                 if (!BasicBlock::sameEHRegion(moveAfter, next))
@@ -1863,13 +1866,13 @@ void Compiler::optFindNaturalLoops()
                                 for (flowList* predIter = testBlock->bbPreds; predIter != nullptr;
                                      predIter           = predIter->flNext)
                                 {
-                                    BasicBlock*  testPred       = predIter->flBlock;
-                                    unsigned int predPosNum     = positionNum(testPred);
-                                    unsigned int blockPosNum    = positionNum(block);
-                                    unsigned int lastExitPosNum = positionNum(lastExitBlock);
+                                    BasicBlock*  testPred          = predIter->flBlock;
+                                    unsigned int predPosNum        = positionNum(testPred);
+                                    unsigned int blockPosNum       = positionNum(block);
+                                    unsigned int lastNonLoopPosNum = positionNum(lastNonLoopBlock);
 
                                     if (loopBlocks.isMember(predPosNum) ||
-                                        ((predPosNum >= blockPosNum) && (predPosNum <= lastExitPosNum)))
+                                        ((predPosNum >= blockPosNum) && (predPosNum <= lastNonLoopPosNum)))
                                     {
                                         // This pred is in the loop (or what will be the loop if we determine this
                                         // run of exit blocks doesn't include a side-entry).
@@ -1900,19 +1903,21 @@ void Compiler::optFindNaturalLoops()
                             continue;
                         }
 
-                        fgUnlinkRange(block, lastExitBlock);
-                        fgMoveBlocksAfter(block, lastExitBlock, moveAfter);
-                        ehUpdateLastBlocks(moveAfter, lastExitBlock);
+                        BasicBlock* moveBefore = moveAfter->bbNext;
+
+                        fgUnlinkRange(block, lastNonLoopBlock);
+                        fgMoveBlocksAfter(block, lastNonLoopBlock, moveAfter);
+                        ehUpdateLastBlocks(moveAfter, lastNonLoopBlock);
 
-                        // If lastExitBlock fell through to nextLoopBlock, then it also would have been in the loop.
-                        assert(!lastExitBlock->bbFallsThrough());
+                        // If lastNonLoopBlock fell through to nextLoopBlock, then it also would have been in the loop.
+                        assert(!lastNonLoopBlock->bbFallsThrough());
 
                         if (moveAfter->bbFallsThrough())
                         {
                             // We've just inserted blocks between moveAfter and moveBefore which it was supposed
                             // to fall through to, so insert a jump to reconnect it to moveBefore.
                             assert(moveAfter->bbNum >= bottom->bbNum); // Blocks we moved out didn't have fallthrough.
-                            fgConnectFallThrough(moveAfter, lastExitBlock->bbNext);
+                            fgConnectFallThrough(moveAfter, moveBefore);
                         }
 
                         if (previous->bbFallsThrough())
@@ -1952,8 +1957,8 @@ void Compiler::optFindNaturalLoops()
                             }
                         }
 
-                        // Update moveAfter for the next insertion
-                        moveAfter = lastExitBlock;
+                        // Update moveAfter for the next insertion.
+                        moveAfter = lastNonLoopBlock;
 
                         // Note that we've changed the flow graph, and continue without updating
                         // `previous` so that we'll process nextLoopBlock.
@@ -2044,7 +2049,11 @@ void Compiler::optFindNaturalLoops()
                 {
                     exit = nullptr;
                 }
-                optRecordLoop(head, first, top, entry, bottom, exit, exitCount);
+                if (!optRecordLoop(head, first, top, entry, bottom, exit, exitCount))
+                {
+                    // We can't record any more loops (ran into MAX_LOOP_NUM), so stop looking.
+                    break;
+                }
 
 #if COUNT_LOOPS
                 if (!hasMethodLoops)
