JIT: make global morph its own phase, morph blocks in RPO

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<BasicBlock*>(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
 //

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
     //

src/coreclr/jit/compiler.h

@@ -3182,6 +3182,12 @@ class Compiler
     bool fgSafeBasicBlockCreation;
 #endif
 
+    bool fgTrackNewBlockCreation;
+    jitstd::vector<BasicBlock*>* fgNewBBs;
+
+    void fgEnableNewBlockTracking();
+    void fgDisableNewBlockTracking();
+
     BasicBlock* bbNewBasicBlock(BBjumpKinds jumpKind);
 
     /*
@@ -4751,9 +4757,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));

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)

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;

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,

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,57 @@ void Compiler::fgMorphBlocks()
         lvSetMinOptsDoNotEnreg();
     }
 
-    /*-------------------------------------------------------------------------
-     * Process all basic blocks in the function
-     */
-
-    BasicBlock* block = fgFirstBB;
-    noway_assert(block);
-
-    do
+    // Morph all blocks. If we aren't then we just morph in normal bbNext order.
+    //
+    if (!optLocalAssertionProp)
     {
-#ifdef DEBUG
-        if (verbose)
+        // 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())
         {
-            printf("\nMorphing " FMT_BB " of '%s'\n", block->bbNum, info.compFullName);
+            fgMorphBlock(block);
         }
-#endif
+    }
+    else
+    {
+        // We are optimizing. Process in RPO.
+        //
+        fgRenumberBlocks();
+        EnsureBasicBlockEpoch();
+        fgComputeEnterBlocksSet();
+        fgDfsReversePostorder();
 
-        if (optLocalAssertionProp)
+        // Morph can introduce new blocks, so enable tracking block creation
+        //
+        unsigned const bbNumMax = fgBBNumMax;
+        fgEnableNewBlockTracking();
+        for (unsigned i = 1; i <= bbNumMax; i++)
         {
-            //
-            // Clear out any currently recorded assertion candidates
-            // before processing each basic block,
-            // also we must  handle QMARK-COLON specially
-            //
-            optAssertionReset(0);
+            BasicBlock* const block = fgBBReversePostorder[i];
+            fgMorphBlock(block);
         }
+        fgDisableNewBlockTracking();
 
-        // Make the current basic block address available globally.
-        compCurBB = block;
-
-        // 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 morph created new blocks, then morph them as well.
+        // 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 (fgNewBBs->size() > 0)
         {
-            if ((genReturnBB != nullptr) && (genReturnBB != block))
+            JITDUMP("Morph created %u new blocks, processing them out of RPO\n", fgNewBBs->size());
+
+            INDEBUG(fgSafeBasicBlockCreation = false);
+            for (BasicBlock* const block : *fgNewBBs)
             {
-                fgMergeBlockReturn(block);
+                fgMorphBlock(block);
             }
+            INDEBUG(fgSafeBasicBlockCreation = true);
         }
-
-        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 +13708,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;
 }
 
 //------------------------------------------------------------------------

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))
         {