Stop tracking genReturnBB after global morph (dotnet#97130)

* Stop tracking `genReturnBB` after global morph

The `genReturnBB` pointer is set if a merged return block is created
during `fgAddInternal` (so, quite early during compilation). It needs
to be maintained (and the unreferenced block it points to needs to be
kept around) until global morph, which hooks up flow to this block.

After global morph, clear the pointer and don't maintain it; it is no
longer needed. Later code that was checking for it can instead check
for `BBJ_RETURN` blocks or `GT_RETURN` nodes. Also, remove the marking
of the `genReturnBB` block as `BBF_DONT_REMOVE`.

This leads to diffs, as unreachable return blocks get deleted. This can
happen, say, if all other exits are tail calls. If that happens and we're
in a case of needing a profiler hook, then the profile exit hook is also
not needed (it would be unreachable).

* Allow x86 synchronized methods to have unreachable return blocks

The JIT still needs to report an end-of-synchronized-range offset
in the GC info for x86 synchronized methods, so just create a new
label at the end of all blocks. The VM will automatically exit
the synchronization on an exceptional method exit.

* Create final IG before clearing GC info

Author: BruceForstall

src/coreclr/gcdump/i386/gcdumpx86.cpp

@@ -154,7 +154,8 @@ size_t            GCDump::DumpInfoHdr (PTR_CBYTE      gcInfoBlock,
                                 gcPrintf("    GuardStack cookie  = [%s%u]\n",
                                           header->ebpFrame ? "EBP-" : "ESP+", header->gsCookieOffset);
     if (header->syncStartOffset != INVALID_SYNC_OFFSET)
-                                gcPrintf("    Sync region = [%u,%u]\n",
+                                gcPrintf("    Sync region = [%u,%u] ([0x%x,0x%x])\n",
+                                          header->syncStartOffset, header->syncEndOffset,
                                           header->syncStartOffset, header->syncEndOffset);
 
     if  (header->epilogCount > 1 || (header->epilogCount != 0 &&

src/coreclr/jit/codegencommon.cpp

@@ -7499,6 +7499,7 @@ void CodeGen::genLongReturn(GenTree* treeNode)
 //------------------------------------------------------------------------
 // genReturn: Generates code for return statement.
 //            In case of struct return, delegates to the genStructReturn method.
+//            In case of LONG return on 32-bit, delegates to the genLongReturn method.
 //
 // Arguments:
 //    treeNode - The GT_RETURN or GT_RETFILT tree node.
@@ -7508,7 +7509,8 @@ void CodeGen::genLongReturn(GenTree* treeNode)
 //
 void CodeGen::genReturn(GenTree* treeNode)
 {
-    assert(treeNode->OperGet() == GT_RETURN || treeNode->OperGet() == GT_RETFILT);
+    assert(treeNode->OperIs(GT_RETURN, GT_RETFILT));
+
     GenTree*  op1        = treeNode->gtGetOp1();
     var_types targetType = treeNode->TypeGet();
 
@@ -7609,9 +7611,9 @@ void CodeGen::genReturn(GenTree* treeNode)
     // maintain such an invariant irrespective of whether profiler hook needed or not.
     // Also, there is not much to be gained by materializing it as an explicit node.
     //
-    // There should be a single return block while generating profiler ELT callbacks,
-    // so we just look for that block to trigger insertion of the profile hook.
-    if ((compiler->compCurBB == compiler->genReturnBB) && compiler->compIsProfilerHookNeeded())
+    // There should be a single GT_RETURN while generating profiler ELT callbacks.
+    //
+    if (treeNode->OperIs(GT_RETURN) && compiler->compIsProfilerHookNeeded())
     {
         // !! NOTE !!
         // Since we are invalidating the assumption that we would slip into the epilog

src/coreclr/jit/codegenlinear.cpp

@@ -835,6 +835,25 @@ void CodeGen::genCodeForBBlist()
 #endif // DEBUG
     }  //------------------ END-FOR each block of the method -------------------
 
+#if !defined(FEATURE_EH_FUNCLETS)
+    // If this is a synchronized method on x86, and we generated all the code without
+    // generating the "exit monitor" call, then we must have deleted the single return block
+    // with that call because it was dead code. We still need to report the monitor range
+    // to the VM in the GC info, so create a label at the very end so we have a marker for
+    // the monitor end range.
+    //
+    // Do this before cleaning the GC refs below; we don't want to create an IG that clears
+    // the `this` pointer for lvaKeepAliveAndReportThis.
+
+    if ((compiler->info.compFlags & CORINFO_FLG_SYNCH) && (compiler->syncEndEmitCookie == nullptr))
+    {
+        JITDUMP("Synchronized method with missing exit monitor call; adding final label\n");
+        compiler->syncEndEmitCookie =
+            GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur, gcInfo.gcRegByrefSetCur);
+        noway_assert(compiler->syncEndEmitCookie != nullptr);
+    }
+#endif // !FEATURE_EH_FUNCLETS
+
     // There could be variables alive at this point. For example see lvaKeepAliveAndReportThis.
     // This call is for cleaning the GC refs
     genUpdateLife(VarSetOps::MakeEmpty(compiler));

src/coreclr/jit/codegenxarch.cpp

@@ -6176,17 +6176,17 @@ void CodeGen::genCall(GenTreeCall* call)
         {
             case CORINFO_HELP_MON_ENTER:
             case CORINFO_HELP_MON_ENTER_STATIC:
-                noway_assert(compiler->syncStartEmitCookie == NULL);
+                noway_assert(compiler->syncStartEmitCookie == nullptr);
                 compiler->syncStartEmitCookie =
                     GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur, gcInfo.gcRegByrefSetCur);
-                noway_assert(compiler->syncStartEmitCookie != NULL);
+                noway_assert(compiler->syncStartEmitCookie != nullptr);
                 break;
             case CORINFO_HELP_MON_EXIT:
             case CORINFO_HELP_MON_EXIT_STATIC:
-                noway_assert(compiler->syncEndEmitCookie == NULL);
+                noway_assert(compiler->syncEndEmitCookie == nullptr);
                 compiler->syncEndEmitCookie =
                     GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur, gcInfo.gcRegByrefSetCur);
-                noway_assert(compiler->syncEndEmitCookie != NULL);
+                noway_assert(compiler->syncEndEmitCookie != nullptr);
                 break;
             default:
                 break;

src/coreclr/jit/compiler.cpp

@@ -3332,6 +3332,12 @@ void Compiler::compInitOptions(JitFlags* jitFlags)
         printf("OPTIONS: compProcedureSplitting   = %s\n", dspBool(opts.compProcedureSplitting));
         printf("OPTIONS: compProcedureSplittingEH = %s\n", dspBool(opts.compProcedureSplittingEH));
 
+        // This is rare; don't clutter up the dump with it normally.
+        if (compProfilerHookNeeded)
+        {
+            printf("OPTIONS: compProfilerHookNeeded   = %s\n", dspBool(compProfilerHookNeeded));
+        }
+
         if (jitFlags->IsSet(JitFlags::JIT_FLAG_BBOPT))
         {
             printf("OPTIONS: optimizer should use profile data\n");

src/coreclr/jit/compiler.h

@@ -5358,8 +5358,6 @@ class Compiler
     IL_OFFSET fgFindBlockILOffset(BasicBlock* block);
     void fgFixEntryFlowForOSR();
 
-    void fgUpdateSingleReturnBlock(BasicBlock* block);
-
     BasicBlock* fgSplitBlockAtBeginning(BasicBlock* curr);
     BasicBlock* fgSplitBlockAtEnd(BasicBlock* curr);
     BasicBlock* fgSplitBlockAfterStatement(BasicBlock* curr, Statement* stmt);
@@ -10745,14 +10743,14 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
     unsigned  compHndBBtabCount;      // element count of used elements in EH data array
     unsigned  compHndBBtabAllocCount; // element count of allocated elements in EH data array
 
-#if defined(TARGET_X86)
+#if !defined(FEATURE_EH_FUNCLETS)
 
     //-------------------------------------------------------------------------
     //  Tracking of region covered by the monitor in synchronized methods
     void* syncStartEmitCookie; // the emitter cookie for first instruction after the call to MON_ENTER
     void* syncEndEmitCookie;   // the emitter cookie for first instruction after the call to MON_EXIT
 
-#endif // !TARGET_X86
+#endif // !FEATURE_EH_FUNCLETS
 
     Phases      mostRecentlyActivePhase; // the most recently active phase
     PhaseChecks activePhaseChecks;       // the currently active phase checks

src/coreclr/jit/compiler.hpp

@@ -2320,7 +2320,7 @@ inline void LclVarDsc::incRefCnts(weight_t weight, Compiler* comp, RefCountState
 
 inline bool Compiler::lvaKeepAliveAndReportThis()
 {
-    if (info.compIsStatic || lvaTable[0].TypeGet() != TYP_REF)
+    if (info.compIsStatic || (lvaTable[0].TypeGet() != TYP_REF))
     {
         return false;
     }
@@ -2354,7 +2354,7 @@ inline bool Compiler::lvaKeepAliveAndReportThis()
     // We keep it alive in the lookup scenario, even when the VM didn't ask us to,
     // because collectible types need the generics context when gc-ing.
     //
-    // Methoods that can inspire OSR methods must always report context as live
+    // Methods that can inspire OSR methods must always report context as live
     //
     if (genericsContextIsThis)
     {
@@ -4977,8 +4977,9 @@ unsigned Compiler::fgRunDfs(VisitPreorder visitPreorder, VisitPostorder visitPos
         dfsFrom(fgEntryBB);
     }
 
-    if ((genReturnBB != nullptr) && !BitVecOps::IsMember(&traits, visited, genReturnBB->bbNum) && !fgGlobalMorphDone)
+    if ((genReturnBB != nullptr) && !BitVecOps::IsMember(&traits, visited, genReturnBB->bbNum))
     {
+        assert(!fgGlobalMorphDone);
         // We introduce the merged return BB before morph and will redirect
         // other returns to it as part of morph; keep it reachable.
         dfsFrom(genReturnBB);

src/coreclr/jit/emit.cpp

@@ -2333,16 +2333,11 @@ void emitter::emitGeneratePrologEpilog()
 
 void emitter::emitStartPrologEpilogGeneration()
 {
-    /* Save the current IG if it's non-empty */
-
-    if (emitCurIGnonEmpty())
+    // Save the current IG if we have one. It might be empty if we added an end-of-compilation label.
+    if (emitCurIG != nullptr)
     {
         emitSavIG();
     }
-    else
-    {
-        assert(emitCurIG == nullptr);
-    }
 }
 
 /*****************************************************************************

src/coreclr/jit/fgbasic.cpp

@@ -4755,24 +4755,6 @@ IL_OFFSET Compiler::fgFindBlockILOffset(BasicBlock* block)
     return BAD_IL_OFFSET;
 }
 
-//------------------------------------------------------------------------------
-// fgUpdateSingleReturnBlock : A block has been split. If it was the single return
-// block, then update the single return block pointer.
-//
-// Arguments:
-//    block - The block that was split
-//
-void Compiler::fgUpdateSingleReturnBlock(BasicBlock* block)
-{
-    assert(block->KindIs(BBJ_ALWAYS));
-    if (genReturnBB == block)
-    {
-        assert(block->GetTarget()->KindIs(BBJ_RETURN));
-        JITDUMP("Updating genReturnBB from " FMT_BB " to " FMT_BB "\n", block->bbNum, block->GetTarget()->bbNum);
-        genReturnBB = block->GetTarget();
-    }
-}
-
 //------------------------------------------------------------------------------
 // fgSplitBlockAtEnd - split the given block into two blocks.
 //                   All code in the block stays in the original block.
@@ -4849,8 +4831,6 @@ BasicBlock* Compiler::fgSplitBlockAtEnd(BasicBlock* curr)
 
     fgAddRefPred(newBlock, curr);
 
-    fgUpdateSingleReturnBlock(curr);
-
     return newBlock;
 }
 

src/coreclr/jit/fgdiagnostic.cpp

@@ -2255,10 +2255,10 @@ void Compiler::fgTableDispBasicBlock(BasicBlock* block, int ibcColWidth /* = 0 *
         }
     }
 
-    // Indicate if it's the single return block
+    // Indicate if it's the merged return block.
     if (block == genReturnBB)
     {
-        printf(" one-return");
+        printf(" merged-return");
     }
 
     printf("\n");

src/coreclr/jit/flowgraph.cpp

@@ -1868,7 +1868,7 @@ class MergedReturns
                 return;
             }
 
-            // We'e reached our threshold
+            // We've reached our threshold
             mergingReturns = true;
 
             // Merge any returns we've already identified
@@ -2478,29 +2478,25 @@ PhaseStatus Compiler::fgAddInternal()
 
     if (info.compFlags & CORINFO_FLG_SYNCH)
     {
-        GenTree* tree = NULL;
+        GenTree* tree = nullptr;
 
         /* Insert the expression "enterCrit(this)" or "enterCrit(handle)" */
 
         if (info.compIsStatic)
         {
             tree = fgGetCritSectOfStaticMethod();
-
             tree = gtNewHelperCallNode(CORINFO_HELP_MON_ENTER_STATIC, TYP_VOID, tree);
         }
         else
         {
             noway_assert(lvaTable[info.compThisArg].lvType == TYP_REF);
-
             tree = gtNewLclvNode(info.compThisArg, TYP_REF);
-
             tree = gtNewHelperCallNode(CORINFO_HELP_MON_ENTER, TYP_VOID, tree);
         }
 
         /* Create a new basic block and stick the call in it */
 
         fgEnsureFirstBBisScratch();
-
         fgNewStmtAtEnd(fgFirstBB, tree);
 
 #ifdef DEBUG
@@ -2522,13 +2518,11 @@ PhaseStatus Compiler::fgAddInternal()
         if (info.compIsStatic)
         {
             tree = fgGetCritSectOfStaticMethod();
-
             tree = gtNewHelperCallNode(CORINFO_HELP_MON_EXIT_STATIC, TYP_VOID, tree);
         }
         else
         {
             tree = gtNewLclvNode(info.compThisArg, TYP_REF);
-
             tree = gtNewHelperCallNode(CORINFO_HELP_MON_EXIT, TYP_VOID, tree);
         }
 
@@ -2537,15 +2531,16 @@ PhaseStatus Compiler::fgAddInternal()
 #ifdef DEBUG
         if (verbose)
         {
-            printf("\nSynchronized method - Add exit expression ");
-            printTreeID(tree);
+            printf("\nSynchronized method - Add exitCrit statement in single return block %s\n",
+                   genReturnBB->dspToString());
+            gtDispTree(tree);
             printf("\n");
         }
 #endif
 
         // Reset cookies used to track start and end of the protected region in synchronized methods
-        syncStartEmitCookie = NULL;
-        syncEndEmitCookie   = NULL;
+        syncStartEmitCookie = nullptr;
+        syncEndEmitCookie   = nullptr;
         madeChanges         = true;
     }
 

src/coreclr/jit/gcencode.cpp

@@ -1563,15 +1563,16 @@ size_t GCInfo::gcInfoBlockHdrSave(
 
     header->syncStartOffset = INVALID_SYNC_OFFSET;
     header->syncEndOffset   = INVALID_SYNC_OFFSET;
+
 #ifndef UNIX_X86_ABI
     // JIT is responsible for synchronization on funclet-based EH model that x86/Linux uses.
     if (compiler->info.compFlags & CORINFO_FLG_SYNCH)
     {
-        assert(compiler->syncStartEmitCookie != NULL);
+        assert(compiler->syncStartEmitCookie != nullptr);
         header->syncStartOffset = compiler->GetEmitter()->emitCodeOffset(compiler->syncStartEmitCookie, 0);
         assert(header->syncStartOffset != INVALID_SYNC_OFFSET);
 
-        assert(compiler->syncEndEmitCookie != NULL);
+        assert(compiler->syncEndEmitCookie != nullptr);
         header->syncEndOffset = compiler->GetEmitter()->emitCodeOffset(compiler->syncEndEmitCookie, 0);
         assert(header->syncEndOffset != INVALID_SYNC_OFFSET);
 

src/coreclr/jit/lower.cpp

@@ -4271,7 +4271,7 @@ void Lowering::LowerRet(GenTreeUnOp* ret)
     }
 
     // Method doing PInvokes has exactly one return block unless it has tail calls.
-    if (comp->compMethodRequiresPInvokeFrame() && (comp->compCurBB == comp->genReturnBB))
+    if (comp->compMethodRequiresPInvokeFrame())
     {
         InsertPInvokeMethodEpilog(comp->compCurBB DEBUGARG(ret));
     }
@@ -5369,7 +5369,7 @@ void Lowering::InsertPInvokeMethodEpilog(BasicBlock* returnBB DEBUGARG(GenTree*
     JITDUMP("======= Inserting PInvoke method epilog\n");
 
     // Method doing PInvoke calls has exactly one return block unless it has "jmp" or tail calls.
-    assert(((returnBB == comp->genReturnBB) && returnBB->KindIs(BBJ_RETURN)) || returnBB->endsWithTailCallOrJmp(comp));
+    assert(returnBB->KindIs(BBJ_RETURN) || returnBB->endsWithTailCallOrJmp(comp));
 
     LIR::Range& returnBlockRange = LIR::AsRange(returnBB);
 

src/coreclr/jit/morph.cpp

@@ -14140,6 +14140,14 @@ PhaseStatus Compiler::fgMorphBlocks()
         fgEntryBB = nullptr;
     }
 
+    // We don't maintain `genReturnBB` after this point.
+    if (genReturnBB != nullptr)
+    {
+        // It no longer needs special "keep" treatment.
+        genReturnBB->RemoveFlags(BBF_DONT_REMOVE);
+        genReturnBB = nullptr;
+    }
+
     // We are done with the global morphing phase
     //
     fgInvalidateDfsTree();