From 8ce6f0c8c02a8e549aca0613dfa5fffcfd4ecd97 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jun 2024 13:11:29 +0200 Subject: [PATCH 1/7] JIT: Clean up liveness We have a DFS tree available in both early liveness and SSA's liveness, and we can use it to make the data flow cheaper by running in an RPO over the DFS tree. This allows us to propagate the maximal amount of knowledge in each iteration and also to stop the data flow early when there is no cycle in the DFS tree. We do not have the DFS tree available in lowering where we also call liveness. However, lowering was already iterating all blocks to remove dead blocks; switch this to `fgDfsBlocksAndRemove` to remove dead blocks and compute the DFS tree in one go, and remove the old code doing this. Additionally there was a bunch of logic in liveness to consider debug scopes for debug codegen. I'm not sure what this code is doing; we do not run liveness in debug codegen, so seems like it can just be removed. --- src/coreclr/jit/compiler.h | 17 +- src/coreclr/jit/fgopt.cpp | 105 ------ src/coreclr/jit/liveness.cpp | 624 ++--------------------------------- src/coreclr/jit/lower.cpp | 30 +- 4 files changed, 39 insertions(+), 737 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6136c97d5d47b8..f0e74eb12de176 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5525,7 +5525,7 @@ class Compiler void fgAddHandlerLiveVars(BasicBlock* block, VARSET_TP& ehHandlerLiveVars, MemoryKindSet& memoryLiveness); - void fgLiveVarAnalysis(bool updateInternalOnly = false); + void fgLiveVarAnalysis(); void fgComputeLifeCall(VARSET_TP& life, GenTreeCall* call); @@ -5916,8 +5916,6 @@ class Compiler PhaseStatus fgComputeDominators(); // Compute dominators - bool fgRemoveDeadBlocks(); // Identify and remove dead blocks. - public: enum GCPollType { @@ -6678,19 +6676,6 @@ class Compiler void fgMarkUseDef(GenTreeLclVarCommon* tree); - void fgBeginScopeLife(VARSET_TP* inScope, VarScopeDsc* var); - void fgEndScopeLife(VARSET_TP* inScope, VarScopeDsc* var); - - void fgMarkInScope(BasicBlock* block, VARSET_VALARG_TP inScope); - void fgUnmarkInScope(BasicBlock* block, VARSET_VALARG_TP unmarkScope); - - void fgExtendDbgScopes(); - void fgExtendDbgLifetimes(); - -#ifdef DEBUG - void fgDispDebugScopes(); -#endif // DEBUG - //------------------------------------------------------------------------- // // The following keeps track of any code we've added for things like array diff --git a/src/coreclr/jit/fgopt.cpp b/src/coreclr/jit/fgopt.cpp index 7f0c0f2e342a5e..6e36442dd92a42 100644 --- a/src/coreclr/jit/fgopt.cpp +++ b/src/coreclr/jit/fgopt.cpp @@ -162,111 +162,6 @@ bool Compiler::fgRemoveUnreachableBlocks(CanRemoveBlockBody canRemoveBlock) return changed; } -//------------------------------------------------------------------------ -// fgRemoveDeadBlocks: Identify all the unreachable blocks and remove them. -// -bool Compiler::fgRemoveDeadBlocks() -{ - JITDUMP("\n*************** In fgRemoveDeadBlocks()"); - - unsigned prevFgCurBBEpoch = fgCurBBEpoch; - EnsureBasicBlockEpoch(); - - BlockSet visitedBlocks(BlockSetOps::MakeEmpty(this)); - - jitstd::list worklist(jitstd::allocator(getAllocator(CMK_Reachability))); - worklist.push_back(fgFirstBB); - - // Visit all the reachable blocks, everything else can be removed - while (!worklist.empty()) - { - BasicBlock* block = *(worklist.begin()); - worklist.pop_front(); - - if (BlockSetOps::IsMember(this, visitedBlocks, block->bbNum)) - { - continue; - } - - BlockSetOps::AddElemD(this, visitedBlocks, block->bbNum); - - for (BasicBlock* succ : block->Succs(this)) - { - worklist.push_back(succ); - } - - // Add all the "EH" successors. For every `try`, add its handler (including filter) to the worklist. - if (bbIsTryBeg(block)) - { - // Due to EH normalization, a block can only be the start of a single `try` region, with the exception - // of mutually-protect regions. - assert(block->hasTryIndex()); - unsigned tryIndex = block->getTryIndex(); - EHblkDsc* ehDsc = ehGetDsc(tryIndex); - for (;;) - { - worklist.push_back(ehDsc->ebdHndBeg); - if (ehDsc->HasFilter()) - { - worklist.push_back(ehDsc->ebdFilter); - } - tryIndex = ehDsc->ebdEnclosingTryIndex; - if (tryIndex == EHblkDsc::NO_ENCLOSING_INDEX) - { - break; - } - ehDsc = ehGetDsc(tryIndex); - if (ehDsc->ebdTryBeg != block) - { - break; - } - } - } - } - - // Track if there is any unreachable block. Even if it is marked with - // BBF_DONT_REMOVE, fgRemoveUnreachableBlocks() still removes the code - // inside the block. So this variable tracks if we ever found such blocks - // or not. - bool hasUnreachableBlock = false; - - auto isBlockRemovable = [&](BasicBlock* block) -> bool { - const bool isVisited = BlockSetOps::IsMember(this, visitedBlocks, block->bbNum); - const bool isRemovable = !isVisited || (block->bbRefs == 0); - - hasUnreachableBlock |= isRemovable; - return isRemovable; - }; - - bool changed = false; - unsigned iterationCount = 1; - do - { - JITDUMP("\nRemoving unreachable blocks for fgRemoveDeadBlocks iteration #%u\n", iterationCount); - - // Just to be paranoid, avoid infinite loops; fall back to minopts. - if (iterationCount++ > 10) - { - noway_assert(!"Too many unreachable block removal loops"); - } - changed = fgRemoveUnreachableBlocks(isBlockRemovable); - } while (changed); - -#ifdef DEBUG - if (verbose && hasUnreachableBlock) - { - printf("\nAfter dead block removal:\n"); - fgDispBasicBlocks(verboseTrees); - printf("\n"); - } - - fgVerifyHandlerTab(); - fgDebugCheckBBlist(false); -#endif // DEBUG - - return hasUnreachableBlock; -} - //------------------------------------------------------------- // fgComputeDominators: Compute dominators // diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index ef3fedf5b31d32..82df6fcf3d1f8e 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -344,60 +344,6 @@ void Compiler::fgPerBlockLocalVarLiveness() unsigned livenessVarEpoch = GetCurLVEpoch(); - BasicBlock* block; - - // If we don't require accurate local var lifetimes, things are simple. - if (!backendRequiresLocalVarLifetimes()) - { - unsigned lclNum; - LclVarDsc* varDsc; - - VARSET_TP liveAll(VarSetOps::MakeEmpty(this)); - - /* We simply make everything live everywhere */ - - for (lclNum = 0, varDsc = lvaTable; lclNum < lvaCount; lclNum++, varDsc++) - { - if (varDsc->lvTracked) - { - VarSetOps::AddElemD(this, liveAll, varDsc->lvVarIndex); - } - } - - for (block = fgFirstBB; block; block = block->Next()) - { - // Strictly speaking, the assignments for the "Def" cases aren't necessary here. - // The empty set would do as well. Use means "use-before-def", so as long as that's - // "all", this has the right effect. - VarSetOps::Assign(this, block->bbVarUse, liveAll); - VarSetOps::Assign(this, block->bbVarDef, liveAll); - VarSetOps::Assign(this, block->bbLiveIn, liveAll); - block->bbMemoryUse = fullMemoryKindSet; - block->bbMemoryDef = fullMemoryKindSet; - block->bbMemoryLiveIn = fullMemoryKindSet; - block->bbMemoryLiveOut = fullMemoryKindSet; - - switch (block->GetKind()) - { - case BBJ_EHFINALLYRET: - case BBJ_EHFAULTRET: - case BBJ_THROW: - case BBJ_RETURN: - VarSetOps::AssignNoCopy(this, block->bbLiveOut, VarSetOps::MakeEmpty(this)); - break; - default: - VarSetOps::Assign(this, block->bbLiveOut, liveAll); - break; - } - } - - // In minopts, we don't explicitly build SSA or value-number; GcHeap and - // ByrefExposed implicitly (conservatively) change state at each instr. - byrefStatesMatchGcHeapStates = true; - - return; - } - // Avoid allocations in the long case. VarSetOps::AssignNoCopy(this, fgCurUseSet, VarSetOps::MakeEmpty(this)); VarSetOps::AssignNoCopy(this, fgCurDefSet, VarSetOps::MakeEmpty(this)); @@ -406,8 +352,9 @@ void Compiler::fgPerBlockLocalVarLiveness() // memory that is not a GC Heap def. byrefStatesMatchGcHeapStates = true; - for (block = fgFirstBB; block; block = block->Next()) + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { + BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); VarSetOps::ClearD(this, fgCurUseSet); VarSetOps::ClearD(this, fgCurDefSet); @@ -568,449 +515,6 @@ void Compiler::fgPerBlockLocalVarLiveness() #endif // DEBUG } -// Helper functions to mark variables live over their entire scope - -void Compiler::fgBeginScopeLife(VARSET_TP* inScope, VarScopeDsc* var) -{ - assert(var); - - LclVarDsc* lclVarDsc1 = lvaGetDesc(var->vsdVarNum); - - if (lclVarDsc1->lvTracked) - { - VarSetOps::AddElemD(this, *inScope, lclVarDsc1->lvVarIndex); - } -} - -void Compiler::fgEndScopeLife(VARSET_TP* inScope, VarScopeDsc* var) -{ - assert(var); - - LclVarDsc* lclVarDsc1 = lvaGetDesc(var->vsdVarNum); - - if (lclVarDsc1->lvTracked) - { - VarSetOps::RemoveElemD(this, *inScope, lclVarDsc1->lvVarIndex); - } -} - -/*****************************************************************************/ - -void Compiler::fgMarkInScope(BasicBlock* block, VARSET_VALARG_TP inScope) -{ -#ifdef DEBUG - if (verbose) - { - printf("Scope info: block " FMT_BB " marking in scope: ", block->bbNum); - dumpConvertedVarSet(this, inScope); - printf("\n"); - } -#endif // DEBUG - - /* Record which vars are artificially kept alive for debugging */ - - VarSetOps::Assign(this, block->bbScope, inScope); - - /* Being in scope implies a use of the variable. Add the var to bbVarUse - so that redoing fgLiveVarAnalysis() will work correctly */ - - VarSetOps::UnionD(this, block->bbVarUse, inScope); - - /* Artificially mark all vars in scope as alive */ - - VarSetOps::UnionD(this, block->bbLiveIn, inScope); - VarSetOps::UnionD(this, block->bbLiveOut, inScope); -} - -void Compiler::fgUnmarkInScope(BasicBlock* block, VARSET_VALARG_TP unmarkScope) -{ -#ifdef DEBUG - if (verbose) - { - printf("Scope info: block " FMT_BB " UNmarking in scope: ", block->bbNum); - dumpConvertedVarSet(this, unmarkScope); - printf("\n"); - } -#endif // DEBUG - - assert(VarSetOps::IsSubset(this, unmarkScope, block->bbScope)); - - VarSetOps::DiffD(this, block->bbScope, unmarkScope); - VarSetOps::DiffD(this, block->bbVarUse, unmarkScope); - VarSetOps::DiffD(this, block->bbLiveIn, unmarkScope); - VarSetOps::DiffD(this, block->bbLiveOut, unmarkScope); -} - -#ifdef DEBUG - -void Compiler::fgDispDebugScopes() -{ - printf("\nDebug scopes:\n"); - - for (BasicBlock* const block : Blocks()) - { - printf(FMT_BB ": ", block->bbNum); - dumpConvertedVarSet(this, block->bbScope); - printf("\n"); - } -} - -#endif // DEBUG - -/***************************************************************************** - * - * Mark variables live across their entire scope. - */ - -void Compiler::fgExtendDbgScopes() -{ - compResetScopeLists(); - -#ifdef DEBUG - if (verbose) - { - printf("\nMarking vars alive over their entire scope :\n\n"); - compDispScopeLists(); - } -#endif // DEBUG - - VARSET_TP inScope(VarSetOps::MakeEmpty(this)); - - if (UsesFunclets()) - { - // Mark all tracked LocalVars live over their scope - walk the blocks - // keeping track of the current life, and assign it to the blocks. - - for (BasicBlock* const block : Blocks()) - { - // If we get to a funclet, reset the scope lists and start again, since the block - // offsets will be out of order compared to the previous block. - - if (block->HasFlag(BBF_FUNCLET_BEG)) - { - compResetScopeLists(); - VarSetOps::ClearD(this, inScope); - } - - // Process all scopes up to the current offset - - if (block->bbCodeOffs != BAD_IL_OFFSET) - { - compProcessScopesUntil(block->bbCodeOffs, &inScope, &Compiler::fgBeginScopeLife, - &Compiler::fgEndScopeLife); - } - - // Assign the current set of variables that are in scope to the block variables tracking this. - - fgMarkInScope(block, inScope); - } - -#ifdef DEBUG - if (verbose) - { - fgDispDebugScopes(); - } -#endif // DEBUG - } -#if defined(FEATURE_EH_WINDOWS_X86) - else - { - compProcessScopesUntil(0, &inScope, &Compiler::fgBeginScopeLife, &Compiler::fgEndScopeLife); - - IL_OFFSET lastEndOffs = 0; - - // Mark all tracked LocalVars live over their scope - walk the blocks - // keeping track of the current life, and assign it to the blocks. - - for (BasicBlock* const block : Blocks()) - { - // Find scopes becoming alive. If there is a gap in the instr - // sequence, we need to process any scopes on those missing offsets. - - if (block->bbCodeOffs != BAD_IL_OFFSET) - { - if (lastEndOffs != block->bbCodeOffs) - { - noway_assert(lastEndOffs < block->bbCodeOffs); - - compProcessScopesUntil(block->bbCodeOffs, &inScope, &Compiler::fgBeginScopeLife, - &Compiler::fgEndScopeLife); - } - else - { - while (VarScopeDsc* varScope = compGetNextEnterScope(block->bbCodeOffs)) - { - fgBeginScopeLife(&inScope, varScope); - } - } - } - - // Assign the current set of variables that are in scope to the block variables tracking this. - - fgMarkInScope(block, inScope); - - // Find scopes going dead. - - if (block->bbCodeOffsEnd != BAD_IL_OFFSET) - { - VarScopeDsc* varScope; - while ((varScope = compGetNextExitScope(block->bbCodeOffsEnd)) != nullptr) - { - fgEndScopeLife(&inScope, varScope); - } - - lastEndOffs = block->bbCodeOffsEnd; - } - } - - /* Everything should be out of scope by the end of the method. But if the - last BB got removed, then inScope may not be empty. */ - - noway_assert(VarSetOps::IsEmpty(this, inScope) || lastEndOffs < info.compILCodeSize); - } -#endif // FEATURE_EH_WINDOWS_X86 -} - -/***************************************************************************** - * - * For debuggable code, we allow redundant stores to vars - * by marking them live over their entire scope. - */ - -void Compiler::fgExtendDbgLifetimes() -{ -#ifdef DEBUG - if (verbose) - { - printf("*************** In fgExtendDbgLifetimes()\n"); - } -#endif // DEBUG - - noway_assert(opts.compDbgCode && (info.compVarScopesCount > 0)); - - /*------------------------------------------------------------------------- - * Extend the lifetimes over the entire reported scope of the variable. - */ - - fgExtendDbgScopes(); - - /*------------------------------------------------------------------------- - * Partly update liveness info so that we handle any funky BBF_INTERNAL - * blocks inserted out of sequence. - */ - -#ifdef DEBUG - if (verbose && 0) - { - fgDispBBLiveness(); - } -#endif - - fgLiveVarAnalysis(true); - - /* For compDbgCode, we prepend an empty BB which will hold the - initializations of variables which are in scope at IL offset 0 (but - not initialized by the IL code). Since they will currently be - marked as live on entry to fgFirstBB, unmark the liveness so that - the following code will know to add the initializations. */ - - assert(fgFirstBBisScratch()); - - VARSET_TP trackedArgs(VarSetOps::MakeEmpty(this)); - - for (unsigned argNum = 0; argNum < info.compArgsCount; argNum++) - { - LclVarDsc* argDsc = lvaGetDesc(argNum); - if (argDsc->lvPromoted) - { - lvaPromotionType promotionType = lvaGetPromotionType(argDsc); - - if (promotionType == PROMOTION_TYPE_INDEPENDENT) - { - noway_assert(argDsc->lvFieldCnt == 1); // We only handle one field here - - unsigned fieldVarNum = argDsc->lvFieldLclStart; - argDsc = lvaGetDesc(fieldVarNum); - } - } - noway_assert(argDsc->lvIsParam); - if (argDsc->lvTracked) - { - noway_assert(!VarSetOps::IsMember(this, trackedArgs, argDsc->lvVarIndex)); // Each arg should define a - // different bit. - VarSetOps::AddElemD(this, trackedArgs, argDsc->lvVarIndex); - } - } - - // Don't unmark struct locals, either. - VARSET_TP noUnmarkVars(trackedArgs); - - for (unsigned i = 0; i < lvaCount; i++) - { - LclVarDsc* varDsc = lvaGetDesc(i); - if (varTypeIsStruct(varDsc) && varDsc->lvTracked) - { - VarSetOps::AddElemD(this, noUnmarkVars, varDsc->lvVarIndex); - } - } - fgUnmarkInScope(fgFirstBB, VarSetOps::Diff(this, fgFirstBB->bbScope, noUnmarkVars)); - - /*------------------------------------------------------------------------- - * As we keep variables artificially alive over their entire scope, - * we need to also artificially initialize them if the scope does - * not exactly match the real lifetimes, or they will contain - * garbage until they are initialized by the IL code. - */ - - VARSET_TP initVars(VarSetOps::MakeEmpty(this)); // Vars which are artificially made alive - - for (BasicBlock* const block : Blocks()) - { - VarSetOps::ClearD(this, initVars); - - switch (block->GetKind()) - { - case BBJ_ALWAYS: - case BBJ_EHCATCHRET: - case BBJ_EHFILTERRET: - case BBJ_CALLFINALLY: - VarSetOps::UnionD(this, initVars, block->GetTarget()->bbScope); - break; - - case BBJ_CALLFINALLYRET: - VarSetOps::UnionD(this, initVars, block->bbScope); - VarSetOps::UnionD(this, initVars, block->GetFinallyContinuation()->bbScope); - break; - - case BBJ_COND: - PREFIX_ASSUME(!block->IsLast()); - VarSetOps::UnionD(this, initVars, block->GetFalseTarget()->bbScope); - VarSetOps::UnionD(this, initVars, block->GetTrueTarget()->bbScope); - break; - - case BBJ_SWITCH: - for (BasicBlock* const bTarget : block->SwitchTargets()) - { - VarSetOps::UnionD(this, initVars, bTarget->bbScope); - } - break; - - case BBJ_EHFINALLYRET: - case BBJ_EHFAULTRET: - case BBJ_RETURN: - break; - - case BBJ_THROW: - /* We don't have to do anything as we mark - * all vars live on entry to a catch handler as - * volatile anyway - */ - break; - - default: - noway_assert(!"Unexpected bbKind"); - break; - } - - /* If the var is already live on entry to the current BB, - we would have already initialized it. So ignore bbLiveIn */ - - VarSetOps::DiffD(this, initVars, block->bbLiveIn); - - /* Add statements initializing the vars, if there are any to initialize */ - - VarSetOps::Iter iter(this, initVars); - unsigned varIndex = 0; - while (iter.NextElem(&varIndex)) - { - /* Create initialization tree */ - - unsigned varNum = lvaTrackedIndexToLclNum(varIndex); - LclVarDsc* varDsc = lvaGetDesc(varNum); - var_types type = varDsc->TypeGet(); - - // Don't extend struct lifetimes -- they aren't enregistered, anyway. - if (type == TYP_STRUCT) - { - continue; - } - - // If we haven't already done this ... - if (!fgLocalVarLivenessDone) - { - // Create the initializer. - GenTree* zero = gtNewZeroConNode(type); - GenTree* initNode = gtNewStoreLclVarNode(varNum, zero); - - // Insert initialization node. - if (!block->IsLIR()) - { - // Create a statement for the initializer, sequence it, and append it to the current BB. - Statement* initStmt = gtNewStmt(initNode); - gtSetStmtInfo(initStmt); - fgSetStmtSeq(initStmt); - fgInsertStmtNearEnd(block, initStmt); - } - else - { - LIR::Range initRange = LIR::EmptyRange(); - initRange.InsertAfter(nullptr, zero, initNode); - -#if !defined(TARGET_64BIT) - DecomposeLongs::DecomposeRange(this, initRange); -#endif // !defined(TARGET_64BIT) - m_pLowering->LowerRange(block, initRange); - - // Naively inserting the initializer at the end of the block may add code after the block's - // terminator, in which case the inserted code will never be executed (and the IR for the - // block will be invalid). Use `LIR::InsertBeforeTerminator` to avoid this problem. - LIR::InsertBeforeTerminator(block, std::move(initRange)); - } - -#ifdef DEBUG - if (verbose) - { - printf("Created zero-init of V%02u in " FMT_BB "\n", varNum, block->bbNum); - } -#endif // DEBUG - } - - /* Update liveness information so that redoing fgLiveVarAnalysis() - will work correctly if needed */ - - VarSetOps::AddElemD(this, block->bbVarDef, varIndex); - VarSetOps::AddElemD(this, block->bbLiveOut, varIndex); - } - } - - // raMarkStkVars() reserves stack space for unused variables (which - // needs to be initialized). However, arguments don't need to be initialized. - // So just ensure that they don't have a 0 ref cnt - - unsigned lclNum = 0; - for (LclVarDsc* varDsc = lvaTable; lclNum < lvaCount; lclNum++, varDsc++) - { - if (lclNum >= info.compArgsCount) - { - break; // early exit for loop - } - - if (varDsc->lvIsRegArg) - { - varDsc->lvImplicitlyReferenced = true; - } - } - -#ifdef DEBUG - if (verbose) - { - printf("\nBB liveness after fgExtendDbgLifetimes():\n\n"); - fgDispBBLiveness(); - printf("\n"); - } -#endif // DEBUG -} - //------------------------------------------------------------------------ // fgAddHandlerLiveVars: determine set of locals live because of implicit // exception flow from a block. @@ -1036,8 +540,6 @@ class LiveVarAnalysis { Compiler* m_compiler; - bool m_hasPossibleBackEdge; - unsigned m_memoryLiveIn; unsigned m_memoryLiveOut; VARSET_TP m_liveIn; @@ -1046,7 +548,6 @@ class LiveVarAnalysis LiveVarAnalysis(Compiler* compiler) : m_compiler(compiler) - , m_hasPossibleBackEdge(false) , m_memoryLiveIn(emptyMemoryKindSet) , m_memoryLiveOut(emptyMemoryKindSet) , m_liveIn(VarSetOps::MakeEmpty(compiler)) @@ -1055,7 +556,7 @@ class LiveVarAnalysis { } - bool PerBlockAnalysis(BasicBlock* block, bool updateInternalOnly, bool keepAliveThis) + bool PerBlockAnalysis(BasicBlock* block, bool keepAliveThis) { /* Compute the 'liveOut' set */ VarSetOps::ClearD(m_compiler, m_liveOut); @@ -1090,10 +591,6 @@ class LiveVarAnalysis // state index variable may be live at this point without appearing // as an explicit use anywhere. VarSetOps::UnionD(m_compiler, m_liveOut, m_compiler->fgEntryBB->bbLiveIn); - if (m_compiler->fgEntryBB->bbNum <= block->bbNum) - { - m_hasPossibleBackEdge = true; - } } // Additionally, union in all the live-in tracked vars of regular @@ -1103,10 +600,6 @@ class LiveVarAnalysis block->VisitRegularSuccs(m_compiler, [=](BasicBlock* succ) { VarSetOps::UnionD(m_compiler, m_liveOut, succ->bbLiveIn); m_memoryLiveOut |= succ->bbMemoryLiveIn; - if (succ->bbNum <= block->bbNum) - { - m_hasPossibleBackEdge = true; - } return BasicBlockVisit::Continue; }); @@ -1131,10 +624,6 @@ class LiveVarAnalysis m_compiler->fgAddHandlerLiveVars(block, m_ehHandlerLiveVars, m_memoryLiveOut); VarSetOps::UnionD(m_compiler, m_liveIn, m_ehHandlerLiveVars); VarSetOps::UnionD(m_compiler, m_liveOut, m_ehHandlerLiveVars); - - // Implicit eh edges can induce loop-like behavior, - // so make sure we iterate to closure. - m_hasPossibleBackEdge = true; } // Even if block->bbMemoryDef is set, we must assume that it doesn't kill memory liveness from m_memoryLiveOut, @@ -1146,35 +635,8 @@ class LiveVarAnalysis bool liveInChanged = !VarSetOps::Equal(m_compiler, block->bbLiveIn, m_liveIn); if (liveInChanged || !VarSetOps::Equal(m_compiler, block->bbLiveOut, m_liveOut)) { - if (updateInternalOnly) - { - // Only "extend" liveness over BBF_INTERNAL blocks - - noway_assert(block->HasFlag(BBF_INTERNAL)); - - liveInChanged = !VarSetOps::IsSubset(m_compiler, m_liveIn, block->bbLiveIn); - if (liveInChanged || !VarSetOps::IsSubset(m_compiler, m_liveOut, block->bbLiveOut)) - { -#ifdef DEBUG - if (m_compiler->verbose) - { - printf("Scope info: block " FMT_BB " LiveIn+ ", block->bbNum); - dumpConvertedVarSet(m_compiler, VarSetOps::Diff(m_compiler, m_liveIn, block->bbLiveIn)); - printf(", LiveOut+ "); - dumpConvertedVarSet(m_compiler, VarSetOps::Diff(m_compiler, m_liveOut, block->bbLiveOut)); - printf("\n"); - } -#endif // DEBUG - - VarSetOps::UnionD(m_compiler, block->bbLiveIn, m_liveIn); - VarSetOps::UnionD(m_compiler, block->bbLiveOut, m_liveOut); - } - } - else - { - VarSetOps::Assign(m_compiler, block->bbLiveIn, m_liveIn); - VarSetOps::Assign(m_compiler, block->bbLiveOut, m_liveOut); - } + VarSetOps::Assign(m_compiler, block->bbLiveIn, m_liveIn); + VarSetOps::Assign(m_compiler, block->bbLiveOut, m_liveOut); } const bool memoryLiveInChanged = (block->bbMemoryLiveIn != m_memoryLiveIn); @@ -1187,11 +649,12 @@ class LiveVarAnalysis return liveInChanged || memoryLiveInChanged; } - void Run(bool updateInternalOnly) + void Run() { const bool keepAliveThis = m_compiler->lvaKeepAliveAndReportThis() && m_compiler->lvaTable[m_compiler->info.compThisArg].lvTracked; + const FlowGraphDfsTree* dfsTree = m_compiler->m_dfsTree; /* Live Variable Analysis - Backward dataflow */ bool changed; do @@ -1206,47 +669,22 @@ class LiveVarAnalysis m_memoryLiveIn = emptyMemoryKindSet; m_memoryLiveOut = emptyMemoryKindSet; - for (BasicBlock* block = m_compiler->fgLastBB; block; block = block->Prev()) + for (unsigned i = 0; i < dfsTree->GetPostOrderCount(); i++) { - // sometimes block numbers are not monotonically increasing which - // would cause us not to identify backedges - if (!block->IsLast() && block->Next()->bbNum <= block->bbNum) - { - m_hasPossibleBackEdge = true; - } - - if (updateInternalOnly) - { - /* Only update BBF_INTERNAL blocks as they may be - syntactically out of sequence. */ - - noway_assert(m_compiler->opts.compDbgCode && (m_compiler->info.compVarScopesCount > 0)); - - if (!block->HasFlag(BBF_INTERNAL)) - { - continue; - } - } - - if (PerBlockAnalysis(block, updateInternalOnly, keepAliveThis)) + BasicBlock* block = dfsTree->GetPostOrder(i); + if (PerBlockAnalysis(block, keepAliveThis)) { changed = true; } } - // if there is no way we could have processed a block without seeing all of its predecessors - // then there is no need to iterate - if (!m_hasPossibleBackEdge) - { - break; - } - } while (changed); + } while (changed && dfsTree->HasCycle()); } public: - static void Run(Compiler* compiler, bool updateInternalOnly) + static void Run(Compiler* compiler) { LiveVarAnalysis analysis(compiler); - analysis.Run(updateInternalOnly); + analysis.Run(); } }; @@ -1256,17 +694,12 @@ class LiveVarAnalysis * If updateInternalOnly==true, only update BBF_INTERNAL blocks. */ -void Compiler::fgLiveVarAnalysis(bool updateInternalOnly) +void Compiler::fgLiveVarAnalysis() { - if (!backendRequiresLocalVarLifetimes()) - { - return; - } - - LiveVarAnalysis::Run(this, updateInternalOnly); + LiveVarAnalysis::Run(this); #ifdef DEBUG - if (verbose && !updateInternalOnly) + if (verbose) { printf("\nBB liveness after fgLiveVarAnalysis():\n\n"); fgDispBBLiveness(); @@ -2315,22 +1748,6 @@ void Compiler::fgInterBlockLocalVarLiveness() fgLiveVarAnalysis(); - /* For debuggable code, we mark vars as live over their entire - * reported scope, so that it will be visible over the entire scope - */ - - if (opts.compDbgCode && (info.compVarScopesCount > 0)) - { - fgExtendDbgLifetimes(); - } - - // Nothing more to be done if the backend does not require accurate local var lifetimes. - if (!backendRequiresLocalVarLifetimes()) - { - fgLocalVarLivenessDone = true; - return; - } - //------------------------------------------------------------------------- // Variables involved in exception-handlers and finally blocks need // to be specially marked @@ -2339,8 +1756,9 @@ void Compiler::fgInterBlockLocalVarLiveness() VARSET_TP exceptVars(VarSetOps::MakeEmpty(this)); // vars live on entry to a handler VARSET_TP finallyVars(VarSetOps::MakeEmpty(this)); // vars live on exit of a 'finally' block - for (BasicBlock* const block : Blocks()) + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { + BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); if (block->hasEHBoundaryIn()) { // Note the set of variables live on entry to exception handler. @@ -2421,8 +1839,9 @@ void Compiler::fgInterBlockLocalVarLiveness() VARSET_TP volatileVars(VarSetOps::MakeEmpty(this)); - for (BasicBlock* const block : Blocks()) + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { + BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); /* Tell everyone what block we're working on */ compCurBB = block; @@ -2635,8 +2054,9 @@ void Compiler::fgDispBBLiveness(BasicBlock* block) void Compiler::fgDispBBLiveness() { - for (BasicBlock* const block : Blocks()) + for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { + BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); fgDispBBLiveness(block); } } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a966813f047542..28e0b9b79a0283 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7550,29 +7550,31 @@ PhaseStatus Lowering::DoPhase() const bool setSlotNumbers = false; comp->lvaComputeRefCounts(isRecompute, setSlotNumbers); - comp->fgLocalVarLiveness(); - // local var liveness can delete code, which may create empty blocks - if (comp->opts.OptimizationEnabled()) + // Remove dead blocks and compute DFS (we want to remove empty blocks even + // in MinOpts). + comp->fgDfsBlocksAndRemove(); + + if (comp->backendRequiresLocalVarLifetimes()) { - bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); - modified |= comp->fgRemoveDeadBlocks(); + assert(comp->opts.OptimizationEnabled()); + comp->fgLocalVarLiveness(); + // local var liveness can delete code, which may create empty blocks + bool modified = comp->fgUpdateFlowGraph(/* doTailDuplication */ false, /* isPhase */ false); if (modified) { + comp->fgDfsBlocksAndRemove(); JITDUMP("had to run another liveness pass:\n"); comp->fgLocalVarLiveness(); } - } - else - { - // If we are not optimizing, remove the dead blocks regardless. - comp->fgRemoveDeadBlocks(); + + // Recompute local var ref counts again after liveness to reflect + // impact of any dead code removal. Note this may leave us with + // tracked vars that have zero refs. + comp->lvaComputeRefCounts(isRecompute, setSlotNumbers); } - // Recompute local var ref counts again after liveness to reflect - // impact of any dead code removal. Note this may leave us with - // tracked vars that have zero refs. - comp->lvaComputeRefCounts(isRecompute, setSlotNumbers); + comp->fgInvalidateDfsTree(); return PhaseStatus::MODIFIED_EVERYTHING; } From 802560a121d196b1d0020441f3c39efdbf31abe1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jun 2024 13:16:28 +0200 Subject: [PATCH 2/7] Fix comment --- src/coreclr/jit/lower.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 28e0b9b79a0283..10e404971e1fb7 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7550,8 +7550,8 @@ PhaseStatus Lowering::DoPhase() const bool setSlotNumbers = false; comp->lvaComputeRefCounts(isRecompute, setSlotNumbers); - // Remove dead blocks and compute DFS (we want to remove empty blocks even - // in MinOpts). + // Remove dead blocks and compute DFS (we want to remove unreachable blocks + // even in MinOpts). comp->fgDfsBlocksAndRemove(); if (comp->backendRequiresLocalVarLifetimes()) From f9143b4a00ac240e363a19f4320ccc70e07d2725 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jun 2024 14:16:29 +0200 Subject: [PATCH 3/7] Remove BasicBlock::bbScope, handle throw helper blocks separately --- src/coreclr/jit/block.cpp | 4 ---- src/coreclr/jit/block.h | 2 -- src/coreclr/jit/compiler.h | 4 ++-- src/coreclr/jit/liveness.cpp | 43 ++++++++++++++++++++++-------------- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index ecee292264ec85..be2ba15e254690 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -909,7 +909,6 @@ void BasicBlock::CloneBlockState(Compiler* compiler, BasicBlock* to, const Basic to->bbStkDepth = from->bbStkDepth; to->bbCodeOffs = from->bbCodeOffs; to->bbCodeOffsEnd = from->bbCodeOffsEnd; - VarSetOps::AssignAllowUninitRhs(compiler, to->bbScope, from->bbScope); #ifdef DEBUG to->bbTgtStkDepth = from->bbTgtStkDepth; #endif // DEBUG @@ -1470,7 +1469,6 @@ void BasicBlock::InitVarSets(Compiler* comp) VarSetOps::AssignNoCopy(comp, bbVarDef, VarSetOps::MakeEmpty(comp)); VarSetOps::AssignNoCopy(comp, bbLiveIn, VarSetOps::MakeEmpty(comp)); VarSetOps::AssignNoCopy(comp, bbLiveOut, VarSetOps::MakeEmpty(comp)); - VarSetOps::AssignNoCopy(comp, bbScope, VarSetOps::MakeEmpty(comp)); bbMemoryUse = emptyMemoryKindSet; bbMemoryDef = emptyMemoryKindSet; @@ -1672,7 +1670,6 @@ BasicBlock* BasicBlock::New(Compiler* compiler) VarSetOps::AssignNoCopy(compiler, block->bbVarDef, VarSetOps::MakeEmpty(compiler)); VarSetOps::AssignNoCopy(compiler, block->bbLiveIn, VarSetOps::MakeEmpty(compiler)); VarSetOps::AssignNoCopy(compiler, block->bbLiveOut, VarSetOps::MakeEmpty(compiler)); - VarSetOps::AssignNoCopy(compiler, block->bbScope, VarSetOps::MakeEmpty(compiler)); } else { @@ -1680,7 +1677,6 @@ BasicBlock* BasicBlock::New(Compiler* compiler) VarSetOps::AssignNoCopy(compiler, block->bbVarDef, VarSetOps::UninitVal()); VarSetOps::AssignNoCopy(compiler, block->bbLiveIn, VarSetOps::UninitVal()); VarSetOps::AssignNoCopy(compiler, block->bbLiveOut, VarSetOps::UninitVal()); - VarSetOps::AssignNoCopy(compiler, block->bbScope, VarSetOps::UninitVal()); } block->bbMemoryUse = emptyMemoryKindSet; diff --git a/src/coreclr/jit/block.h b/src/coreclr/jit/block.h index 054edae23113b5..903e574b676148 100644 --- a/src/coreclr/jit/block.h +++ b/src/coreclr/jit/block.h @@ -1611,8 +1611,6 @@ struct BasicBlock : private LIR::Range unsigned bbMemorySsaNumIn[MemoryKindCount]; // The SSA # of memory on entry to the block. unsigned bbMemorySsaNumOut[MemoryKindCount]; // The SSA # of memory on exit from the block. - VARSET_TP bbScope; // variables in scope over the block - void InitVarSets(class Compiler* comp); /* The following are the standard bit sets for dataflow analysis. diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f0e74eb12de176..59ecd4e5a1dd7b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5545,10 +5545,10 @@ class Compiler void fgComputeLife(VARSET_TP& life, GenTree* startNode, GenTree* endNode, - VARSET_VALARG_TP volatileVars, + VARSET_VALARG_TP keepAliveVars, bool* pStmtInfoDirty DEBUGARG(bool* treeModf)); - void fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP volatileVars); + void fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP keepAliveVars); bool fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange); diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 82df6fcf3d1f8e..39d019d878898e 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -678,6 +678,22 @@ class LiveVarAnalysis } } } while (changed && dfsTree->HasCycle()); + + // If we had unremovable blocks that are not in the DFS tree then make + // 'this' alive in them if we need to keep it alive. + // This would normally not be necessary assuming those blocks are + // actually unreachable; however, throw helpers fall into this category + // because we do not model them correctly, and those will actually end + // up reachable. Fix that up here. + if (keepAliveThis && (dfsTree->GetPostOrderCount() != m_compiler->fgBBcount)) + { + unsigned thisIndex = m_compiler->lvaGetDesc(m_compiler->info.compThisArg)->lvVarIndex; + for (BasicBlock* block : m_compiler->Blocks()) + { + VarSetOps::AddElemD(m_compiler, block->bbLiveIn, thisIndex); + VarSetOps::AddElemD(m_compiler, block->bbLiveOut, thisIndex); + } + } } public: @@ -1095,12 +1111,10 @@ GenTree* Compiler::fgTryRemoveDeadStoreEarly(Statement* stmt, GenTreeLclVarCommo void Compiler::fgComputeLife(VARSET_TP& life, GenTree* startNode, GenTree* endNode, - VARSET_VALARG_TP volatileVars, + VARSET_VALARG_TP keepAliveVars, bool* pStmtInfoDirty DEBUGARG(bool* treeModf)) { // Don't kill vars in scope - VARSET_TP keepAliveVars(VarSetOps::Union(this, volatileVars, compCurBB->bbScope)); - noway_assert(VarSetOps::IsSubset(this, keepAliveVars, life)); noway_assert(endNode || (startNode == compCurStmt->GetRootNode())); assert(!fgIsDoingEarlyLiveness); @@ -1161,11 +1175,8 @@ void Compiler::fgComputeLife(VARSET_TP& life, } } -void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP volatileVars) +void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP keepAliveVars) { - // Don't kill volatile vars and vars in scope. - VARSET_TP keepAliveVars(VarSetOps::Union(this, volatileVars, block->bbScope)); - noway_assert(VarSetOps::IsSubset(this, keepAliveVars, life)); LIR::Range& blockRange = LIR::AsRange(block); @@ -1837,7 +1848,7 @@ void Compiler::fgInterBlockLocalVarLiveness() * Now fill in liveness info within each basic block - Backward DataFlow */ - VARSET_TP volatileVars(VarSetOps::MakeEmpty(this)); + VARSET_TP keepAliveVars(VarSetOps::MakeEmpty(this)); for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) { @@ -1848,15 +1859,15 @@ void Compiler::fgInterBlockLocalVarLiveness() /* Remember those vars live on entry to exception handlers */ /* if we are part of a try block */ - VarSetOps::ClearD(this, volatileVars); + VarSetOps::ClearD(this, keepAliveVars); if (block->HasPotentialEHSuccs(this)) { MemoryKindSet memoryLiveness = 0; - fgAddHandlerLiveVars(block, volatileVars, memoryLiveness); + fgAddHandlerLiveVars(block, keepAliveVars, memoryLiveness); - // volatileVars is a subset of exceptVars - noway_assert(VarSetOps::IsSubset(this, volatileVars, exceptVars)); + // keepAliveVars is a subset of exceptVars + noway_assert(VarSetOps::IsSubset(this, keepAliveVars, exceptVars)); } /* Start with the variables live on exit from the block */ @@ -1867,7 +1878,7 @@ void Compiler::fgInterBlockLocalVarLiveness() if (block->IsLIR()) { - fgComputeLifeLIR(life, block, volatileVars); + fgComputeLifeLIR(life, block, keepAliveVars); } else if (fgNodeThreading == NodeThreading::AllTrees) { @@ -1897,7 +1908,7 @@ void Compiler::fgInterBlockLocalVarLiveness() /* Compute the liveness for each tree node in the statement */ bool stmtInfoDirty = false; - fgComputeLife(life, compCurStmt->GetRootNode(), nullptr, volatileVars, + fgComputeLife(life, compCurStmt->GetRootNode(), nullptr, keepAliveVars, &stmtInfoDirty DEBUGARG(&treeModf)); if (stmtInfoDirty) @@ -1921,7 +1932,6 @@ void Compiler::fgInterBlockLocalVarLiveness() { assert(fgIsDoingEarlyLiveness && (fgNodeThreading == NodeThreading::AllLocals)); compCurStmt = nullptr; - VARSET_TP keepAliveVars(VarSetOps::Union(this, volatileVars, compCurBB->bbScope)); Statement* firstStmt = block->firstStmt(); @@ -2054,9 +2064,8 @@ void Compiler::fgDispBBLiveness(BasicBlock* block) void Compiler::fgDispBBLiveness() { - for (unsigned i = m_dfsTree->GetPostOrderCount(); i != 0; i--) + for (BasicBlock* const block : Blocks()) { - BasicBlock* block = m_dfsTree->GetPostOrder(i - 1); fgDispBBLiveness(block); } } From 81c9db284b41d4c407b9779b48a6dcf83ad15fd5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jun 2024 15:01:56 +0200 Subject: [PATCH 4/7] Fix keep alive set --- src/coreclr/jit/liveness.cpp | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 39d019d878898e..7864095e202c6b 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -680,18 +680,36 @@ class LiveVarAnalysis } while (changed && dfsTree->HasCycle()); // If we had unremovable blocks that are not in the DFS tree then make - // 'this' alive in them if we need to keep it alive. - // This would normally not be necessary assuming those blocks are - // actually unreachable; however, throw helpers fall into this category - // because we do not model them correctly, and those will actually end - // up reachable. Fix that up here. - if (keepAliveThis && (dfsTree->GetPostOrderCount() != m_compiler->fgBBcount)) + // the 'keepAlive' set live in them. This would normally not be + // necessary assuming those blocks are actually unreachable; however, + // throw helpers fall into this category because we do not model them + // correctly, and those will actually end up reachable. Fix that up + // here. + if (m_compiler->fgBBcount != dfsTree->GetPostOrderCount()) { - unsigned thisIndex = m_compiler->lvaGetDesc(m_compiler->info.compThisArg)->lvVarIndex; for (BasicBlock* block : m_compiler->Blocks()) { - VarSetOps::AddElemD(m_compiler, block->bbLiveIn, thisIndex); - VarSetOps::AddElemD(m_compiler, block->bbLiveOut, thisIndex); + if (dfsTree->Contains(block)) + { + continue; + } + + VarSetOps::ClearD(m_compiler, block->bbLiveOut); + if (keepAliveThis) + { + unsigned thisVarIndex = m_compiler->lvaGetDesc(m_compiler->info.compThisArg)->lvVarIndex; + VarSetOps::AddElemD(m_compiler, block->bbLiveOut, thisVarIndex); + } + + if (block->HasPotentialEHSuccs(m_compiler)) + { + block->VisitEHSuccs(m_compiler, [=](BasicBlock* succ) { + VarSetOps::UnionD(m_compiler, block->bbLiveOut, succ->bbLiveIn); + return BasicBlockVisit::Continue; + }); + } + + VarSetOps::Assign(m_compiler, block->bbLiveIn, block->bbLiveOut); } } } From fab44bdbe97b2514d6e00aaae298a184d1493d33 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jun 2024 21:56:13 +0200 Subject: [PATCH 5/7] Do dump in lexical order --- src/coreclr/jit/liveness.cpp | 50 +++++++++++++++++------------------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/liveness.cpp b/src/coreclr/jit/liveness.cpp index 7864095e202c6b..e014cb3919c77a 100644 --- a/src/coreclr/jit/liveness.cpp +++ b/src/coreclr/jit/liveness.cpp @@ -462,53 +462,51 @@ void Compiler::fgPerBlockLocalVarLiveness() } } + VarSetOps::Assign(this, block->bbVarUse, fgCurUseSet); + VarSetOps::Assign(this, block->bbVarDef, fgCurDefSet); + block->bbMemoryUse = fgCurMemoryUse; + block->bbMemoryDef = fgCurMemoryDef; + block->bbMemoryHavoc = fgCurMemoryHavoc; + + /* also initialize the IN set, just in case we will do multiple DFAs */ + + VarSetOps::AssignNoCopy(this, block->bbLiveIn, VarSetOps::MakeEmpty(this)); + block->bbMemoryLiveIn = emptyMemoryKindSet; + } + + noway_assert(livenessVarEpoch == GetCurLVEpoch()); #ifdef DEBUG - if (verbose) + if (verbose) + { + for (BasicBlock* block : Blocks()) { - VARSET_TP allVars(VarSetOps::Union(this, fgCurUseSet, fgCurDefSet)); + VARSET_TP allVars(VarSetOps::Union(this, block->bbVarUse, block->bbVarDef)); printf(FMT_BB, block->bbNum); - printf(" USE(%d)=", VarSetOps::Count(this, fgCurUseSet)); - lvaDispVarSet(fgCurUseSet, allVars); + printf(" USE(%d)=", VarSetOps::Count(this, block->bbVarUse)); + lvaDispVarSet(block->bbVarUse, allVars); for (MemoryKind memoryKind : allMemoryKinds()) { - if ((fgCurMemoryUse & memoryKindSet(memoryKind)) != 0) + if ((block->bbMemoryUse & memoryKindSet(memoryKind)) != 0) { printf(" + %s", memoryKindNames[memoryKind]); } } - printf("\n DEF(%d)=", VarSetOps::Count(this, fgCurDefSet)); - lvaDispVarSet(fgCurDefSet, allVars); + printf("\n DEF(%d)=", VarSetOps::Count(this, block->bbVarDef)); + lvaDispVarSet(block->bbVarDef, allVars); for (MemoryKind memoryKind : allMemoryKinds()) { - if ((fgCurMemoryDef & memoryKindSet(memoryKind)) != 0) + if ((block->bbMemoryDef & memoryKindSet(memoryKind)) != 0) { printf(" + %s", memoryKindNames[memoryKind]); } - if ((fgCurMemoryHavoc & memoryKindSet(memoryKind)) != 0) + if ((block->bbMemoryHavoc & memoryKindSet(memoryKind)) != 0) { printf("*"); } } printf("\n\n"); } -#endif // DEBUG - VarSetOps::Assign(this, block->bbVarUse, fgCurUseSet); - VarSetOps::Assign(this, block->bbVarDef, fgCurDefSet); - block->bbMemoryUse = fgCurMemoryUse; - block->bbMemoryDef = fgCurMemoryDef; - block->bbMemoryHavoc = fgCurMemoryHavoc; - - /* also initialize the IN set, just in case we will do multiple DFAs */ - - VarSetOps::AssignNoCopy(this, block->bbLiveIn, VarSetOps::MakeEmpty(this)); - block->bbMemoryLiveIn = emptyMemoryKindSet; - } - - noway_assert(livenessVarEpoch == GetCurLVEpoch()); -#ifdef DEBUG - if (verbose) - { printf("** Memory liveness computed, GcHeap states and ByrefExposed states %s\n", (byrefStatesMatchGcHeapStates ? "match" : "diverge")); } From 90bc6348bd894ef27bada06069474c5c8c7bc68d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 24 Jun 2024 17:39:22 +0200 Subject: [PATCH 6/7] Remove more stuff --- src/coreclr/jit/compiler.cpp | 4 ---- src/coreclr/jit/compiler.h | 2 -- src/coreclr/jit/gcencode.cpp | 4 +--- src/coreclr/jit/jitconfigvalues.h | 8 -------- src/coreclr/jit/lclvars.cpp | 9 --------- 5 files changed, 1 insertion(+), 26 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 9b035278d66d53..299d65f4f33cda 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -10923,9 +10923,6 @@ void Compiler::EnregisterStats::RecordLocal(const LclVarDsc* varDsc) case DoNotEnregisterReason::NoRegVars: m_noRegVars++; break; - case DoNotEnregisterReason::MinOptsGC: - m_minOptsGC++; - break; #if !defined(TARGET_64BIT) case DoNotEnregisterReason::LongParamField: m_longParamField++; @@ -11080,7 +11077,6 @@ void Compiler::EnregisterStats::Dump(FILE* fout) const PRINT_STATS(m_structArg, notEnreg); PRINT_STATS(m_depField, notEnreg); PRINT_STATS(m_noRegVars, notEnreg); - PRINT_STATS(m_minOptsGC, notEnreg); #if !defined(TARGET_64BIT) PRINT_STATS(m_longParamField, notEnreg); #endif // !TARGET_64BIT diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index eb1a3fd33b6587..758ccf9952f5d2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -452,7 +452,6 @@ enum class DoNotEnregisterReason IsStructArg, // Is a struct passed as an argument in a way that requires a stack location. DepField, // It is a field of a dependently promoted struct NoRegVars, // opts.compFlags & CLFLG_REGVAR is not set - MinOptsGC, // It is a GC Ref and we are compiling MinOpts #if !defined(TARGET_64BIT) LongParamField, // It is a decomposed field of a long parameter. #endif @@ -10872,7 +10871,6 @@ class Compiler unsigned m_liveInOutHndlr; unsigned m_depField; unsigned m_noRegVars; - unsigned m_minOptsGC; #ifdef JIT32_GCENCODER unsigned m_PinningRef; #endif // JIT32_GCENCODER diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index a8194fb26c532e..ff5157806086e3 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4053,9 +4053,7 @@ void GCInfo::gcMakeRegPtrTable( { GCENCODER_WITH_LOGGING(gcInfoEncoderWithLog, gcInfoEncoder); - const bool noTrackedGCSlots = - (compiler->opts.MinOpts() && !compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT) && - !JitConfig.JitMinOptsTrackGCrefs()); + const bool noTrackedGCSlots = compiler->opts.MinOpts(); if (mode == MAKE_REG_PTR_MODE_ASSIGN_SLOTS) { diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 97322e200a3fe7..771c86dbcbb744 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -521,14 +521,6 @@ RELEASE_CONFIG_INTEGER(JitEnableNoWayAssert, W("JitEnableNoWayAssert"), 0) RELEASE_CONFIG_INTEGER(JitEnableNoWayAssert, W("JitEnableNoWayAssert"), 1) #endif // !defined(DEBUG) && !defined(_DEBUG) -// Track GC roots -#if defined(TARGET_AMD64) || defined(TARGET_X86) -#define JitMinOptsTrackGCrefs_Default 0 // Not tracking GC refs in MinOpts is new behavior -#else -#define JitMinOptsTrackGCrefs_Default 1 -#endif -RELEASE_CONFIG_INTEGER(JitMinOptsTrackGCrefs, W("JitMinOptsTrackGCrefs"), JitMinOptsTrackGCrefs_Default) - // The following should be wrapped inside "#if MEASURE_MEM_ALLOC / #endif", but // some files include this one without bringing in the definitions from "jit.h" // so we don't always know what the "true" value of that flag should be. For now diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 84d4de9046e6b7..b74d259c632b31 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -3212,10 +3212,6 @@ void Compiler::lvaSetVarDoNotEnregister(unsigned varNum DEBUGARG(DoNotEnregister JITDUMP("opts.compFlags & CLFLG_REGVAR is not set\n"); assert(!compEnregLocals()); break; - case DoNotEnregisterReason::MinOptsGC: - JITDUMP("it is a GC Ref and we are compiling MinOpts\n"); - assert(!JitConfig.JitMinOptsTrackGCrefs() && varTypeIsGC(varDsc->TypeGet())); - break; #if !defined(TARGET_64BIT) case DoNotEnregisterReason::LongParamField: JITDUMP("it is a decomposed field of a long parameter\n"); @@ -4147,11 +4143,6 @@ void Compiler::lvaSortByRefCount() lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::PinningRef)); #endif } - if (opts.MinOpts() && !JitConfig.JitMinOptsTrackGCrefs() && varTypeIsGC(varDsc->TypeGet())) - { - varDsc->lvTracked = 0; - lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::MinOptsGC)); - } if (!compEnregLocals()) { lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::NoRegVars)); From 9e1a6c39b304acad78d506d3adf9352e639cebc9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 24 Jun 2024 23:50:33 +0200 Subject: [PATCH 7/7] Revert change in where GC info optimization is enabled --- src/coreclr/jit/gcencode.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index ff5157806086e3..9125d1484fe5de 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4053,7 +4053,13 @@ void GCInfo::gcMakeRegPtrTable( { GCENCODER_WITH_LOGGING(gcInfoEncoderWithLog, gcInfoEncoder); - const bool noTrackedGCSlots = compiler->opts.MinOpts(); + // TODO: Decide on whether we should enable this optimization for all + // targets: https://github.com/dotnet/runtime/issues/103917 +#ifdef TARGET_XARCH + const bool noTrackedGCSlots = compiler->opts.MinOpts() && !compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_PREJIT); +#else + const bool noTrackedGCSlots = false; +#endif if (mode == MAKE_REG_PTR_MODE_ASSIGN_SLOTS) {