diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 49b6903668403..ad469c439c961 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1415,6 +1415,9 @@ struct BasicBlock : private LIR::Range template BasicBlockVisit VisitAllSuccs(Compiler* comp, TFunc func); + template + BasicBlockVisit VisitEHSuccs(Compiler* comp, TFunc func); + template BasicBlockVisit VisitRegularSuccs(Compiler* comp, TFunc func); diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 3a25901f65841..fb72d971b3c02 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -487,9 +487,14 @@ BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs(Compiler* comp, TFunc func) } //------------------------------------------------------------------------------ -// VisitEHSuccessors: Given a block inside a handler region, visit all handlers +// VisitEHSuccs: Given a block inside a handler region, visit all handlers // that control may flow to as part of EH. // +// Type arguments: +// skipJumpDest - Whether the jump destination has already been +// yielded, in which case it should be skipped here. +// TFunc - Functor type +// // Arguments: // comp - Compiler instance // block - The block @@ -499,12 +504,13 @@ BasicBlockVisit BasicBlock::VisitEHSecondPassSuccs(Compiler* comp, TFunc func) // Whether or not the visiting should proceed. // // Remarks: -// This encapsulates the "exception handling" successors of a block. That is, -// if a basic block BB1 occurs in a try block, we consider the first basic -// block BB2 of the corresponding handler to be an "EH successor" of BB1. +// This encapsulates the "exception handling" successors of a block. These +// are the blocks that control may flow to as part of exception handling: +// 1. On thrown exceptions, control may flow to handlers +// 2. As part of two pass EH, control may flow from filters to enclosed handlers // -template -static BasicBlockVisit VisitEHSuccessors(Compiler* comp, BasicBlock* block, TFunc func) +template +static BasicBlockVisit VisitEHSuccs(Compiler* comp, BasicBlock* block, TFunc func) { if (!block->HasPotentialEHSuccs(comp)) { @@ -516,12 +522,10 @@ static BasicBlockVisit VisitEHSuccessors(Compiler* comp, BasicBlock* block, TFun { while (true) { - // If the original block whose EH successors we're iterating over - // is a BBJ_CALLFINALLY, that finally clause's first block - // will be yielded as a normal successor. Don't also yield as - // an exceptional successor. + // For BBJ_CALLFINALLY the user may already have processed one of + // the EH successors as a regular successor; skip it if requested. BasicBlock* flowBlock = eh->ExFlowBlock(); - if (!block->KindIs(BBJ_CALLFINALLY) || !block->HasJumpTo(flowBlock)) + if (!skipJumpDest || !block->HasJumpTo(flowBlock)) { RETURN_ON_ABORT(func(flowBlock)); } @@ -538,6 +542,26 @@ static BasicBlockVisit VisitEHSuccessors(Compiler* comp, BasicBlock* block, TFun return block->VisitEHSecondPassSuccs(comp, func); } +//------------------------------------------------------------------------------ +// VisitEHSuccs: Given a block inside a handler region, visit all handlers +// that control may flow to as part of EH. +// +// Arguments: +// comp - Compiler instance +// func - Callback +// +// Returns: +// Whether or not the visiting should proceed. +// +// Remarks: +// For more documentation, see ::VisitEHSuccs. +// +template +BasicBlockVisit BasicBlock::VisitEHSuccs(Compiler* comp, TFunc func) +{ + return ::VisitEHSuccs(comp, this, func); +} + //------------------------------------------------------------------------------ // VisitAllSuccs: Visit all successors (including EH successors) of this block. // @@ -559,14 +583,17 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) RETURN_ON_ABORT(func(bbJumpEhf->bbeSuccs[i])); } - return VisitEHSuccessors(comp, this, func); + return VisitEHSuccs(comp, func); case BBJ_CALLFINALLY: + RETURN_ON_ABORT(func(bbJumpDest)); + return ::VisitEHSuccs(comp, this, func); + case BBJ_EHCATCHRET: case BBJ_EHFILTERRET: case BBJ_LEAVE: RETURN_ON_ABORT(func(bbJumpDest)); - return VisitEHSuccessors(comp, this, func); + return VisitEHSuccs(comp, func); case BBJ_ALWAYS: RETURN_ON_ABORT(func(bbJumpDest)); @@ -577,14 +604,14 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) // and we skip its normal EH successors. if (!isBBCallAlwaysPairTail()) { - RETURN_ON_ABORT(VisitEHSuccessors(comp, this, func)); + RETURN_ON_ABORT(VisitEHSuccs(comp, func)); } return BasicBlockVisit::Continue; case BBJ_NONE: RETURN_ON_ABORT(func(bbNext)); - return VisitEHSuccessors(comp, this, func); + return VisitEHSuccs(comp, func); case BBJ_COND: RETURN_ON_ABORT(func(bbNext)); @@ -594,7 +621,7 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) RETURN_ON_ABORT(func(bbJumpDest)); } - return VisitEHSuccessors(comp, this, func); + return VisitEHSuccs(comp, func); case BBJ_SWITCH: { @@ -604,13 +631,13 @@ BasicBlockVisit BasicBlock::VisitAllSuccs(Compiler* comp, TFunc func) RETURN_ON_ABORT(func(sd.nonDuplicates[i])); } - return VisitEHSuccessors(comp, this, func); + return VisitEHSuccs(comp, func); } case BBJ_THROW: case BBJ_RETURN: case BBJ_EHFAULTRET: - return VisitEHSuccessors(comp, this, func); + return VisitEHSuccs(comp, func); default: unreached(); diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index 5601d58864ab1..64b40d11f65bf 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -816,7 +816,7 @@ void SsaBuilder::RenameDef(GenTree* defNode, BasicBlock* block) #endif // DEBUG // Now add this SSA # to all phis of the reachable catch blocks. - AddMemoryDefToHandlerPhis(ByrefExposed, block, ssaNum); + AddMemoryDefToEHSuccessorPhis(ByrefExposed, block, ssaNum); } if (!isLocal) @@ -839,7 +839,7 @@ void SsaBuilder::RenameDef(GenTree* defNode, BasicBlock* block) m_renameStack.PushMemory(GcHeap, block, ssaNum); m_pCompiler->GetMemorySsaMap(GcHeap)->Set(defNode, ssaNum); - AddMemoryDefToHandlerPhis(GcHeap, block, ssaNum); + AddMemoryDefToEHSuccessorPhis(GcHeap, block, ssaNum); } } } @@ -884,9 +884,9 @@ unsigned SsaBuilder::RenamePushDef(GenTree* defNode, BasicBlock* block, unsigned // If necessary, add SSA name to the arg list of a phi def in any handlers for try // blocks that "block" is within. (But only do this for "real" definitions, not phis.) - if (!defNode->IsPhiDefn()) + if (!defNode->IsPhiDefn() && block->HasPotentialEHSuccs(m_pCompiler)) { - AddDefToHandlerPhis(block, lclNum, ssaNum); + AddDefToEHSuccessorPhis(block, lclNum, ssaNum); } return ssaNum; @@ -923,135 +923,123 @@ void SsaBuilder::RenameLclUse(GenTreeLclVarCommon* lclNode, BasicBlock* block) lclNode->SetSsaNum(ssaNum); } -void SsaBuilder::AddDefToHandlerPhis(BasicBlock* block, unsigned lclNum, unsigned ssaNum) +void SsaBuilder::AddDefToEHSuccessorPhis(BasicBlock* block, unsigned lclNum, unsigned ssaNum) { - assert(m_pCompiler->lvaTable[lclNum].lvTracked); // Precondition. + assert(block->HasPotentialEHSuccs(m_pCompiler)); + assert(m_pCompiler->lvaTable[lclNum].lvTracked); + + DBG_SSA_JITDUMP("Definition of local V%02u/d:%d in block " FMT_BB + " has potential EH successors; adding as phi arg to EH successors\n", + lclNum, ssaNum, block->bbNum); + unsigned lclIndex = m_pCompiler->lvaTable[lclNum].lvVarIndex; - EHblkDsc* tryBlk = m_pCompiler->ehGetBlockExnFlowDsc(block); - if (tryBlk != nullptr) - { - DBG_SSA_JITDUMP("Definition of local V%02u/d:%d in block " FMT_BB - " has exn handler; adding as phi arg to handlers.\n", - lclNum, ssaNum, block->bbNum); - while (true) + block->VisitEHSuccs(m_pCompiler, [=](BasicBlock* succ) { + // Is "lclNum" live on entry to the handler? + if (!VarSetOps::IsMember(m_pCompiler, succ->bbLiveIn, lclIndex)) { - BasicBlock* handler = tryBlk->ExFlowBlock(); + return BasicBlockVisit::Continue; + } - // Is "lclNum" live on entry to the handler? - if (VarSetOps::IsMember(m_pCompiler, handler->bbLiveIn, lclIndex)) - { #ifdef DEBUG - bool phiFound = false; + bool phiFound = false; #endif - // A prefix of blocks statements will be SSA definitions. Search those for "lclNum". - for (Statement* const stmt : handler->Statements()) - { - // If the tree is not an SSA def, break out of the loop: we're done. - if (!stmt->IsPhiDefnStmt()) - { - break; - } + // A prefix of blocks statements will be SSA definitions. Search those for "lclNum". + for (Statement* const stmt : succ->Statements()) + { + // If the tree is not an SSA def, break out of the loop: we're done. + if (!stmt->IsPhiDefnStmt()) + { + break; + } - GenTreeLclVar* phiDef = stmt->GetRootNode()->AsLclVar(); - assert(phiDef->IsPhiDefn()); + GenTreeLclVar* phiDef = stmt->GetRootNode()->AsLclVar(); + assert(phiDef->IsPhiDefn()); - if (phiDef->GetLclNum() == lclNum) - { - // It's the definition for the right local. Add "ssaNum" to the RHS. - AddPhiArg(handler, stmt, phiDef->Data()->AsPhi(), lclNum, ssaNum, block); + if (phiDef->GetLclNum() == lclNum) + { + // It's the definition for the right local. Add "ssaNum" to the RHS. + AddPhiArg(succ, stmt, phiDef->Data()->AsPhi(), lclNum, ssaNum, block); #ifdef DEBUG - phiFound = true; + phiFound = true; #endif - break; - } - } - assert(phiFound); - } - - unsigned nextTryIndex = tryBlk->ebdEnclosingTryIndex; - if (nextTryIndex == EHblkDsc::NO_ENCLOSING_INDEX) - { break; } - - tryBlk = m_pCompiler->ehGetDsc(nextTryIndex); } - } + assert(phiFound); + + return BasicBlockVisit::Continue; + + }); } -void SsaBuilder::AddMemoryDefToHandlerPhis(MemoryKind memoryKind, BasicBlock* block, unsigned ssaNum) +void SsaBuilder::AddMemoryDefToEHSuccessorPhis(MemoryKind memoryKind, BasicBlock* block, unsigned ssaNum) { - if (m_pCompiler->ehBlockHasExnFlowDsc(block)) + assert(block->HasPotentialEHSuccs(m_pCompiler)); + + // Don't do anything for a compiler-inserted BBJ_ALWAYS that is a "leave helper". + if ((block->bbFlags & BBF_INTERNAL) && block->isBBCallAlwaysPairTail()) { - // Don't do anything for a compiler-inserted BBJ_ALWAYS that is a "leave helper". - if ((block->bbFlags & BBF_INTERNAL) && block->isBBCallAlwaysPairTail()) - { - return; - } + return; + } + + // Otherwise... + DBG_SSA_JITDUMP("Definition of %s/d:%d in block " FMT_BB + " has potential EH successors; adding as phi arg to EH successors.\n", + memoryKindNames[memoryKind], ssaNum, block->bbNum); - // Otherwise... - DBG_SSA_JITDUMP("Definition of %s/d:%d in block " FMT_BB " has exn handler; adding as phi arg to handlers.\n", - memoryKindNames[memoryKind], ssaNum, block->bbNum); - EHblkDsc* tryBlk = m_pCompiler->ehGetBlockExnFlowDsc(block); - while (true) + block->VisitEHSuccs(m_pCompiler, [=](BasicBlock* succ) { + // Is memoryKind live on entry to the handler? + if ((succ->bbMemoryLiveIn & memoryKindSet(memoryKind)) == 0) { - BasicBlock* handler = tryBlk->ExFlowBlock(); + return BasicBlockVisit::Continue; + } - // Is memoryKind live on entry to the handler? - if ((handler->bbMemoryLiveIn & memoryKindSet(memoryKind)) != 0) - { - // Add "ssaNum" to the phi args of memoryKind. - BasicBlock::MemoryPhiArg*& handlerMemoryPhi = handler->bbMemorySsaPhiFunc[memoryKind]; + // Add "ssaNum" to the phi args of memoryKind. + BasicBlock::MemoryPhiArg*& handlerMemoryPhi = succ->bbMemorySsaPhiFunc[memoryKind]; #if DEBUG - if (m_pCompiler->byrefStatesMatchGcHeapStates) - { - // When sharing phis for GcHeap and ByrefExposed, callers should ask to add phis - // for ByrefExposed only. - assert(memoryKind != GcHeap); - if (memoryKind == ByrefExposed) - { - // The GcHeap and ByrefExposed phi funcs should always be in sync. - assert(handlerMemoryPhi == handler->bbMemorySsaPhiFunc[GcHeap]); - } - } + if (m_pCompiler->byrefStatesMatchGcHeapStates) + { + // When sharing phis for GcHeap and ByrefExposed, callers should ask to add phis + // for ByrefExposed only. + assert(memoryKind != GcHeap); + if (memoryKind == ByrefExposed) + { + // The GcHeap and ByrefExposed phi funcs should always be in sync. + assert(handlerMemoryPhi == succ->bbMemorySsaPhiFunc[GcHeap]); + } + } #endif - if (handlerMemoryPhi == BasicBlock::EmptyMemoryPhiDef) - { - handlerMemoryPhi = new (m_pCompiler) BasicBlock::MemoryPhiArg(ssaNum); - } - else - { + if (handlerMemoryPhi == BasicBlock::EmptyMemoryPhiDef) + { + handlerMemoryPhi = new (m_pCompiler) BasicBlock::MemoryPhiArg(ssaNum); + } + else + { #ifdef DEBUG - BasicBlock::MemoryPhiArg* curArg = handler->bbMemorySsaPhiFunc[memoryKind]; - while (curArg != nullptr) - { - assert(curArg->GetSsaNum() != ssaNum); - curArg = curArg->m_nextArg; - } + BasicBlock::MemoryPhiArg* curArg = succ->bbMemorySsaPhiFunc[memoryKind]; + while (curArg != nullptr) + { + assert(curArg->GetSsaNum() != ssaNum); + curArg = curArg->m_nextArg; + } #endif // DEBUG - handlerMemoryPhi = new (m_pCompiler) BasicBlock::MemoryPhiArg(ssaNum, handlerMemoryPhi); - } + handlerMemoryPhi = new (m_pCompiler) BasicBlock::MemoryPhiArg(ssaNum, handlerMemoryPhi); + } - DBG_SSA_JITDUMP(" Added phi arg u:%d for %s to phi defn in handler block " FMT_BB ".\n", ssaNum, - memoryKindNames[memoryKind], memoryKind, handler->bbNum); + DBG_SSA_JITDUMP(" Added phi arg u:%d for %s to phi defn in handler block " FMT_BB ".\n", ssaNum, + memoryKindNames[memoryKind], memoryKind, succ->bbNum); - if ((memoryKind == ByrefExposed) && m_pCompiler->byrefStatesMatchGcHeapStates) - { - // Share the phi between GcHeap and ByrefExposed. - handler->bbMemorySsaPhiFunc[GcHeap] = handlerMemoryPhi; - } - } - unsigned tryInd = tryBlk->ebdEnclosingTryIndex; - if (tryInd == EHblkDsc::NO_ENCLOSING_INDEX) - { - break; - } - tryBlk = m_pCompiler->ehGetDsc(tryInd); + if ((memoryKind == ByrefExposed) && m_pCompiler->byrefStatesMatchGcHeapStates) + { + // Share the phi between GcHeap and ByrefExposed. + succ->bbMemorySsaPhiFunc[GcHeap] = handlerMemoryPhi; } - } + + return BasicBlockVisit::Continue; + }); } //------------------------------------------------------------------------ @@ -1134,7 +1122,10 @@ void SsaBuilder::BlockRenameVariables(BasicBlock* block) { unsigned ssaNum = m_pCompiler->lvMemoryPerSsaData.AllocSsaNum(m_allocator); m_renameStack.PushMemory(memoryKind, block, ssaNum); - AddMemoryDefToHandlerPhis(memoryKind, block, ssaNum); + if (block->HasPotentialEHSuccs(m_pCompiler)) + { + AddMemoryDefToEHSuccessorPhis(memoryKind, block, ssaNum); + } block->bbMemorySsaNumOut[memoryKind] = ssaNum; } diff --git a/src/coreclr/jit/ssabuilder.h b/src/coreclr/jit/ssabuilder.h index 2251553735567..7609681da5437 100644 --- a/src/coreclr/jit/ssabuilder.h +++ b/src/coreclr/jit/ssabuilder.h @@ -86,14 +86,14 @@ class SsaBuilder void RenameLclUse(GenTreeLclVarCommon* lclNode, BasicBlock* block); // Assumes that "block" contains a definition for local var "lclNum", with SSA number "ssaNum". - // IF "block" is within one or more try blocks, - // and the local variable is live at the start of the corresponding handlers, + // IF "block" is within one or more blocks with EH successors, + // and the local variable is live at the start of the corresponding successors, // add this SSA number "ssaNum" to the argument list of the phi for the variable in the start // block of those handlers. - void AddDefToHandlerPhis(BasicBlock* block, unsigned lclNum, unsigned ssaNum); + void AddDefToEHSuccessorPhis(BasicBlock* block, unsigned lclNum, unsigned ssaNum); // Same as above, for memory. - void AddMemoryDefToHandlerPhis(MemoryKind memoryKind, BasicBlock* block, unsigned ssaNum); + void AddMemoryDefToEHSuccessorPhis(MemoryKind memoryKind, BasicBlock* block, unsigned ssaNum); // Add GT_PHI_ARG nodes to the GT_PHI nodes within block's successors. void AddPhiArgsToSuccessors(BasicBlock* block);