From 3f7c770d6a3f3683bb69b78d42e1caf8a3c7e449 Mon Sep 17 00:00:00 2001 From: Annabelle Huo Date: Wed, 27 Mar 2024 13:25:42 -0400 Subject: [PATCH] Do not continue to merge back cold path if guard2 block has been removed `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. Fixes: eclipse-openj9/openj9#18873 Signed-off-by: Annabelle Huo --- compiler/optimizer/VirtualGuardHeadMerger.cpp | 37 ++++++++++++++++--- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/compiler/optimizer/VirtualGuardHeadMerger.cpp b/compiler/optimizer/VirtualGuardHeadMerger.cpp index 80e287ba061..1a736409d90 100644 --- a/compiler/optimizer/VirtualGuardHeadMerger.cpp +++ b/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