Merge pull request #7297 from a7ehuo/fix-VirtualGuardHeadMerger-1

Do not continue to merge back cold path if guard2 block has been removed

compiler/optimizer/VirtualGuardHeadMerger.cpp

@@ -284,20 +284,23 @@ static void moveBlockAfterDest(TR::CFG *cfg, TR::Block *toMove, TR::Block *dest)
    }
 
 // This opt tries to reduce merge backs from cold code that are the result of inliner
-// gnerated nopable virtual guards
+// generated nopable virtual guards
 // It looks for one basic pattern
 //
 // guard1 -> cold1
 // BBEND
 // BBSTART
 // guard2 -> cold2
-// if guard1 is the guard for a method which calls the method guard2 protects or cold1 is
-// a predecessor of cold2 (a situation commonly greated by virtual guard tail splitter) we
-// can transform the guards as follows when guard1 and guard2 a
+//
+// If guard1 is the guard for a method which calls the method guard2 protects or cold1 is
+// a predecessor of cold2 (a situation commonly created by virtual guard tail splitter) we
+// can transform the guards as follows:
+//
 // guard1 -> cold1
 // BBEND
 // BBSTART
 // guard2 -> cold1
+//
 // This is safe because there are no trees between the guards and calling the caller will
 // result in the call to the callee if we need to patch guard2. cold2 and its mergebacks
 // can then be eliminated
@@ -383,6 +386,14 @@ int32_t TR_VirtualGuardHeadMerger::perform() {
          // scan for candidate guards to merge with guard1 identified above
          for (TR::Block *nextBlock = block->getNextBlock(); nextBlock; nextBlock = nextBlock->getNextBlock())
             {
+            if (block->nodeIsRemoved() || nextBlock->nodeIsRemoved())
+               {
+               if (trace())
+                  traceMsg(comp(), "block block_%d %p (%d) or nextBlock block_%d %p (%d) has been removed\n",
+                     block->getNumber(), block, block->nodeIsRemoved(), nextBlock->getNumber(), nextBlock, nextBlock->nodeIsRemoved());
+               break;
+               }
+
             if (!(nextBlock->getPredecessors().size() == 1) ||
                 !nextBlock->hasPredecessor(block))
                {
@@ -467,6 +478,22 @@ int32_t TR_VirtualGuardHeadMerger::perform() {
             if (guard2->getBranchDestination() != guard1->getBranchDestination())
                guard2Block->changeBranchDestination(guard1->getBranchDestination(), cfg);
 
+            // changeBranchDestination tries to remove unreachable blocks after the removal of the old edge
+            // from guard2 to cold2. Rarely, a previous transformation in this pass could have made
+            // guard2Block unreachable without removing it, in which case it might have been removed just now.
+            // Normally, if all relevant blocks are removed, later moveBlockAfterDest will not cause any issue
+            // since it will be just moving around all unreachable blocks. However, in an even more rare case,
+            // if some of the blocks are removed and some of them are not (eg, guard2Block and the previous block
+            // of guard2Block are removed but the next block of guard2Block is still valid), moveBlockAfterDest
+            // could end up joining an invalid/removed block with a valid block. Therefore, if guard2Block
+            // is no longer valid, it should not proceed.
+            if (guard2Block->nodeIsRemoved())
+               {
+               if (trace())
+                  traceMsg(comp(), "guard2Block block_%d %p has been removed after changeBranchDestination\n", guard2Block->getNumber(), guard2Block);
+               break;
+               }
+
             if (guard2Tree != guard2Block->getFirstRealTreeTop())
                {
                cfg->setStructure(NULL);
@@ -477,7 +504,7 @@ int32_t TR_VirtualGuardHeadMerger::perform() {
                // 2, it might contain live monitor, moving it up above a guard can affect the monitor's live range
                if (!isStopTheWorldGuard(guard2))
                   {
-	          // the block created above guard2 contains only privarg treetops or monitor stores if
+                  // the block created above guard2 contains only privarg treetops or monitor stores if
                   // guard2 is a runtime-patchable guard and is safe to merge. We need to move the priv
                   // args up to the runtime insert point and leave the monitor stores in place
                   // It's safe to do so because there is no data dependency between the monitor store and