From c29fc7b6ed42323781587772d37bfd9b89870b60 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 26 May 2023 14:03:52 -0700 Subject: [PATCH 1/3] JIT: make global morph its own phase, morph blocks in RPO Some refactoring in antcipation of enabling cross-block local assertion prop during global morph. Process blocks in RPO. Try and verify that no newly added blocks can alter the set of assertions that flow into the pre-existing ones. --- src/coreclr/jit/compiler.cpp | 22 ++--- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/compphases.h | 1 + src/coreclr/jit/morph.cpp | 167 +++++++++++++++++++++-------------- 4 files changed, 115 insertions(+), 79 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index c0bfbb7a37203f..65713ef5eec945 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4765,9 +4765,10 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // Morph the trees in all the blocks of the method // - auto morphGlobalPhase = [this]() { - unsigned prevBBCount = fgBBcount; - fgMorphBlocks(); + unsigned const preMorphBBCount = fgBBcount; + DoPhase(this, PHASE_MORPH_GLOBAL, &Compiler::fgMorphBlocks); + + auto postMorphPhase = [this]() { // Fix any LclVar annotations on discarded struct promotion temps for implicit by-ref args fgMarkDemotedImplicitByRefArgs(); @@ -4783,16 +4784,17 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl compCurBB = nullptr; #endif // DEBUG - // If we needed to create any new BasicBlocks then renumber the blocks - if (fgBBcount > prevBBCount) - { - fgRenumberBlocks(); - } - // Enable IR checks activePhaseChecks |= PhaseChecks::CHECK_IR; }; - DoPhase(this, PHASE_MORPH_GLOBAL, morphGlobalPhase); + DoPhase(this, PHASE_POST_MORPH, postMorphPhase); + + // If we needed to create any new BasicBlocks then renumber the blocks + // + if (fgBBcount > preMorphBBCount) + { + fgRenumberBlocks(); + } // GS security checks for unsafe buffers // diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 8f09c55cb3da7e..a3a5617cdb7fd2 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4751,9 +4751,9 @@ class Compiler FoldResult fgFoldConditional(BasicBlock* block); + PhaseStatus fgMorphBlocks(); + void fgMorphBlock(BasicBlock* block); void fgMorphStmts(BasicBlock* block); - void fgMorphBlocks(); - void fgMergeBlockReturn(BasicBlock* block); bool fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(const char* msg)); diff --git a/src/coreclr/jit/compphases.h b/src/coreclr/jit/compphases.h index 460b47f9bca855..4f9c39e2efa3ad 100644 --- a/src/coreclr/jit/compphases.h +++ b/src/coreclr/jit/compphases.h @@ -48,6 +48,7 @@ CompPhaseNameMacro(PHASE_FWD_SUB, "Forward Substitution", CompPhaseNameMacro(PHASE_MORPH_IMPBYREF, "Morph - ByRefs", false, -1, false) CompPhaseNameMacro(PHASE_PROMOTE_STRUCTS, "Morph - Promote Structs", false, -1, false) CompPhaseNameMacro(PHASE_MORPH_GLOBAL, "Morph - Global", false, -1, false) +CompPhaseNameMacro(PHASE_POST_MORPH, "Post-Morph", false, -1, false) CompPhaseNameMacro(PHASE_MORPH_END, "Morph - Finish", false, -1, true) CompPhaseNameMacro(PHASE_GS_COOKIE, "GS Cookie", false, -1, false) CompPhaseNameMacro(PHASE_COMPUTE_EDGE_WEIGHTS, "Compute edge weights (1, false)",false, -1, false) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 8e4c6612b41a10..9255df466662fc 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13372,13 +13372,12 @@ void Compiler::fgMorphStmtBlockOps(BasicBlock* block, Statement* stmt) } } -/***************************************************************************** - * - * Morph the statements of the given block. - * This function should be called just once for a block. Use fgMorphBlockStmt() - * for reentrant calls. - */ - +//------------------------------------------------------------------------ +// fgMorphStmts: Morph all statements in a block +// +// Arguments: +// block - block in question +// void Compiler::fgMorphStmts(BasicBlock* block) { fgRemoveRestOfBlock = false; @@ -13569,36 +13568,65 @@ void Compiler::fgMorphStmts(BasicBlock* block) fgRemoveRestOfBlock = false; } -/***************************************************************************** - * - * Morph the blocks of the method. - * Returns true if the basic block list is modified. - * This function should be called just once. - */ - -void Compiler::fgMorphBlocks() +//------------------------------------------------------------------------ +// fgMorphBlock: Morph a basic block +// +// Arguments: +// block - block in question +// +void Compiler::fgMorphBlock(BasicBlock* block) { -#ifdef DEBUG - if (verbose) + JITDUMP("\nMorphing " FMT_BB "\n", block->bbNum); + + if (optLocalAssertionProp) { - printf("\n*************** In fgMorphBlocks()\n"); + // Clear out any currently recorded assertion candidates + // before processing each basic block, + // also we must handle QMARK-COLON specially + // + optAssertionReset(0); } -#endif - /* Since fgMorphTree can be called after various optimizations to re-arrange - * the nodes we need a global flag to signal if we are during the one-pass - * global morphing */ + // Make the current basic block address available globally. + compCurBB = block; - fgGlobalMorph = true; + // Process all statement trees in the basic block. + fgMorphStmts(block); + + // Do we need to merge the result of this block into a single return block? + if ((block->bbJumpKind == BBJ_RETURN) && ((block->bbFlags & BBF_HAS_JMP) == 0)) + { + if ((genReturnBB != nullptr) && (genReturnBB != block)) + { + fgMergeBlockReturn(block); + } + } + compCurBB = nullptr; +} + +//------------------------------------------------------------------------ +// fgMorphBlocks: Morph all blocks in the method +// +// Returns: +// Suitable phase status. +// +// Note: +// Morph almost always changes IR, so we don't actually bother to +// track if it made any changees. +// +PhaseStatus Compiler::fgMorphBlocks() +{ + // This is the one and only global morph phase // + fgGlobalMorph = true; + // Local assertion prop is enabled if we are optimized // optLocalAssertionProp = opts.OptimizationEnabled(); if (optLocalAssertionProp) { - // // Initialize for local assertion prop // optAssertionInit(true); @@ -13614,53 +13642,59 @@ void Compiler::fgMorphBlocks() lvSetMinOptsDoNotEnreg(); } - /*------------------------------------------------------------------------- - * Process all basic blocks in the function - */ + // Build a reverse postorder + // + fgRenumberBlocks(); + EnsureBasicBlockEpoch(); + fgComputeEnterBlocksSet(); + fgDfsReversePostorder(); - BasicBlock* block = fgFirstBB; - noway_assert(block); + // Morph may introduce new blocks, so remember + // how many there were to start with + // + unsigned const bbNumMax = fgBBNumMax; - do + for (unsigned i = 1; i <= bbNumMax; i++) { -#ifdef DEBUG - if (verbose) - { - printf("\nMorphing " FMT_BB " of '%s'\n", block->bbNum, info.compFullName); - } -#endif + BasicBlock* const block = fgBBReversePostorder[i]; + fgMorphBlock(block); + } - if (optLocalAssertionProp) + // If morph created new blocks, then morph them as well. + // (TODO: track this set more efficiently) + // + if (fgBBNumMax != bbNumMax) + { + for (BasicBlock* block : Blocks()) { - // - // Clear out any currently recorded assertion candidates - // before processing each basic block, - // also we must handle QMARK-COLON specially - // - optAssertionReset(0); - } + if (block->bbNum > bbNumMax) + { - // Make the current basic block address available globally. - compCurBB = block; +#ifdef DEBUG + // Sanity checks to hopefully ensure the newly added blocks + // don't invalidate any forward assertion propagtion we did for the + // original blocks. + // + if (block != fgFirstBB) + { + const unsigned numSucc = block->NumSucc(); + assert(numSucc <= 1); - // Process all statement trees in the basic block. - fgMorphStmts(block); + if (numSucc == 1) + { + BasicBlock* const viableTarget = opts.IsOSR() ? fgEntryBB : fgFirstBB; + assert(block->isEmpty() || (block->GetUniqueSucc() == viableTarget)); + } + } +#endif - // Do we need to merge the result of this block into a single return block? - if ((block->bbJumpKind == BBJ_RETURN) && ((block->bbFlags & BBF_HAS_JMP) == 0)) - { - if ((genReturnBB != nullptr) && (genReturnBB != block)) - { - fgMergeBlockReturn(block); + // TODO: we might need to pass a flag here indicating that this block + // cannot safely use the pred block assertion state. + // + fgMorphBlock(block); } } - - block = block->bbNext; - } while (block != nullptr); - - // We are done with the global morphing phase - fgGlobalMorph = false; - compCurBB = nullptr; + } // Under OSR, we no longer need to specially protect the original method entry // @@ -13676,12 +13710,11 @@ void Compiler::fgMorphBlocks() fgEntryBB = nullptr; } -#ifdef DEBUG - if (verboseTrees) - { - fgDispBasicBlocks(true); - } -#endif + // We are done with the global morphing phase + // + fgGlobalMorph = false; + + return PhaseStatus::MODIFIED_EVERYTHING; } //------------------------------------------------------------------------ From 9ba3db0e3cbcf6b5ac17f89a550569fd56d15691 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 27 May 2023 08:40:55 -0700 Subject: [PATCH 2/3] only do RPO when optimizing to mitigate TP impact --- src/coreclr/jit/morph.cpp | 73 ++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 9255df466662fc..5803a34fa54954 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13642,52 +13642,47 @@ PhaseStatus Compiler::fgMorphBlocks() lvSetMinOptsDoNotEnreg(); } - // Build a reverse postorder + // Morph all blocks. If we aren't then we just morph in normal bbNext order. // - fgRenumberBlocks(); - EnsureBasicBlockEpoch(); - fgComputeEnterBlocksSet(); - fgDfsReversePostorder(); - - // Morph may introduce new blocks, so remember - // how many there were to start with - // - unsigned const bbNumMax = fgBBNumMax; - - for (unsigned i = 1; i <= bbNumMax; i++) - { - BasicBlock* const block = fgBBReversePostorder[i]; - fgMorphBlock(block); - } - - // If morph created new blocks, then morph them as well. - // (TODO: track this set more efficiently) - // - if (fgBBNumMax != bbNumMax) + if (!optLocalAssertionProp) { + // Note morph can add blocks downstream from the current block, + // and alter (but not null out) the current block's bbNext; + // this iterator ensures they all get visited. + // for (BasicBlock* block : Blocks()) { - if (block->bbNum > bbNumMax) - { + fgMorphBlock(block); + } + } + else + { + // We are optimizing. Process in RPO. + // + fgRenumberBlocks(); + EnsureBasicBlockEpoch(); + fgComputeEnterBlocksSet(); + fgDfsReversePostorder(); -#ifdef DEBUG - // Sanity checks to hopefully ensure the newly added blocks - // don't invalidate any forward assertion propagtion we did for the - // original blocks. - // - if (block != fgFirstBB) - { - const unsigned numSucc = block->NumSucc(); - assert(numSucc <= 1); + // Morph can introduce new blocks, so remember + // how many there were to start with + // + unsigned const bbNumMax = fgBBNumMax; - if (numSucc == 1) - { - BasicBlock* const viableTarget = opts.IsOSR() ? fgEntryBB : fgFirstBB; - assert(block->isEmpty() || (block->GetUniqueSucc() == viableTarget)); - } - } -#endif + for (unsigned i = 1; i <= bbNumMax; i++) + { + BasicBlock* const block = fgBBReversePostorder[i]; + fgMorphBlock(block); + } + // If morph created new blocks, then morph them as well. + // (TODO: track this set more efficiently) + // (TODO: verify that this potential out of order processing does not cause issues) + // + if (fgBBNumMax != bbNumMax) + { + for (BasicBlock* block : Blocks()) + { // TODO: we might need to pass a flag here indicating that this block // cannot safely use the pred block assertion state. // From 4467a6762d38cd42f8615390204b5bb1b49bcf67 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 27 May 2023 14:16:21 -0700 Subject: [PATCH 3/3] track new blocks --- src/coreclr/jit/block.cpp | 34 ++++++++++++++++++++++++++++++++ src/coreclr/jit/compiler.h | 6 ++++++ src/coreclr/jit/fgbasic.cpp | 3 +++ src/coreclr/jit/fgdiagnostic.cpp | 4 ++++ src/coreclr/jit/morph.cpp | 21 +++++++++++--------- src/coreclr/jit/morphblock.cpp | 1 + 6 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/block.cpp b/src/coreclr/jit/block.cpp index 7c57df63822c7c..13bae600f17840 100644 --- a/src/coreclr/jit/block.cpp +++ b/src/coreclr/jit/block.cpp @@ -1396,6 +1396,11 @@ BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind) block = new (this, CMK_BasicBlock) BasicBlock; + if (fgTrackNewBlockCreation) + { + fgNewBBs->push_back(block); + } + #if MEASURE_BLOCK_SIZE BasicBlock::s_Count += 1; BasicBlock::s_Size += sizeof(*block); @@ -1497,6 +1502,35 @@ BasicBlock* Compiler::bbNewBasicBlock(BBjumpKinds jumpKind) return block; } +//------------------------------------------------------------------------ +// fgEnableNewBlockTracking: start tracking creation of new blocks +// +void Compiler::fgEnableNewBlockTracking() +{ + assert(!fgTrackNewBlockCreation); + fgTrackNewBlockCreation = true; + + if (fgNewBBs == nullptr) + { + CompAllocator allocator = getAllocator(CMK_BasicBlock); + fgNewBBs = new (allocator) jitstd::vector(allocator); + } + + fgNewBBs->clear(); +} + +//------------------------------------------------------------------------ +// fgDisableNewBlockTracking: stop tracking creation of new blocks +// +// Notes: +// Does not alter fgNewBBs +// +void Compiler::fgDisableNewBlockTracking() +{ + assert(fgTrackNewBlockCreation); + fgTrackNewBlockCreation = false; +} + //------------------------------------------------------------------------ // isBBCallAlwaysPair: Determine if this is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair // diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a3a5617cdb7fd2..56d2bff60900c0 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3182,6 +3182,12 @@ class Compiler bool fgSafeBasicBlockCreation; #endif + bool fgTrackNewBlockCreation; + jitstd::vector* fgNewBBs; + + void fgEnableNewBlockTracking(); + void fgDisableNewBlockTracking(); + BasicBlock* bbNewBasicBlock(BBjumpKinds jumpKind); /* diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index a95a2dcadb622e..4e259a21f016ba 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -84,6 +84,9 @@ void Compiler::fgInit() fgSafeBasicBlockCreation = true; #endif // DEBUG + fgTrackNewBlockCreation = false; + fgNewBBs = nullptr; + fgLocalVarLivenessDone = false; fgIsDoingEarlyLiveness = false; fgDidEarlyLiveness = false; diff --git a/src/coreclr/jit/fgdiagnostic.cpp b/src/coreclr/jit/fgdiagnostic.cpp index 2968d808895934..18a789a90895a0 100644 --- a/src/coreclr/jit/fgdiagnostic.cpp +++ b/src/coreclr/jit/fgdiagnostic.cpp @@ -2785,6 +2785,10 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef fgDebugCheckBlockLinks(); fgFirstBBisScratch(); + // We should not be leaving new block tracking enabled across phases + // + assert(!fgTrackNewBlockCreation); + if (fgBBcount > 10000 && expensiveDebugCheckLevel < 1) { // The basic block checks are too expensive if there are too many blocks, diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 5803a34fa54954..a9fed97831dbfd 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13664,30 +13664,33 @@ PhaseStatus Compiler::fgMorphBlocks() fgComputeEnterBlocksSet(); fgDfsReversePostorder(); - // Morph can introduce new blocks, so remember - // how many there were to start with + // Morph can introduce new blocks, so enable tracking block creation // unsigned const bbNumMax = fgBBNumMax; - + fgEnableNewBlockTracking(); for (unsigned i = 1; i <= bbNumMax; i++) { BasicBlock* const block = fgBBReversePostorder[i]; fgMorphBlock(block); } + fgDisableNewBlockTracking(); // If morph created new blocks, then morph them as well. - // (TODO: track this set more efficiently) + // Temporarily forbid creating even more new blocks so + // we don't have to iterate until closure. + // // (TODO: verify that this potential out of order processing does not cause issues) // - if (fgBBNumMax != bbNumMax) + if (fgNewBBs->size() > 0) { - for (BasicBlock* block : Blocks()) + JITDUMP("Morph created %u new blocks, processing them out of RPO\n", fgNewBBs->size()); + + INDEBUG(fgSafeBasicBlockCreation = false); + for (BasicBlock* const block : *fgNewBBs) { - // TODO: we might need to pass a flag here indicating that this block - // cannot safely use the pred block assertion state. - // fgMorphBlock(block); } + INDEBUG(fgSafeBasicBlockCreation = true); } } diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 074abb1f77cd32..17811cd327a4c7 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -831,6 +831,7 @@ void MorphCopyBlockHelper::MorphStructCases() bool srcFldIsProfitable = ((m_srcVarDsc != nullptr) && (!m_srcVarDsc->lvDoNotEnregister || (m_srcVarDsc->HasGCPtr() && (m_dstVarDsc == nullptr)) || (m_srcVarDsc->lvFieldCnt == 1))); + // Are both dest and src promoted structs? if (m_dstDoFldStore && m_srcDoFldStore && (dstFldIsProfitable || srcFldIsProfitable)) {