JIT: build pred lists before instrumentation (#81288)
authorAndy Ayers <andya@microsoft.com>
Mon, 30 Jan 2023 23:25:58 +0000 (15:25 -0800)
committerGitHub <noreply@github.com>
Mon, 30 Jan 2023 23:25:58 +0000 (15:25 -0800)
Move pred list building to just before instrumentation (and just after
importation -- we are getting very close to the front of the phase list now).

The block and edge count instrumenters were both using cheap preds to keep
track of some relocated count probes. Revise this so they can use the regular
pred lists. Also rework both approaches so their `RelocateProbes` methods are
fairly similar and perhaps could be unified one day.

Contributes to #80193.

src/coreclr/jit/compiler.cpp
src/coreclr/jit/fgprofile.cpp

index 282d2f3..f5104df 100644 (file)
@@ -4425,13 +4425,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
         return;
     }
 
-    // If instrumenting, add block and class probes.
-    //
-    if (compileFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR))
-    {
-        DoPhase(this, PHASE_IBCINSTR, &Compiler::fgInstrumentMethod);
-    }
-
     // Compute bbNum, bbRefs and bbPreds
     //
     // This is the first time full (not cheap) preds will be computed.
@@ -4449,6 +4442,13 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
     };
     DoPhase(this, PHASE_COMPUTE_PREDS, computePredsPhase);
 
+    // If instrumenting, add block and class probes.
+    //
+    if (compileFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR))
+    {
+        DoPhase(this, PHASE_IBCINSTR, &Compiler::fgInstrumentMethod);
+    }
+
     // Expand any patchpoints
     //
     DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgTransformPatchpoints);
index 0d928a1..9a719e5 100644 (file)
@@ -409,10 +409,10 @@ void BlockCountInstrumentor::Prepare(bool preImport)
 //   would appear in post-tail call blocks.
 //
 // Notes:
-//   Conveys relocation information by modifying the cheap pred list.
+//   Conveys relocation information by updating the m_relocationMap.
 //
 //   Actual relocation happens during Instrument, keying off of the
