diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index dcfbcdb9884630..1d6ac2301c45dd 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -156,10 +156,6 @@ void CodeGen::genCodeForBBlist() genMarkLabelsForCodegen(); - assert(!compiler->fgFirstBBScratch || - compiler->fgFirstBB == compiler->fgFirstBBScratch); // compiler->fgFirstBBScratch - // has to be first. - /* Initialize structures used in the block list iteration */ genInitialize(); @@ -367,9 +363,9 @@ void CodeGen::genCodeForBBlist() siBeginBlock(block); // BBF_INTERNAL blocks don't correspond to any single IL instruction. - if (compiler->opts.compDbgInfo && block->HasFlag(BBF_INTERNAL) && - !compiler->fgBBisScratch(block)) // If the block is the distinguished first scratch block, then no need to - // emit a NO_MAPPING entry, immediately after the prolog. + // Add a NoMapping entry unless this is right after the prolog where it + // is unnecessary. + if (compiler->opts.compDbgInfo && block->HasFlag(BBF_INTERNAL) && !block->IsFirst()) { genIPmappingAdd(IPmappingDscKind::NoMapping, DebugInfo(), true); } @@ -388,17 +384,17 @@ void CodeGen::genCodeForBBlist() #ifdef SWIFT_SUPPORT // Reassemble Swift struct parameters on the local stack frame in the - // scratch BB right after the prolog. There can be arbitrary amounts of + // init BB right after the prolog. There can be arbitrary amounts of // codegen related to doing this, so it cannot be done in the prolog. - if (compiler->fgBBisScratch(block) && compiler->lvaHasAnySwiftStackParamToReassemble()) + if (block->IsFirst() && compiler->lvaHasAnySwiftStackParamToReassemble()) { genHomeSwiftStructParameters(/* handleStack */ true); } #endif - // Emit poisoning into scratch BB that comes right after prolog. + // Emit poisoning into the init BB that comes right after prolog. // We cannot emit this code in the prolog as it might make the prolog too large. - if (compiler->compShouldPoisonFrame() && compiler->fgBBisScratch(block)) + if (compiler->compShouldPoisonFrame() && block->IsFirst()) { genPoisonFrame(newLiveRegSet); } diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 16f5b55ca27289..4e9577254eb617 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -3743,20 +3743,6 @@ void Compiler::compInitDebuggingInfo() compInitScopeLists(); } - if (opts.compDbgCode && (info.compVarScopesCount > 0)) - { - /* Create a new empty basic block. fgExtendDbgLifetimes() may add - initialization of variables which are in scope right from the - start of the (real) first BB (and therefore artificially marked - as alive) into this block. - */ - - fgEnsureFirstBBisScratch(); - - fgNewStmtAtEnd(fgFirstBB, gtNewNothingNode()); - - JITDUMP("Debuggable code - Add new %s to perform initialization of variables\n", fgFirstBB->dspToString()); - } /*------------------------------------------------------------------------- * * Read the stmt-offsets table and the line-number table @@ -4523,6 +4509,9 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl activePhaseChecks |= PhaseChecks::CHECK_PROFILE; DoPhase(this, PHASE_INCPROFILE, &Compiler::fgIncorporateProfileData); + activePhaseChecks |= PhaseChecks::CHECK_FG_INIT_BLOCK; + DoPhase(this, PHASE_CANONICALIZE_ENTRY, &Compiler::fgCanonicalizeFirstBB); + // If we are doing OSR, update flow to initially reach the appropriate IL offset. // if (opts.IsOSR()) @@ -4691,10 +4680,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl if (opts.OptimizationEnabled()) { - // Canonicalize entry to have unique entry BB to put IR in for the upcoming phases - // - DoPhase(this, PHASE_CANONICALIZE_ENTRY, &Compiler::fgCanonicalizeFirstBB); - // Build post-order and remove dead blocks // DoPhase(this, PHASE_DFS_BLOCKS2, &Compiler::fgDfsBlocksAndRemove); @@ -4789,10 +4774,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl return fgHeadTailMerge(false); }); - // Canonicalize entry to give a unique dominator tree root - // - DoPhase(this, PHASE_CANONICALIZE_ENTRY, &Compiler::fgCanonicalizeFirstBB); - // Compute DFS tree and remove all unreachable blocks. // DoPhase(this, PHASE_DFS_BLOCKS3, &Compiler::fgDfsBlocksAndRemove); @@ -5024,12 +5005,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl assert(opts.optRepeat); - // We may have optimized away the canonical entry BB that SSA - // depends on above, so if we are going for another iteration then - // make sure we still have a canonical entry. - // - DoPhase(this, PHASE_CANONICALIZE_ENTRY, &Compiler::fgCanonicalizeFirstBB); - ResetOptAnnotations(); RecomputeFlowGraphAnnotations(); diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4acd093a08af5b..0456c67a10bd87 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1542,6 +1542,7 @@ enum class PhaseChecks : unsigned int CHECK_LIKELIHOODS = 1 << 5, // profile data likelihood integrity CHECK_PROFILE = 1 << 6, // profile data full integrity CHECK_LINKED_LOCALS = 1 << 7, // check linked list of locals + CHECK_FG_INIT_BLOCK = 1 << 8, // flow graph has an init block }; inline constexpr PhaseChecks operator ~(PhaseChecks a) @@ -5200,8 +5201,6 @@ class Compiler BasicBlock* fgEntryBB = nullptr; // For OSR, the original method's entry point BasicBlock* fgOSREntryBB = nullptr; // For OSR, the logical entry point (~ patchpoint) BasicBlock* fgFirstFuncletBB = nullptr; // First block of outlined funclets (to allow block insertion before the funclets) - BasicBlock* fgFirstBBScratch = nullptr; // Block inserted for initialization stuff. Is nullptr if no such block has been - // created. BasicBlockList* fgReturnBlocks = nullptr; // list of BBJ_RETURN blocks unsigned fgEdgeCount = 0; // # of control flow edges between the BBs unsigned fgBBcount = 0; // # of BBs in the method (in the linked list that starts with fgFirstBB) @@ -5249,10 +5248,6 @@ class Compiler return getAllocator(cmk).allocate(fgBBNumMax + 1); } - bool fgEnsureFirstBBisScratch(); - bool fgFirstBBisScratch(); - bool fgBBisScratch(BasicBlock* block); - void fgExtendEHRegionBefore(BasicBlock* block); void fgExtendEHRegionAfter(BasicBlock* block); @@ -5443,6 +5438,7 @@ class Compiler }; PhaseStatus fgMorphBlocks(); + BasicBlock* fgGetFirstILBlock(); void fgMorphBlock(BasicBlock* block, MorphUnreachableInfo* unreachableInfo = nullptr); void fgMorphStmts(BasicBlock* block); @@ -6162,6 +6158,7 @@ class Compiler bool fgCheckRemoveStmt(BasicBlock* block, Statement* stmt); PhaseStatus fgCanonicalizeFirstBB(); + void fgCreateNewInitBB(); void fgSetEHRegionForNewPreheaderOrExit(BasicBlock* preheader); @@ -6183,6 +6180,8 @@ class Compiler bool fgCanCompactBlock(BasicBlock* block); + bool fgCanCompactInitBlock(); + void fgCompactBlock(BasicBlock* block); BasicBlock* fgConnectFallThrough(BasicBlock* bSrc, BasicBlock* bDst); @@ -6359,6 +6358,7 @@ class Compiler void fgDebugCheckBBNumIncreasing(); void fgDebugCheckBBlist(bool checkBBNum = false, bool checkBBRefs = true); void fgDebugCheckBlockLinks(); + void fgDebugCheckInitBB(); void fgDebugCheckLinks(bool morphTrees = false); void fgDebugCheckStmtsList(BasicBlock* block, bool morphTrees); void fgDebugCheckNodeLinks(BasicBlock* block, Statement* stmt); diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 30a3a83581a3bb..320ad81906c310 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -8,167 +8,102 @@ // Flowgraph Construction and Maintenance -//------------------------------------------------------------------------ -// fgEnsureFirstBBisScratch: Ensure that fgFirstBB is a scratch BasicBlock +//------------------------------------------------------------------------------ +// fgCanonicalizeFirstBB: Canonicalize the method entry to be dominate all +// blocks in the BB and to be executed exactly once. // // Returns: -// True, if a new basic block was allocated. -// -// Notes: -// This should be called before adding on-entry initialization code to -// the method, to ensure that fgFirstBB is not part of a loop. -// -// Does nothing, if fgFirstBB is already a scratch BB. After calling this, -// fgFirstBB may already contain code. Callers have to be careful -// that they do not mess up the order of things added to this block and -// inadvertently change semantics. -// -// We maintain the invariant that a scratch BB ends with BBJ_ALWAYS, -// so that when adding independent bits of initialization, -// callers can generally append to the fgFirstBB block without worrying -// about what code is there already. +// Suitable phase status. // -// Can be called at any time, and can be called multiple times. -// -bool Compiler::fgEnsureFirstBBisScratch() +PhaseStatus Compiler::fgCanonicalizeFirstBB() { - // Have we already allocated a scratch block? - if (fgFirstBBisScratch()) + if (fgFirstBB->hasTryIndex()) { - return false; + JITDUMP("Canonicalizing entry because it currently is the beginning of a try region\n"); + } + else if (fgFirstBB->bbPreds != nullptr) + { + JITDUMP("Canonicalizing entry because it currently has predecessors\n"); + } + else if (opts.compDbgCode && !fgFirstBB->HasFlag(BBF_INTERNAL)) + { + // For debug ensure the first BB is internal so as to not conflate user + // code with JIT added code. + JITDUMP("Canonicalizing entry because it currently is a user BB and we are generating debug code\n"); + } + else + { + return PhaseStatus::MODIFIED_NOTHING; } - assert(fgFirstBBScratch == nullptr); - - BasicBlock* block; + fgCreateNewInitBB(); + return PhaseStatus::MODIFIED_EVERYTHING; +} - if (fgFirstBB != nullptr) - { - // The first block has an implicit ref count which we must - // remove. Note the ref count could be greater than one, if - // the first block is not scratch and is targeted by a - // branch. - assert(fgFirstBB->bbRefs >= 1); - fgFirstBB->bbRefs--; +//------------------------------------------------------------------------------ +// fgCreateNewInitBB: +// Create a new init BB at the beginning of the function. +// +void Compiler::fgCreateNewInitBB() +{ + // The first block has an implicit ref count which we must remove. Note the + // ref count could be greater than one, if the first block is targeted by a + // branch. + assert(fgFirstBB->bbRefs >= 1); + fgFirstBB->bbRefs--; - block = BasicBlock::New(this); + BasicBlock* block = BasicBlock::New(this); - // If we have profile data determine the weight of the scratch BB + // If we have profile data determine the weight of the initBB BB + // + if (fgFirstBB->hasProfileWeight()) + { + // If current entry has preds, sum up those weights // - if (fgFirstBB->hasProfileWeight()) + weight_t nonEntryWeight = 0; + for (FlowEdge* const edge : fgFirstBB->PredEdges()) { - // If current entry has preds, sum up those weights - // - weight_t nonEntryWeight = 0; - for (FlowEdge* const edge : fgFirstBB->PredEdges()) - { - nonEntryWeight += edge->getLikelyWeight(); - } + nonEntryWeight += edge->getLikelyWeight(); + } - // entry weight is weight not from any pred + // entry weight is weight not from any pred + // + weight_t const entryWeight = fgFirstBB->bbWeight - nonEntryWeight; + if (entryWeight <= 0) + { + // If the result is clearly nonsensical, just inherit // - weight_t const entryWeight = fgFirstBB->bbWeight - nonEntryWeight; - if (entryWeight <= 0) - { - // If the result is clearly nonsensical, just inherit - // - JITDUMP( - "\fgEnsureFirstBBisScratch: Profile data could not be locally repaired. Data %s inconsistent.\n", + JITDUMP("\fgCanonicalizeFirstBB: Profile data could not be locally repaired. Data %s inconsistent.\n", fgPgoConsistent ? "is now" : "was already"); - if (fgPgoConsistent) - { - Metrics.ProfileInconsistentScratchBB++; - fgPgoConsistent = false; - } - - block->inheritWeight(fgFirstBB); - } - else + if (fgPgoConsistent) { - block->setBBProfileWeight(entryWeight); + Metrics.ProfileInconsistentScratchBB++; + fgPgoConsistent = false; } - } - // The new scratch bb will fall through to the old first bb - FlowEdge* const edge = fgAddRefPred(fgFirstBB, block); - block->SetKindAndTargetEdge(BBJ_ALWAYS, edge); - fgInsertBBbefore(fgFirstBB, block); - } - else - { - noway_assert(fgLastBB == nullptr); - block = BasicBlock::New(this, BBJ_ALWAYS); - fgFirstBB = block; - fgLastBB = block; + block->inheritWeight(fgFirstBB); + } + else + { + block->setBBProfileWeight(entryWeight); + } } - noway_assert(fgLastBB != nullptr); + // The new scratch bb will fall through to the old first bb + FlowEdge* const edge = fgAddRefPred(fgFirstBB, block); + block->SetKindAndTargetEdge(BBJ_ALWAYS, edge); + fgInsertBBbefore(fgFirstBB, block); // Set the expected flags - block->SetFlags(BBF_INTERNAL | BBF_IMPORTED); + block->SetFlags(BBF_INTERNAL); // This new first BB has an implicit ref, and no others. // - // But if we call this early, before fgLinkBasicBlocks, - // defer and let it handle adding the implicit ref. - // - block->bbRefs = fgPredsComputed ? 1 : 0; - - fgFirstBBScratch = fgFirstBB; - -#ifdef DEBUG - if (verbose) - { - printf("New scratch " FMT_BB "\n", block->bbNum); - } -#endif - - return true; -} - -//------------------------------------------------------------------------ -// fgFirstBBisScratch: Check if fgFirstBB is a scratch block -// -// Returns: -// true if fgFirstBB is a scratch block. -// -bool Compiler::fgFirstBBisScratch() -{ - if (fgFirstBBScratch != nullptr) - { - assert(fgFirstBBScratch == fgFirstBB); - assert(fgFirstBBScratch->HasFlag(BBF_INTERNAL)); - if (fgPredsComputed) - { - assert(fgFirstBBScratch->countOfInEdges() == 1); - } - - // Normally, the first scratch block is a fall-through block. However, if the block after it was an empty - // BBJ_ALWAYS block, it might get removed, and the code that removes it will make the first scratch block - // a BBJ_ALWAYS block. - assert(fgFirstBBScratch->KindIs(BBJ_ALWAYS)); - - return true; - } - else - { - return false; - } -} + assert(fgPredsComputed); + block->bbRefs = 1; -//------------------------------------------------------------------------ -// fgBBisScratch: Check if a given block is a scratch block. -// -// Arguments: -// block - block in question -// -// Returns: -// true if this block is the first block and is a scratch block. -// -bool Compiler::fgBBisScratch(BasicBlock* block) -{ - return fgFirstBBisScratch() && (block == fgFirstBB); + JITDUMP("New init " FMT_BB "\n", block->bbNum); } /* @@ -4071,8 +4006,9 @@ void Compiler::fgFixEntryFlowForOSR() // Now branch from method start to the OSR entry. // - fgEnsureFirstBBisScratch(); - assert(fgFirstBB->KindIs(BBJ_ALWAYS) && fgFirstBB->JumpsToNext()); + fgCreateNewInitBB(); + assert(fgFirstBB->KindIs(BBJ_ALWAYS)); + fgRedirectTargetEdge(fgFirstBB, fgOSREntryBB); // We don't know the right weight for this block, since @@ -4952,22 +4888,9 @@ void Compiler::fgUnlinkBlock(BasicBlock* block) { assert(block == fgFirstBB); assert(block != fgLastBB); - assert((fgFirstBBScratch == nullptr) || (fgFirstBBScratch == fgFirstBB)); fgFirstBB = block->Next(); fgFirstBB->SetPrevToNull(); - - if (fgFirstBBScratch != nullptr) - { -#ifdef DEBUG - // We had created an initial scratch BB, but now we're deleting it. - if (verbose) - { - printf("Unlinking scratch " FMT_BB "\n", block->bbNum); - } -#endif // DEBUG - fgFirstBBScratch = nullptr; - } } else if (block->IsLast()) { @@ -5126,15 +5049,6 @@ BasicBlock* Compiler::fgRemoveBlock(BasicBlock* block, bool unreachable) BasicBlock* succBlock = block->GetTarget(); - bool skipUnmarkLoop = false; - - if (succBlock->isLoopHead() && bPrev && (succBlock->bbNum <= bPrev->bbNum)) - { - // It looks like `block` is the source of a back edge of a loop, and once we remove `block` the - // loop will still exist because we'll move the edge to `bPrev`. So, don't unscale the loop blocks. - skipUnmarkLoop = true; - } - // Update fgFirstFuncletBB if necessary if (block == fgFirstFuncletBB) { @@ -5925,7 +5839,7 @@ BasicBlock* Compiler::fgNewBBFromTreeAfter( */ void Compiler::fgInsertBBbefore(BasicBlock* insertBeforeBlk, BasicBlock* newBlk) { - if (fgFirstBB == insertBeforeBlk) + if (insertBeforeBlk == fgFirstBB) { newBlk->SetNext(fgFirstBB); @@ -5937,8 +5851,7 @@ void Compiler::fgInsertBBbefore(BasicBlock* insertBeforeBlk, BasicBlock* newBlk) fgInsertBBafter(insertBeforeBlk->Prev(), newBlk); } - /* Update fgFirstFuncletBB if insertBeforeBlk is the first block of the funclet region. */ - if (fgFirstFuncletBB == insertBeforeBlk) + if (insertBeforeBlk == fgFirstFuncletBB) { fgFirstFuncletBB = newBlk; } diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 0abf553f2f0c60..cc65f6269fd315 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -2905,7 +2905,6 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef } fgDebugCheckBlockLinks(); - fgFirstBBisScratch(); if (fgBBcount > 10000 && expensiveDebugCheckLevel < 1) { @@ -3217,6 +3216,17 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef } } +//------------------------------------------------------------------------ +// fgDebugCheckInitBB: Check that the first BB is a valid init BB. +// +void Compiler::fgDebugCheckInitBB() +{ + assert(fgFirstBB != nullptr); + assert(!fgFirstBB->hasTryIndex()); + assert(fgFirstBB->bbPreds == nullptr); + assert(!opts.compDbgCode || fgFirstBB->HasFlag(BBF_INTERNAL)); +} + //------------------------------------------------------------------------ // fgDebugCheckTypes: Validate node types used in the given tree // @@ -4712,7 +4722,9 @@ void Compiler::fgDebugCheckFlowGraphAnnotations() { if (m_dfsTree == nullptr) { - assert((m_loops == nullptr) && (m_domTree == nullptr) && (m_reachabilitySets == nullptr)); + assert(m_loops == nullptr); + assert((m_domTree == nullptr) && (m_domFrontiers == nullptr)); + assert(m_reachabilitySets == nullptr); return; } diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 9d80caeb5db3d5..834a53be776a61 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -823,9 +823,9 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block) return false; } - // Don't compact the first block if it was specially created as a scratch block. + // Ensure we leave a valid init BB around. // - if (fgBBisScratch(block)) + if ((block == fgFirstBB) && !fgCanCompactInitBlock()) { return false; } @@ -851,6 +851,40 @@ bool Compiler::fgCanCompactBlock(BasicBlock* block) return true; } +//------------------------------------------------------------- +// fgCanCompactInitBlock: Check if the first BB (the init BB) can be compacted +// into its target. +// +// Returns: +// true if compaction is allowed +// +bool Compiler::fgCanCompactInitBlock() +{ + assert(fgFirstBB->KindIs(BBJ_ALWAYS)); + BasicBlock* target = fgFirstBB->GetTarget(); + if (target->hasTryIndex()) + { + // Inside a try region + return false; + } + + assert(target->bbPreds != nullptr); + if (target->bbPreds->getNextPredEdge() != nullptr) + { + // Multiple preds + return false; + } + + if (opts.compDbgCode && !target->HasFlag(BBF_INTERNAL)) + { + // Init BB must be internal for debug code to avoid conflating + // JIT-inserted code with user code. + return false; + } + + return true; +} + //------------------------------------------------------------- // fgCompactBlock: Compact BBJ_ALWAYS block and its target into one. // @@ -1389,7 +1423,7 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block) if (bPrev == nullptr) { assert(block == fgFirstBB); - if (!block->JumpsToNext()) + if (!block->JumpsToNext() || !fgCanCompactInitBlock()) { break; } @@ -1401,8 +1435,9 @@ bool Compiler::fgOptimizeEmptyBlock(BasicBlock* block) break; } - // Don't remove fgEntryBB - if (block == fgEntryBB) + // Don't remove the init BB if it does not leave a proper init BB + // in place + if ((block == fgFirstBB) && !fgCanCompactInitBlock()) { break; } @@ -2161,11 +2196,6 @@ bool Compiler::fgOptimizeUncondBranchToSimpleCond(BasicBlock* block, BasicBlock* return false; } - if (fgBBisScratch(block)) - { - return false; - } - unsigned lclNum = BAD_VAR_NUM; // First check if the successor tests a local and then branches on the result @@ -2525,12 +2555,6 @@ bool Compiler::fgOptimizeBranch(BasicBlock* bJump) return false; } - // Don't hoist a conditional branch into the scratch block; we'd prefer it stay BBJ_ALWAYS. - if (fgBBisScratch(bJump)) - { - return false; - } - BasicBlock* bDest = bJump->GetTarget(); if (!bDest->KindIs(BBJ_COND)) @@ -6481,9 +6505,9 @@ PhaseStatus Compiler::fgHeadTailMerge(bool early) Statement* const stmt = info.m_stmt; BasicBlock* const predBlock = info.m_block; - // Never pick the scratch block as the victim as that would + // Never pick the init block as the victim as that would // cause us to add a predecessor to it, which is invalid. - if (fgBBisScratch(predBlock)) + if (predBlock == fgFirstBB) { continue; } diff --git a/src/coreclr/jit/fgprofile.cpp b/src/coreclr/jit/fgprofile.cpp index 18d96b3bac19a8..0b8f4378c48270 100644 --- a/src/coreclr/jit/fgprofile.cpp +++ b/src/coreclr/jit/fgprofile.cpp @@ -4474,7 +4474,7 @@ bool Compiler::fgComputeCalledCount(weight_t returnWeight) // If we allocated a scratch block as the first BB then we need // to set its profile-derived weight to be fgCalledCount - if (fgFirstBBisScratch()) + if (fgFirstBB->HasFlag(BBF_INTERNAL)) { fgFirstBB->setBBProfileWeight(fgCalledCount); madeChanges = true; diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 91d62de8316b75..72b9d0dc34774a 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -1388,14 +1388,9 @@ void Compiler::fgAddSyncMethodEnterExit() NYI("No support for synchronized methods"); #endif // !FEATURE_EH - // Create a scratch first BB where we can put the new variable initialization. - // Don't put the scratch BB in the protected region. - - fgEnsureFirstBBisScratch(); - // Create a block for the start of the try region, where the monitor enter call // will go. - BasicBlock* const tryBegBB = fgSplitBlockAtEnd(fgFirstBB); + BasicBlock* const tryBegBB = fgSplitBlockAtBeginning(fgFirstBB); BasicBlock* const tryLastBB = fgLastBB; // Create a block for the fault. @@ -1517,7 +1512,7 @@ void Compiler::fgAddSyncMethodEnterExit() GenTree* zero = gtNewZeroConNode(typeMonAcquired); GenTree* initNode = gtNewStoreLclVarNode(lvaMonAcquired, zero); - fgNewStmtAtEnd(fgFirstBB, initNode); + fgNewStmtAtBeg(fgFirstBB, initNode); #ifdef DEBUG if (verbose) @@ -1543,7 +1538,7 @@ void Compiler::fgAddSyncMethodEnterExit() GenTree* thisNode = gtNewLclVarNode(info.compThisArg); GenTree* initNode = gtNewStoreLclVarNode(lvaCopyThis, thisNode); - fgNewStmtAtEnd(tryBegBB, initNode); + fgNewStmtAtBeg(tryBegBB, initNode); } // For OSR, we do not need the enter tree as the monitor is acquired by the original method. @@ -1603,38 +1598,45 @@ GenTree* Compiler::fgCreateMonitorTree(unsigned lvaMonAcquired, unsigned lvaThis } #endif - if (block->KindIs(BBJ_RETURN) && block->lastStmt()->GetRootNode()->OperIs(GT_RETURN)) + if (enter) { - GenTreeUnOp* retNode = block->lastStmt()->GetRootNode()->AsUnOp(); - GenTree* retExpr = retNode->gtOp1; - - if (retExpr != nullptr) + fgNewStmtAtBeg(block, tree); + } + else + { + if (block->KindIs(BBJ_RETURN) && block->lastStmt()->GetRootNode()->OperIs(GT_RETURN)) { - // have to insert this immediately before the GT_RETURN so we transform: - // ret(...) -> - // ret(comma(comma(tmp=...,call mon_exit), tmp)) - // - TempInfo tempInfo = fgMakeTemp(retExpr); - GenTree* lclVar = tempInfo.load; + GenTreeUnOp* retNode = block->lastStmt()->GetRootNode()->AsUnOp(); + GenTree* retExpr = retNode->gtOp1; - // TODO-1stClassStructs: delete this NO_CSE propagation. Requires handling multi-regs in copy prop. - lclVar->gtFlags |= (retExpr->gtFlags & GTF_DONT_CSE); + if (retExpr != nullptr) + { + // have to insert this immediately before the GT_RETURN so we transform: + // ret(...) -> + // ret(comma(comma(tmp=...,call mon_exit), tmp)) + // + TempInfo tempInfo = fgMakeTemp(retExpr); + GenTree* lclVar = tempInfo.load; - retExpr = gtNewOperNode(GT_COMMA, lclVar->TypeGet(), tree, lclVar); - retExpr = gtNewOperNode(GT_COMMA, lclVar->TypeGet(), tempInfo.store, retExpr); - retNode->gtOp1 = retExpr; - retNode->AddAllEffectsFlags(retExpr); + // TODO-1stClassStructs: delete this NO_CSE propagation. Requires handling multi-regs in copy prop. + lclVar->gtFlags |= (retExpr->gtFlags & GTF_DONT_CSE); + + retExpr = gtNewOperNode(GT_COMMA, lclVar->TypeGet(), tree, lclVar); + retExpr = gtNewOperNode(GT_COMMA, lclVar->TypeGet(), tempInfo.store, retExpr); + retNode->gtOp1 = retExpr; + retNode->AddAllEffectsFlags(retExpr); + } + else + { + // Insert this immediately before the GT_RETURN + fgNewStmtNearEnd(block, tree); + } } else { - // Insert this immediately before the GT_RETURN - fgNewStmtNearEnd(block, tree); + fgNewStmtAtEnd(block, tree); } } - else - { - fgNewStmtAtEnd(block, tree); - } return tree; } @@ -1727,8 +1729,6 @@ void Compiler::fgAddReversePInvokeEnterExit() tree = gtNewHelperCallNode(CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER, TYP_VOID, pInvokeFrameVar); } - fgEnsureFirstBBisScratch(); - fgNewStmtAtBeg(fgFirstBB, tree); #ifdef DEBUG @@ -2307,15 +2307,6 @@ PhaseStatus Compiler::fgAddInternal() // type with a runtime lookup madeChanges |= fgCreateFiltersForGenericExceptions(); - // The backend requires a scratch BB into which it can safely insert a P/Invoke method prolog if one is - // required. Similarly, we need a scratch BB for poisoning and when we have Swift parameters to reassemble. - // Create it here. - if (compMethodRequiresPInvokeFrame() || compShouldPoisonFrame() || lvaHasAnySwiftStackParamToReassemble()) - { - madeChanges |= fgEnsureFirstBBisScratch(); - fgFirstBB->SetFlags(BBF_DONT_REMOVE); - } - /* VSW441487 @@ -2351,8 +2342,7 @@ PhaseStatus Compiler::fgAddInternal() // Now assign the original input "this" to the temp. GenTree* store = gtNewStoreLclVarNode(lvaArg0Var, gtNewLclVarNode(info.compThisArg)); - fgEnsureFirstBBisScratch(); - fgNewStmtAtEnd(fgFirstBB, store); + fgNewStmtAtBeg(fgFirstBB, store); JITDUMP("\nCopy \"this\" to lvaArg0Var in first basic block %s\n", fgFirstBB->dspToString()); DISPTREE(store); @@ -2481,8 +2471,7 @@ PhaseStatus Compiler::fgAddInternal() // Stick the conditional call at the start of the method - fgEnsureFirstBBisScratch(); - fgNewStmtAtEnd(fgFirstBB, gtNewQmarkNode(TYP_VOID, guardCheckCond, callback->AsColon())); + fgNewStmtAtBeg(fgFirstBB, gtNewQmarkNode(TYP_VOID, guardCheckCond, callback->AsColon())); madeChanges = true; } @@ -2509,10 +2498,7 @@ PhaseStatus Compiler::fgAddInternal() tree = gtNewHelperCallNode(CORINFO_HELP_MON_ENTER, TYP_VOID, tree); - /* Create a new basic block and stick the call in it */ - - fgEnsureFirstBBisScratch(); - fgNewStmtAtEnd(fgFirstBB, tree); + fgNewStmtAtBeg(fgFirstBB, tree); #ifdef DEBUG if (verbose) diff --git a/src/coreclr/jit/gschecks.cpp b/src/coreclr/jit/gschecks.cpp index ed2d7538a39787..558c30f6b35769 100644 --- a/src/coreclr/jit/gschecks.cpp +++ b/src/coreclr/jit/gschecks.cpp @@ -525,25 +525,68 @@ void Compiler::gsParamsToShadows() call->gtArgs.PushBack(this, NewCallArg::Primitive(dst)); call->gtArgs.PushBack(this, NewCallArg::Primitive(src)); - fgEnsureFirstBBisScratch(); compCurBB = fgFirstBB; // Needed by some morphing if (opts.IsReversePInvoke()) { - JITDUMP( - "Inserting special copy helper call at the end of the first block after Reverse P/Invoke transition\n"); + // If we are in a reverse P/Invoke, then we need to insert + // the call at the end of the first block as we need to do the GC transition + // before we can call the helper. + // + // TODO-Cleanup: These gymnastics indicate that we are + // inserting reverse pinvoke transitions way too early in the + // JIT. -#ifdef DEBUG - // assert that we don't have any uses of the local variable in the first block - // before we insert the shadow copy statement. + struct HasReversePInvokeEnterVisitor : GenTreeVisitor + { + enum + { + DoPreOrder = true, + }; + + HasReversePInvokeEnterVisitor(Compiler* comp) + : GenTreeVisitor(comp) + { + } + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + if (((*use)->gtFlags & GTF_CALL) == 0) + { + return fgWalkResult::WALK_SKIP_SUBTREES; + } + + if ((*use)->IsHelperCall(m_compiler, CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER) || + (*use)->IsHelperCall(m_compiler, CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER_TRACK_TRANSITIONS)) + { + return fgWalkResult::WALK_ABORT; + } + + return fgWalkResult::WALK_CONTINUE; + } + }; + + HasReversePInvokeEnterVisitor checker(this); + + Statement* reversePInvokeStmt = nullptr; for (Statement* const stmt : fgFirstBB->Statements()) { + // assert that we don't have any uses of the local variable + // at the point before we insert the shadow copy statement. assert(!gtHasRef(stmt->GetRootNode(), lclNum)); + + if (checker.WalkTree(stmt->GetRootNodePointer(), nullptr) == fgWalkResult::WALK_ABORT) + { + reversePInvokeStmt = stmt; + break; + } } -#endif - // If we are in a reverse P/Invoke, then we need to insert - // the call at the end of the first block as we need to do the GC transition - // before we can call the helper. - (void)fgNewStmtAtEnd(fgFirstBB, fgMorphTree(call)); + + noway_assert(reversePInvokeStmt != nullptr); + + JITDUMP("Inserting special copy helper call after Reverse P/Invoke transition " FMT_STMT "\n", + reversePInvokeStmt->GetID()); + + (void)fgInsertStmtAfter(fgFirstBB, reversePInvokeStmt, gtNewStmt(fgMorphTree(call))); } else { @@ -559,9 +602,8 @@ void Compiler::gsParamsToShadows() GenTree* store = gtNewStoreLclVarNode(shadowVarNum, src); - fgEnsureFirstBBisScratch(); compCurBB = fgFirstBB; // Needed by some morphing - (void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store)); + fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store)); } } compCurBB = nullptr; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 033ea50dc20e26..08336a28d553a9 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1274,7 +1274,7 @@ var_types Compiler::impImportCall(OPCODE opcode, { // For normal jitting we may branch back to the firstBB; this // should already be imported. - loopHead = fgFirstBB; + loopHead = fgGetFirstILBlock(); } JITDUMP("\nTail recursive call [%06u] in the method. Mark " FMT_BB " to " FMT_BB diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 53e43858ace0a9..63a03c60975853 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -5856,9 +5856,6 @@ void Lowering::InsertPInvokeMethodProlog() JITDUMP("======= Inserting PInvoke method prolog\n"); - // The first BB must be a scratch BB in order for us to be able to safely insert the P/Invoke prolog. - assert(comp->fgFirstBBisScratch()); - LIR::Range& firstBlockRange = LIR::AsRange(comp->fgFirstBB); const CORINFO_EE_INFO* pInfo = comp->eeGetEEInfo(); diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 5748a143d29c9d..56bba3469eb27b 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -2526,7 +2526,6 @@ void LinearScan::buildIntervals() // handling clobbers REG_SCRATCH, so kill it here. if ((block == compiler->fgFirstBB) && compiler->lvaHasAnySwiftStackParamToReassemble()) { - assert(compiler->fgFirstBBisScratch()); addKillForRegs(genRegMask(REG_SCRATCH), currentLoc + 1); currentLoc += 2; } @@ -2536,7 +2535,6 @@ void LinearScan::buildIntervals() // into the scratch register, so it will be killed here. if (compiler->compShouldPoisonFrame() && (block == compiler->fgFirstBB)) { - assert(compiler->fgFirstBBisScratch()); regMaskTP killed; #if defined(TARGET_XARCH) // Poisoning uses EAX for small vars and rep stosd that kills edi, ecx and eax for large vars. diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index c0b7a6417b2980..de3deec965c5c3 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -51,7 +51,6 @@ PhaseStatus Compiler::fgMorphInit() impTokenLookupContextHandle /* context */) & CORINFO_INITCLASS_USE_HELPER) { - fgEnsureFirstBBisScratch(); fgNewStmtAtBeg(fgFirstBB, fgInitThisClass()); madeChanges = true; } @@ -67,8 +66,7 @@ PhaseStatus Compiler::fgMorphInit() GenTree* op = gtNewLclvNode(i, TYP_REF); op = gtNewHelperCallNode(CORINFO_HELP_CHECK_OBJ, TYP_VOID, op); - fgEnsureFirstBBisScratch(); - fgNewStmtAtEnd(fgFirstBB, op); + fgNewStmtAtBeg(fgFirstBB, op); madeChanges = true; if (verbose) { @@ -6744,24 +6742,19 @@ void Compiler::fgMorphRecursiveFastTailCallIntoLoop(BasicBlock* block, GenTreeCa if (opts.IsOSR()) { // Todo: this may not look like a viable loop header. - // Might need the moral equivalent of a scratch BB. + // Might need the moral equivalent of an init BB. FlowEdge* const newEdge = fgAddRefPred(fgEntryBB, block); block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); } else { - // We should have ensured the first BB was scratch - // in morph init... - // assert(doesMethodHaveRecursiveTailcall()); - assert(fgFirstBBisScratch()); - // Loop detection needs to see a pred out of the loop, - // so mark the scratch block BBF_DONT_REMOVE to prevent empty - // block removal on it. - // - fgFirstBB->SetFlags(BBF_DONT_REMOVE); - FlowEdge* const newEdge = fgAddRefPred(fgFirstBB->Next(), block); + // TODO-Cleanup: We should really be expanding tailcalls into loops + // much earlier than this, at a place where we do not need to have + // hacky workarounds to figure out what the actual IL entry block is. + BasicBlock* firstILBB = fgGetFirstILBlock(); + FlowEdge* const newEdge = fgAddRefPred(firstILBB, block); block->SetKindAndTargetEdge(BBJ_ALWAYS, newEdge); } @@ -13510,14 +13503,6 @@ PhaseStatus Compiler::fgMorphBlocks() lvSetMinOptsDoNotEnreg(); } - // Ensure the first BB is scratch if we might need it as a pred for - // the recursive tail call to loop optimization. - // - if (doesMethodHaveRecursiveTailcall()) - { - fgEnsureFirstBBisScratch(); - } - // Morph all blocks. // if (!optLocalAssertionProp) @@ -13544,17 +13529,16 @@ PhaseStatus Compiler::fgMorphBlocks() MorphUnreachableInfo unreachableInfo(this); // Allow edge creation to genReturnBB (target of return merging) - // and the scratch block successor (target for tail call to loop). + // and the first IL BB (target for tail call to loop). // This will also disallow dataflow into these blocks. // if (genReturnBB != nullptr) { genReturnBB->SetFlags(BBF_CAN_ADD_PRED); } - if (fgFirstBBisScratch()) - { - fgFirstBB->Next()->SetFlags(BBF_CAN_ADD_PRED); - } + // TODO-Cleanup: Remove this by transforming tailcalls to loops earlier. + BasicBlock* firstILBB = opts.IsOSR() ? fgEntryBB : fgGetFirstILBlock(); + firstILBB->SetFlags(BBF_CAN_ADD_PRED); // Remember this so we can sanity check that no new blocks will get created. // @@ -13579,10 +13563,7 @@ PhaseStatus Compiler::fgMorphBlocks() { genReturnBB->RemoveFlags(BBF_CAN_ADD_PRED); } - if (fgFirstBBisScratch()) - { - fgFirstBB->Next()->RemoveFlags(BBF_CAN_ADD_PRED); - } + firstILBB->RemoveFlags(BBF_CAN_ADD_PRED); } // Under OSR, we no longer need to specially protect the original method entry @@ -13630,9 +13611,35 @@ PhaseStatus Compiler::fgMorphBlocks() Metrics.MorphLocals = lvaCount; } + // We may have converted a tailcall into a loop, in which case the first BB + // may no longer be canonical. + fgCanonicalizeFirstBB(); + return PhaseStatus::MODIFIED_EVERYTHING; } +//------------------------------------------------------------------------ +// fgGetFirstILBB: Obtain the first basic block that was created due to IL. +// +// Returns: +// The basic block, skipping the init BB. +// +// Remarks: +// TODO-Cleanup: Refactor users to be able to remove this function. +// +BasicBlock* Compiler::fgGetFirstILBlock() +{ + BasicBlock* firstILBB = fgFirstBB; + while (firstILBB->HasFlag(BBF_INTERNAL)) + { + assert(firstILBB->KindIs(BBJ_ALWAYS)); + firstILBB = firstILBB->GetTarget(); + assert((firstILBB != nullptr) && (firstILBB != fgFirstBB)); + } + + return firstILBB; +} + //------------------------------------------------------------------------ // gtRemoveTreesAfterNoReturnCall: // Given a statement that may contain a no-return call, try to find it and diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index c2eae010428a01..35e58d539eea8b 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -5137,33 +5137,6 @@ void Compiler::fgSetEHRegionForNewPreheaderOrExit(BasicBlock* block) } } -//------------------------------------------------------------------------------ -// fgCanonicalizeFirstBB: Canonicalize the method entry for loop and dominator -// purposes. -// -// Returns: -// Suitable phase status. -// -PhaseStatus Compiler::fgCanonicalizeFirstBB() -{ - if (fgFirstBB->hasTryIndex()) - { - JITDUMP("Canonicalizing entry because it currently is the beginning of a try region\n"); - } - else if (fgFirstBB->bbPreds != nullptr) - { - JITDUMP("Canonicalizing entry because it currently has predecessors\n"); - } - else - { - return PhaseStatus::MODIFIED_NOTHING; - } - - assert(!fgFirstBBisScratch()); - fgEnsureFirstBBisScratch(); - return PhaseStatus::MODIFIED_EVERYTHING; -} - LoopSideEffects::LoopSideEffects() : VarInOut(VarSetOps::UninitVal()) , VarUseDef(VarSetOps::UninitVal()) diff --git a/src/coreclr/jit/patchpoint.cpp b/src/coreclr/jit/patchpoint.cpp index 71622ecfc3d759..ab695c0a5c3bb1 100644 --- a/src/coreclr/jit/patchpoint.cpp +++ b/src/coreclr/jit/patchpoint.cpp @@ -47,12 +47,6 @@ class PatchpointTransformer // Number of patchpoints transformed. int Run() { - // If the first block is a patchpoint, insert a scratch block. - if (compiler->fgFirstBB->HasFlag(BBF_PATCHPOINT)) - { - compiler->fgEnsureFirstBBisScratch(); - } - int count = 0; for (BasicBlock* const block : compiler->Blocks(compiler->fgFirstBB->Next())) { @@ -196,8 +190,6 @@ class PatchpointTransformer // ppCounter = void TransformEntry(BasicBlock* block) { - assert(!block->HasFlag(BBF_PATCHPOINT)); - int initialCounterValue = JitConfig.TC_OnStackReplacement_InitialCounter(); if (initialCounterValue < 0) @@ -208,7 +200,7 @@ class PatchpointTransformer GenTree* initialCounterNode = compiler->gtNewIconNode(initialCounterValue, TYP_INT); GenTree* ppCounterStore = compiler->gtNewStoreLclVarNode(ppCounterLclNum, initialCounterNode); - compiler->fgNewStmtNearEnd(block, ppCounterStore); + compiler->fgNewStmtAtBeg(block, ppCounterStore); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/phase.cpp b/src/coreclr/jit/phase.cpp index 9723c123137ea2..0a6299fc1fdeb5 100644 --- a/src/coreclr/jit/phase.cpp +++ b/src/coreclr/jit/phase.cpp @@ -164,6 +164,11 @@ void Phase::PostPhase(PhaseStatus status) comp->fgDebugCheckBBlist(); } + if (hasFlag(checks, PhaseChecks::CHECK_FG_INIT_BLOCK)) + { + comp->fgDebugCheckInitBB(); + } + if (hasFlag(checks, PhaseChecks::CHECK_IR)) { comp->fgDebugCheckLinks();