Skip to content

Commit

Permalink
JIT: build pred lists first (#81448)
Browse files Browse the repository at this point in the history 
Move pred list building before importation. It now runs as the first phase in
the phase list.

* Split up some unions in block.h as some things can't share storage anymore
(may revisit this later).
* Modify importer not to rely on cheap preds. Most of the work here is in
`impImportLeave`.
* Adjust OSR protection strategy for the method entry. In particular, watch
for the degenerate case where the OSR entry is the method entry.
* Ensure we don't double-decrement some ref counts when blocks with degenerate
or folded flow are re-imported.
* Update spill clique discovery to use the actual pred lists.
* Add new method `impFixPredLists` for the importer to use at the end of
importation. This adds pred list entries finally returns, which can't be
done until all `BBJ_LEAVE` blocks are processed.
* trigger badcode inside `fgComputePreds`
* fix issue with `STRESS_CATCH_ARG`

Contributes to #80193.
  • Loading branch information
@AndyAyersMS
AndyAyersMS authored Feb 2, 2023
1 parent ac18cc9 commit 75408bb
Show file tree
Hide file tree
Showing 11 changed files with 313 additions and 188 deletions.
15 changes: 8 additions & 7 deletions src/coreclr/jit/block.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -902,10 +902,7 @@ struct BasicBlock : private LIR::Range
m_firstNode = tree;
}

union {
EntryState* bbEntryState; // verifier tracked state of all entries in stack.
flowList* bbLastPred; // last pred list entry
};
EntryState* bbEntryState; // verifier tracked state of all entries in stack.

#define NO_BASE_TMP UINT_MAX // base# to use when we have none

Expand Down Expand Up @@ -1091,10 +1088,14 @@ struct BasicBlock : private LIR::Range
BlockSet bbReach; // Set of all blocks that can reach this one

union {
BasicBlock* bbIDom; // Represent the closest dominator to this block (called the Immediate
// Dominator) used to compute the dominance tree.
void* bbSparseProbeList; // Used early on by fgInstrument
BasicBlock* bbIDom; // Represent the closest dominator to this block (called the Immediate
// Dominator) used to compute the dominance tree.
flowList* bbLastPred; // Used early on by fgComputePreds
};

union {
void* bbSparseCountInfo; // Used early on by fgIncorporateEdgeCounts
void* bbSparseProbeList; // Used early on by fgInstrument
};

unsigned bbPostOrderNum; // the block's post order number in the graph.
Expand Down
34 changes: 13 additions & 21 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4347,6 +4347,17 @@ void Compiler::EndPhase(Phases phase)
//
void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFlags* compileFlags)
{
compFunctionTraceStart();

// Compute bbRefs and bbPreds
//
auto computePredsPhase = [this]() {
fgComputePreds();
// Enable flow graph checks
activePhaseChecks |= PhaseChecks::CHECK_FG;
};
DoPhase(this, PHASE_COMPUTE_PREDS, computePredsPhase);

// Prepare for importation
//
auto preImportPhase = [this]() {
Expand Down Expand Up @@ -4375,8 +4386,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
};
DoPhase(this, PHASE_PRE_IMPORT, preImportPhase);

compFunctionTraceStart();

// Incorporate profile data.
//
// Note: the importer is sensitive to block weights, so this has
Expand All @@ -4401,8 +4410,8 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl

// Enable the post-phase checks that use internal logic to decide when checking makes sense.
//
activePhaseChecks = PhaseChecks::CHECK_EH | PhaseChecks::CHECK_LOOPS | PhaseChecks::CHECK_UNIQUE |
PhaseChecks::CHECK_PROFILE | PhaseChecks::CHECK_LINKED_LOCALS;
activePhaseChecks |= PhaseChecks::CHECK_EH | PhaseChecks::CHECK_LOOPS | PhaseChecks::CHECK_UNIQUE |
PhaseChecks::CHECK_PROFILE | PhaseChecks::CHECK_LINKED_LOCALS;

// Import: convert the instrs in each basic block to a tree based intermediate representation
//
Expand All @@ -4425,23 +4434,6 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl
return;
}

// Compute bbNum, bbRefs and bbPreds
//
// This is the first time full (not cheap) preds will be computed.
// And, if we have profile data, we can now check integrity.
//
// From this point on the flowgraph information such as bbNum,
// bbRefs or bbPreds has to be kept updated.
//
auto computePredsPhase = [this]() {
JITDUMP("\nRenumbering the basic blocks for fgComputePred\n");
fgRenumberBlocks();
fgComputePreds();
// Enable flow graph checks
activePhaseChecks |= PhaseChecks::CHECK_FG;
};
DoPhase(this, PHASE_COMPUTE_PREDS, computePredsPhase);

// If instrumenting, add block and class probes.
//
if (compileFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR))
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3687,6 +3687,7 @@ class Compiler
public:
void impInit();
void impImport();
void impFixPredLists();

CORINFO_CLASS_HANDLE impGetRefAnyClass();
CORINFO_CLASS_HANDLE impGetRuntimeArgumentHandle();
Expand Down Expand Up @@ -4397,6 +4398,7 @@ class Compiler
DomTreeNode* fgSsaDomTree;