-//   BBF_TAILCALL_SUCCESSOR flag and the (modified) cheap pred list.
+//   BBF_TAILCALL_SUCCESSOR flag and m_relocationMap entries.
 //
 void BlockCountInstrumentor::RelocateProbes()
 {
@@ -435,24 +435,15 @@ void BlockCountInstrumentor::RelocateProbes()
     //
     assert(!m_comp->compIsForInlining());
 
-    // Build cheap preds.
-    //
-    m_comp->fgComputeCheapPreds();
-    m_comp->EnsureBasicBlockEpoch();
-
     // Keep track of return blocks needing special treatment.
-    // We also need to track of duplicate preds.
     //
-    JitExpandArrayStack<BasicBlock*> specialReturnBlocks(m_comp->getAllocator(CMK_Pgo));
-    BlockSet                         predsSeen = BlockSetOps::MakeEmpty(m_comp);
+    ArrayStack<BasicBlock*> criticalPreds(m_comp->getAllocator(CMK_Pgo));
 
     // Walk blocks looking for BBJ_RETURNs that are successors of potential tail calls.
     //
-    // If any such has a conditional pred, we will need to reroute flow from those preds
+    // If any such block has a conditional pred, we will need to reroute flow from those preds
     // via an intermediary block. That block will subsequently hold the relocated block
-    // probe for the return for those preds.
-    //
-    // Scrub the cheap pred list for these blocks so that each pred appears at most once.
+    // probe for the returnBlock for those preds.
     //
     for (BasicBlock* const block : m_comp->Blocks())
     {
@@ -463,120 +454,73 @@ void BlockCountInstrumentor::RelocateProbes()
             continue;
         }
 
-        if ((block->bbFlags & BBF_TAILCALL_SUCCESSOR) != 0)
+        if ((block->bbFlags & BBF_TAILCALL_SUCCESSOR) == 0)
         {
-            JITDUMP("Return " FMT_BB " is successor of possible tail call\n", block->bbNum);
-            assert(block->bbJumpKind == BBJ_RETURN);
-            bool pushed = false;
-            BlockSetOps::ClearD(m_comp, predsSeen);
-            for (BasicBlockList* predEdge = block->bbCheapPreds; predEdge != nullptr; predEdge = predEdge->next)
-            {
-                BasicBlock* const pred = predEdge->block;
+            continue;
+        }
 
-                // If pred is not to be processed, ignore it and scrub from the pred list.
-                //
-                if (!ShouldProcess(pred))
-                {
-                    JITDUMP(FMT_BB " -> " FMT_BB " is dead edge\n", pred->bbNum, block->bbNum);
-                    predEdge->block = nullptr;
-                    continue;
-                }
+        JITDUMP("Return " FMT_BB " is successor of possible tail call\n", block->bbNum);
+        assert(block->bbJumpKind == BBJ_RETURN);
 
-                BasicBlock* const succ = pred->GetUniqueSucc();
+        // Scan for critical preds, and add relocated probes to non-critical preds.
+        //
+        criticalPreds.Reset();
+        for (BasicBlock* const pred : block->PredBlocks())
+        {
+            if (!ShouldProcess(pred))
+            {
+                JITDUMP(FMT_BB " -> " FMT_BB " is dead edge\n", pred->bbNum, block->bbNum);
+                continue;
+            }
 
-                if (succ == nullptr)
-                {
-                    // Flow from pred -> block is conditional, and will require updating.
-                    //
-                    JITDUMP(FMT_BB " -> " FMT_BB " is critical edge\n", pred->bbNum, block->bbNum);
-                    if (!pushed)
-                    {
-                        specialReturnBlocks.Push(block);
-                        pushed = true;
-                    }
+            BasicBlock* const succ = pred->GetUniqueSucc();
 
-                    // Have we seen this pred before?
-                    //
-                    if (BlockSetOps::IsMember(m_comp, predsSeen, pred->bbNum))
-                    {
-                        // Yes, null out the duplicate pred list entry.
-                        //
-                        predEdge->block = nullptr;
-                    }
-                }
-                else
+            if ((succ == nullptr) || pred->isBBCallAlwaysPairTail())
+            {
+                // Route pred through the intermediary.
+                //
+                JITDUMP(FMT_BB " -> " FMT_BB " is critical edge\n", pred->bbNum, block->bbNum);
+                criticalPreds.Push(pred);
+            }
+            else
+            {
+                // Ensure this pred is not a fall through.
+                //
+                if (pred->bbJumpKind == BBJ_NONE)
                 {
-                    // We should only ever see one reference to this pred.
-                    //
-                    assert(!BlockSetOps::IsMember(m_comp, predsSeen, pred->bbNum));
-
-                    // Ensure flow from non-critical preds is BBJ_ALWAYS as we
-                    // may add a new block right before block.
-                    //
-                    if (pred->bbJumpKind == BBJ_NONE)
-                    {
-                        pred->bbJumpKind = BBJ_ALWAYS;
-                        pred->bbJumpDest = block;
-                    }
-                    assert(pred->bbJumpKind == BBJ_ALWAYS);
+                    pred->bbJumpKind = BBJ_ALWAYS;
+                    pred->bbJumpDest = block;
                 }
-
-                BlockSetOps::AddElemD(m_comp, predsSeen, pred->bbNum);
+                assert(pred->bbJumpKind == BBJ_ALWAYS);
             }
         }
-    }
 
-    // Did we find any blocks with probes needing relocation?
-    //
-    if (specialReturnBlocks.Size() == 0)
-    {
-        JITDUMP("No probes need relocating\n");
-        return;
-    }
-
-    JITDUMP("%u probes need relocating\n", specialReturnBlocks.Size());
-
-    // Now process each special return block.
-    // Create an intermediary that falls through to the return.
-    // Update any critical edges to target the intermediary.
-    //
-    // Note we could also route any non-tail-call pred via the
-    // intermedary. Doing so would cut down on probe duplication.
-    //
-    SetModifiedFlow();
-
-    while (specialReturnBlocks.Size() > 0)
-    {
-        bool              first        = true;
-        BasicBlock* const block        = specialReturnBlocks.Pop();
-        BasicBlock* const intermediary = m_comp->fgNewBBbefore(BBJ_NONE, block, /* extendRegion*/ true);
-
-        intermediary->bbFlags |= BBF_IMPORTED;
-        intermediary->inheritWeight(block);
-
-        for (BasicBlockList* predEdge = block->bbCheapPreds; predEdge != nullptr; predEdge = predEdge->next)
+        // If there are any critical preds, create and instrument the
+        // intermediary and reroute flow. Mark the intermediary so we make
+        // sure to instrument it later.
+        //
+        if (criticalPreds.Height() > 0)
         {
-            BasicBlock* const pred = predEdge->block;
+            BasicBlock* const intermediary = m_comp->fgNewBBbefore(BBJ_NONE, block, /* extendRegion*/ true);
+            intermediary->bbFlags |= BBF_IMPORTED | BBF_MARKED;
+            intermediary->inheritWeight(block);
+            m_comp->fgAddRefPred(block, intermediary);
+            SetModifiedFlow();
 
-            if (pred != nullptr)
+            while (criticalPreds.Height() > 0)
             {
-                BasicBlock* const succ = pred->GetUniqueSucc();
+                BasicBlock* const pred = criticalPreds.Pop();
 
-                if (succ == nullptr)
-                {
-                    // This will update all branch targets from pred.
-                    //
-                    m_comp->fgReplaceJumpTarget(pred, intermediary, block);
+                // Redirect any jumps
+                //
+                m_comp->fgReplaceJumpTarget(pred, intermediary, block);
 
-                    // Patch the pred list. Note we only need one pred list
-                    // entry pointing at intermediary.
-                    //
-                    predEdge->block = first ? intermediary : nullptr;
-                    first           = false;
-                }
-                else
+                // Handle case where we had a fall through critical edge
+                //
+                if (pred->bbNext == intermediary)
                 {
-                    assert(pred->bbJumpKind == BBJ_ALWAYS);
+                    m_comp->fgRemoveRefPred(pred, block);
+                    m_comp->fgAddRefPred(intermediary, block);
                 }
             }
         }
@@ -657,29 +601,25 @@ void BlockCountInstrumentor::Instrument(BasicBlock* block, Schema& schema, uint8
 
     if ((block->bbFlags & BBF_TAILCALL_SUCCESSOR) != 0)
     {
-        // We should have built and updated cheap preds during the prepare stage.
-        //
-        assert(m_comp->fgCheapPredsValid);
-
-        // Instrument each predecessor.
+        // This block probe needs to be relocated; instrument each predecessor.
         //
         bool first = true;
-        for (BasicBlockList* predEdge = block->bbCheapPreds; predEdge != nullptr; predEdge = predEdge->next)
+        for (BasicBlock* pred : block->PredBlocks())
         {
-            BasicBlock* const pred = predEdge->block;
+            const bool isLivePred = ShouldProcess(pred) || ((pred->bbFlags & BBF_MARKED) == BBF_MARKED);
+            if (!isLivePred)
+            {
+                continue;
+            }
 
-            // We may have scrubbed cheap pred list duplicates during Prepare.
-            //
-            if (pred != nullptr)
+            JITDUMP("Placing copy of block probe for " FMT_BB " in pred " FMT_BB "\n", block->bbNum, pred->bbNum);
+            if (!first)
             {
-                JITDUMP("Placing copy of block probe for " FMT_BB " in pred " FMT_BB "\n", block->bbNum, pred->bbNum);
-                if (!first)
-                {
-                    asgNode = m_comp->gtCloneExpr(asgNode);
-                }
-                m_comp->fgNewStmtAtBeg(pred, asgNode);
-                first = false;
+                asgNode = m_comp->gtCloneExpr(asgNode);
             }
+            m_comp->fgNewStmtAtBeg(pred, asgNode);
+            pred->bbFlags &= ~BBF_MARKED;
+            first = false;
         }
     }
     else
@@ -1530,7 +1470,6 @@ void EfficientEdgeCountInstrumentor::SplitCriticalEdges()
 //   would appear in post-tail call blocks.
 //
 // Notes:
-//   May build and modify the cheap pred lists.
 //   May create Leader and Duplicate probes.
 //
 void EfficientEdgeCountInstrumentor::RelocateProbes()
@@ -1554,27 +1493,17 @@ void EfficientEdgeCountInstrumentor::RelocateProbes()
     //
     assert(!m_comp->compIsForInlining());
 
-    // Build cheap preds.
-    //
-    m_comp->fgComputeCheapPreds();
-    m_comp->EnsureBasicBlockEpoch();
-
-    // Keep track of return blocks needing special treatment.
-    // We also need to track of duplicate preds.
+    // We may need to track the critical predecessors of some blocks.
     //
-    JitExpandArrayStack<BasicBlock*> specialReturnBlocks(m_comp->getAllocator(CMK_Pgo));
-    BlockSet                         retsPushed = BlockSetOps::MakeEmpty(m_comp);
-    BlockSet                         predsSeen  = BlockSetOps::MakeEmpty(m_comp);
+    ArrayStack<BasicBlock*> criticalPreds(m_comp->getAllocator(CMK_Pgo));
 
     // Walk probe list looking for probes that would appear in BBJ_RETURNs
-    // that are successors of potential tail calls.
+    // that are successors of potential tail calls, and relocate them.
     //
-    // If any such has a conditional pred, we will need to reroute flow from those preds
-    // via an intermediary block. That block will subsequently hold the relocated
+    // If any such block has a conditional pred, we will need to reroute flow from those preds
+    // via an intermediary block. That block will subsequently hold the relocated edge
     // probe for the return for those preds.
     //
-    // Scrub the cheap pred list for these blocks so that each pred appears at most once.
-    //
     for (BasicBlock* const block : m_comp->Blocks())
     {
         if (!ShouldProcess(block))
@@ -1582,115 +1511,15 @@ void EfficientEdgeCountInstrumentor::RelocateProbes()
             continue;
         }
 
-        for (Probe* probe = (Probe*)block->bbSparseProbeList; probe != nullptr; probe = probe->next)
+        // Nothing to do unless the block is a tail call successor.
+        //
+        if ((block->bbFlags & BBF_TAILCALL_SUCCESSOR) == 0)
         {
-            if (probe->kind == EdgeKind::Deleted)
-            {
-                continue;
-            }
-
-            // Figure out what block the probe will appear in.
-            // We do not expect to see any critical edges as we should have split them already.
-            //
-            BasicBlock* const source            = probe->source;
-            BasicBlock* const target            = probe->target;
-            BasicBlock*       instrumentedBlock = nullptr;
-
-            switch (probe->kind)
-            {
-                case EdgeKind::PostdominatesSource:
-                    instrumentedBlock = source;
-                    break;
-                case EdgeKind::DominatesTarget:
-                    instrumentedBlock = target;
-                    break;
-                case EdgeKind::Relocated:
-                    instrumentedBlock = block;
-                    break;
-                default:
-                    assert(!"unexpected probe kind");
-            }
-
-            assert(instrumentedBlock != nullptr);
-
-            // Nothing to do unless the block we wanted to instrument is a tail call successor.
-            //
-            if ((instrumentedBlock->bbFlags & BBF_TAILCALL_SUCCESSOR) == 0)
-            {
-                continue;
-            }
-
-            JITDUMP("Instrumentation target " FMT_BB " is successor of possible tail call\n", instrumentedBlock->bbNum);
-            assert(instrumentedBlock->bbJumpKind == BBJ_RETURN);
-
-            // We will need to relocate probes in this block. Add to our list if not already there.
-            //
-            if (!BlockSetOps::IsMember(m_comp, retsPushed, instrumentedBlock->bbNum))
-            {
-                specialReturnBlocks.Push(instrumentedBlock);
-                BlockSetOps::AddElemD(m_comp, retsPushed, instrumentedBlock->bbNum);
-            }
-
-            // Figure out which preds we'll relocate things to.
-            //
-            BlockSetOps::ClearD(m_comp, predsSeen);
-
-            for (BasicBlockList* predEdge = instrumentedBlock->bbCheapPreds; predEdge != nullptr;
-                 predEdge                 = predEdge->next)
-            {
-                BasicBlock* const pred = predEdge->block;
-                BasicBlock* const succ = pred->GetUniqueSucc();
-
-                if (succ == nullptr)
-                {
-                    // Flow from pred -> block is conditional, and will require updating.
-                    //
-                    JITDUMP(FMT_BB " -> " FMT_BB " is critical edge\n", pred->bbNum, instrumentedBlock->bbNum);
-
-                    // Have we seen this pred before?
-                    //
-                    if (BlockSetOps::IsMember(m_comp, predsSeen, pred->bbNum))
-                    {
-                        // Yes, null out the duplicate pred list entry.
-                        //
-                        predEdge->block = nullptr;
-                    }
-                }
-                else
-                {
-                    // We should only ever see one reference to this pred.
-                    //
-                    assert(!BlockSetOps::IsMember(m_comp, predsSeen, pred->bbNum));
-
-                    // Ensure flow from non-critical preds is BBJ_ALWAYS as we
-                    // may add a new block right before block.
-                    //
-                    if (pred->bbJumpKind == BBJ_NONE)
-                    {
-                        pred->bbJumpKind = BBJ_ALWAYS;
-                        pred->bbJumpDest = block;
-                    }
-                    assert(pred->bbJumpKind == BBJ_ALWAYS);
-                }
-
-                BlockSetOps::AddElemD(m_comp, predsSeen, pred->bbNum);
-            }
+            continue;
         }
-    }
 
-    // Did we find any blocks with probes needing relocation?
-    //
-    if (specialReturnBlocks.Size() == 0)
-    {
-        JITDUMP("No probes need relocating\n");
-        return;
-    }
-
-    JITDUMP("%u blocks have probes need relocating\n", specialReturnBlocks.Size());
-
-    while (specialReturnBlocks.Size() > 0)
-    {
-        BasicBlock* const block = specialReturnBlocks.Pop();
+        JITDUMP("Return " FMT_BB " is successor of possible tail call\n", block->bbNum);
+        assert(block->bbJumpKind == BBJ_RETURN);
 
         // This block should have just one probe, which we no longer need.
         //
@@ -1699,26 +1528,15 @@ void EfficientEdgeCountInstrumentor::RelocateProbes()
         assert(probe->kind == EdgeKind::PostdominatesSource);
         probe->kind = EdgeKind::Deleted;
 
-        // Any critical edge preds will probe via this intermediary block
-        // that we will create when necessary.
-        //
-        BasicBlock* intermediary = nullptr;
-
         // The first probe we add will be the leader of a duplicate probe group.
         //
         Probe* leader = nullptr;
 
-        for (BasicBlockList* predEdge = block->bbCheapPreds; predEdge != nullptr; predEdge = predEdge->next)
+        // Scan for critical preds, and add relocated probes to non-critical preds.
+        //
+        criticalPreds.Reset();
+        for (BasicBlock* const pred : block->PredBlocks())
         {
-            BasicBlock* const pred = predEdge->block;
-
-            if (pred == nullptr)
-            {
-                // Pred edge for a duplicate pred we scrubbed above.
-                //
-                continue;
-            }
-
             // Does this pred reach along a critical edge,
             // or is the pred the tail of a callfinally pair?
             //
@@ -1726,27 +1544,44 @@ void EfficientEdgeCountInstrumentor::RelocateProbes()
 
             if ((succ == nullptr) || pred->isBBCallAlwaysPairTail())
             {
-                // Yes. Create intermediary if necessary and add probe there.
-                //
-                if (intermediary == nullptr)
-                {
-                    intermediary = m_comp->fgNewBBbefore(BBJ_NONE, block, /* extendRegion*/ true);
-
-                    intermediary->bbFlags |= BBF_IMPORTED;
-                    intermediary->inheritWeight(block);
-                    NewRelocatedProbe(intermediary, probe->source, probe->target, &leader);
-                    SetModifiedFlow();
-                }
-
-                // Alter flow from pred->block to go via intermediary
+                // Route pred through the intermediary.
                 //
-                m_comp->fgReplaceJumpTarget(pred, intermediary, block);
+                JITDUMP(FMT_BB " -> " FMT_BB " is critical edge\n", pred->bbNum, block->bbNum);
+                criticalPreds.Push(pred);
             }
             else
             {
                 // Put a copy of probe into the pred.
                 //
                 NewRelocatedProbe(pred, probe->source, probe->target, &leader);
+
+                // Ensure this pred is not a fall through.
+                //
+                if (pred->bbJumpKind == BBJ_NONE)
+                {
+                    pred->bbJumpKind = BBJ_ALWAYS;
+                    pred->bbJumpDest = block;
+                }
+                assert(pred->bbJumpKind == BBJ_ALWAYS);
+            }
+        }
+
+        // If there are any critical preds, create and instrument the
+        // intermediary and reroute flow.
+        //
+        if (criticalPreds.Height() > 0)
+        {
+            BasicBlock* intermediary = m_comp->fgNewBBbefore(BBJ_NONE, block, /* extendRegion*/ true);
+            intermediary->bbFlags |= BBF_IMPORTED;
+            intermediary->inheritWeight(block);
+            m_comp->fgAddRefPred(block, intermediary);
+            NewRelocatedProbe(intermediary, probe->source, probe->target, &leader);
+            SetModifiedFlow();
+
+            while (criticalPreds.Height() > 0)
+            {
+                BasicBlock* const pred = criticalPreds.Pop();
+                m_comp->fgReplaceJumpTarget(pred, intermediary, block);
             }
         }
     }
@@ -2333,7 +2168,7 @@ PhaseStatus Compiler::fgPrepareToInstrumentMethod()
     }
     else
     {
-        JITDUMP("Using block profiling, because %s\n", edgesEnabled ? "edge profiling disabled" : "prejitting");
+        JITDUMP("Using block profiling, because %s\n", prejit ? "prejitting" : "edge profiling disabled");
         fgCountInstrumentor = new (this, CMK_Pgo) BlockCountInstrumentor(this);
     }
 
@@ -2466,13 +2301,6 @@ PhaseStatus Compiler::fgInstrumentMethod()
         fgCountInstrumentor->SuppressProbes();
         fgHistogramInstrumentor->SuppressProbes();
 
-        // If we needed to create cheap preds, we're done with them now.
-        //
-        if (fgCheapPredsValid)
-        {
-            fgRemovePreds();
-        }
-
         // We may have modified control flow preparing for instrumentation.
         //
         const bool modifiedFlow = fgCountInstrumentor->ModifiedFlow() || fgHistogramInstrumentor->ModifiedFlow();
@@ -2510,13 +2338,6 @@ PhaseStatus Compiler::fgInstrumentMethod()
     fgCountInstrumentor->InstrumentMethodEntry(schema, profileMemory);
     fgHistogramInstrumentor->InstrumentMethodEntry(schema, profileMemory);
 
-    // If we needed to create cheap preds, we're done with them now.
-    //
-    if (fgCheapPredsValid)
-    {
-        fgRemovePreds();
-    }
-
     return PhaseStatus::MODIFIED_EVERYTHING;
 }