bool fgBBVarSetsInited;
bool fgOSROriginalEntryBBProtected;

// Allocate array like T* a = new T[fgBBNumMax + 1];
// Using helper so we don't keep forgetting +1.
Expand Down
17 changes: 12 additions & 5 deletions src/coreclr/jit/fgbasic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,12 @@ void Compiler::fgInit()

/* Initialize the basic block list */

fgFirstBB = nullptr;
fgLastBB = nullptr;
fgFirstColdBlock = nullptr;
fgEntryBB = nullptr;
fgOSREntryBB = nullptr;
fgFirstBB = nullptr;
fgLastBB = nullptr;
fgFirstColdBlock = nullptr;
fgEntryBB = nullptr;
fgOSREntryBB = nullptr;
fgOSROriginalEntryBBProtected = false;

#if defined(FEATURE_EH_FUNCLETS)
fgFirstFuncletBB = nullptr;
Expand Down Expand Up @@ -3922,6 +3923,10 @@ void Compiler::fgCheckForLoopsInHandlers()
// the middle of the try. But we defer that until after importation.
// See fgPostImportationCleanup.
//
// Also protect the original method entry, if it was imported, since
// we may decide to branch there during morph as part of the tail recursion
// to loop optimization.
//
void Compiler::fgFixEntryFlowForOSR()
{
// Ensure lookup IL->BB lookup table is valid
Expand All @@ -3944,6 +3949,8 @@ void Compiler::fgFixEntryFlowForOSR()
// Now branch from method start to the right spot.
//
fgEnsureFirstBBisScratch();
assert(fgFirstBB->bbJumpKind == BBJ_NONE);
fgRemoveRefPred(fgFirstBB->bbNext, fgFirstBB);
fgFirstBB->bbJumpKind = BBJ_ALWAYS;
fgFirstBB->bbJumpDest = osrEntry;
fgAddRefPred(osrEntry, fgFirstBB);
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/fgdiagnostic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -785,7 +785,7 @@ bool Compiler::fgDumpFlowGraph(Phases phase, PhasePosition pos)
return false;
}

JITDUMP("Dumping flow graph %s phase %s\n", (pos == PhasePosition::PrePhase) ? "before" : "after",
JITDUMP("Writing out flow graph %s phase %s\n", (pos == PhasePosition::PrePhase) ? "before" : "after",
PhaseNames[phase]);

bool validWeights = fgHaveValidEdgeWeights;
Expand Down Expand Up @@ -2664,8 +2664,8 @@ bool BBPredsChecker::CheckJump(BasicBlock* blockPred, BasicBlock* block)
break;

case BBJ_LEAVE:
// We may see BBJ_LEAVE preds in unimported blocks if we haven't done cleanup yet.
if (!comp->compPostImportationCleanupDone && ((blockPred->bbFlags & BBF_IMPORTED) == 0))
// We may see BBJ_LEAVE preds if we haven't done cleanup yet.
if (!comp->compPostImportationCleanupDone)
{
return true;
}
Expand Down Expand Up @@ -2907,7 +2907,7 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef

// Under OSR, if we also are keeping the original method entry around,
// mark that as implicitly referenced as well.
if (opts.IsOSR() && (block == fgEntryBB) && ((block->bbFlags & BBF_IMPORTED) != 0))
if (opts.IsOSR() && (block == fgEntryBB) && fgOSROriginalEntryBBProtected)
{
blockRefs += 1;
}
Expand Down
12 changes: 2 additions & 10 deletions src/coreclr/jit/fgflow.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -693,14 +693,6 @@ void Compiler::fgComputePreds()
// the first block is always reachable
fgFirstBB->bbRefs = 1;

// Under OSR, we may need to specially protect the original method entry.
//
if (opts.IsOSR() && (fgEntryBB != nullptr) && (fgEntryBB->bbFlags & BBF_IMPORTED))
{
JITDUMP("OSR: protecting original method entry " FMT_BB "\n", fgEntryBB->bbNum);
fgEntryBB->bbRefs = 1;
}

for (BasicBlock* const block : Blocks())
{
switch (block->bbJumpKind)
Expand Down Expand Up @@ -760,15 +752,15 @@ void Compiler::fgComputePreds()

if (!block->hasHndIndex())
{
NO_WAY("endfinally outside a finally/fault block.");
BADCODE("endfinally outside a finally/fault block.");
}

unsigned hndIndex = block->getHndIndex();
EHblkDsc* ehDsc = ehGetDsc(hndIndex);

if (!ehDsc->HasFinallyOrFaultHandler())
{
NO_WAY("endfinally outside a finally/fault block.");
BADCODE("endfinally outside a finally/fault block.");
}

if (ehDsc->HasFinallyHandler())
Expand Down
8 changes: 0 additions & 8 deletions src/coreclr/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -616,14 +616,6 @@ PhaseStatus Compiler::fgImport()
compInlineResult->SetImportedILSize(info.compILImportSize);
}

// Full preds are only used later on
assert(!fgComputePredsDone);
if (fgCheapPredsValid)
{
// Cheap predecessors are only used during importation
fgRemovePreds();
}

return PhaseStatus::MODIFIED_EVERYTHING;
}

Expand Down
Loading

0 comments on commit 75408bb

Please sign in to comment.