Skip to content

Commit

Permalink
JIT: stop using ehTrueEnclosingTryIndexIL outside of importation
Browse files Browse the repository at this point in the history
Once we can inline methods with EH, IL ranges are no longer a reliable indicator
of a mutual-protect try regions. Instead, after importation, we can rely on
mutual-protect trys having the same start and end blocks.

Also update other case where we were using `info.compXcptnsCount` in morph
to decide if we needed a frame pointer. This lets us simplify the logic around
frame pointers and EH (though I still think we're making up our minds too early).

Contributes to #108900.
  • Loading branch information
AndyAyersMS committed Mar 3, 2025
1 parent 3cf22b7 commit bc50b97
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 46 deletions.
8 changes: 4 additions & 4 deletions src/coreclr/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2355,8 +2355,8 @@ void CodeGen::genReportEH()
{
for (XTnum = 0; XTnum < compiler->compHndBBtabCount; XTnum++)
{
for (enclosingTryIndex = compiler->ehTrueEnclosingTryIndexIL(XTnum); // find the true enclosing try index,
// ignoring 'mutual protect' trys
for (enclosingTryIndex = compiler->ehTrueEnclosingTryIndex(XTnum); // find the true enclosing try index,
// ignoring 'mutual protect' trys
enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX;
enclosingTryIndex = compiler->ehGetEnclosingTryIndex(enclosingTryIndex))
{
Expand Down Expand Up @@ -2662,8 +2662,8 @@ void CodeGen::genReportEH()

EHblkDsc* fletTab = compiler->ehGetDsc(XTnum2);

for (enclosingTryIndex = compiler->ehTrueEnclosingTryIndexIL(XTnum2); // find the true enclosing try index,
// ignoring 'mutual protect' trys
for (enclosingTryIndex = compiler->ehTrueEnclosingTryIndex(XTnum2); // find the true enclosing try index,
// ignoring 'mutual protect' trys
enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX;
enclosingTryIndex = compiler->ehGetEnclosingTryIndex(enclosingTryIndex))
{
Expand Down
5 changes: 5 additions & 0 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -2779,6 +2779,9 @@ class Compiler
// Find the true enclosing try index, ignoring 'mutual protect' try. Uses IL ranges to check.
unsigned ehTrueEnclosingTryIndexIL(unsigned regionIndex);

// Find the true enclosing try index, ignoring 'mutual protect' try. Uses blocks to check.
unsigned ehTrueEnclosingTryIndex(unsigned regionIndex);

// Return the index of the most nested enclosing region for a particular EH region. Returns NO_ENCLOSING_INDEX
// if there is no enclosing region. If the returned index is not NO_ENCLOSING_INDEX, then '*inTryRegion'
// is set to 'true' if the enclosing region is a 'try', or 'false' if the enclosing region is a handler.
Expand Down Expand Up @@ -5285,6 +5288,8 @@ class Compiler
// This is derived from the profile data
// or is BB_UNITY_WEIGHT when we don't have profile data

bool fgImportDone = false; // true once importation has finished

bool fgFuncletsCreated = false; // true if the funclet creation phase has been run

bool fgGlobalMorph = false; // indicates if we are during the global morphing phase
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/fgehopt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2736,7 +2736,7 @@ BasicBlock* Compiler::fgCloneTryRegion(BasicBlock* tryEntry, CloneTryInfo& info,
break;
}
outermostEbd = ehGetDsc(enclosingTryIndex);
if (!EHblkDsc::ebdIsSameILTry(outermostEbd, tryEbd))
if (!EHblkDsc::ebdIsSameTry(outermostEbd, tryEbd))
{
break;
}
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/flowgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,8 @@ PhaseStatus Compiler::fgImport()
INDEBUG(fgPgoDeferredInconsistency = false);
}

fgImportDone = true;

return PhaseStatus::MODIFIED_EVERYTHING;
}

Expand Down Expand Up @@ -6140,7 +6142,7 @@ bool FlowGraphNaturalLoop::CanDuplicateWithEH(INDEBUG(const char** reason))
//
const bool headerInTry = header->hasTryIndex();
unsigned blockIndex = block->getTryIndex();
unsigned outermostBlockIndex = comp->ehTrueEnclosingTryIndexIL(blockIndex);
unsigned outermostBlockIndex = comp->ehTrueEnclosingTryIndex(blockIndex);

if ((headerInTry && (outermostBlockIndex == header->getTryIndex())) ||
(!headerInTry && (outermostBlockIndex == EHblkDsc::NO_ENCLOSING_INDEX)))
Expand Down
67 changes: 60 additions & 7 deletions src/coreclr/jit/jiteh.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -799,14 +799,24 @@ unsigned Compiler::ehGetMostNestedRegionIndex(BasicBlock* block, bool* inTryRegi
return mostNestedRegion;
}

/*****************************************************************************
* Returns the try index of the enclosing try, skipping all EH regions with the
* same try region (that is, all 'mutual protect' regions). If there is no such
* enclosing try, returns EHblkDsc::NO_ENCLOSING_INDEX.
*/
//-------------------------------------------------------------
// ehTrueEnclosingTryIndexIL: find the outermost enclosing try
// region that is not a mutual-protect try
//
// Arguments:
// regionIndex - index of interest
//
// Returns:
// Index of enclosng non-mutual protect try region, or EHblkDsc::NO_ENCLOSING_INDEX.
//
// Notes:
// Only safe to use during importation, before we have normalize the
// EH in the flow graph. Post importation use, the non-IL version.
//
unsigned Compiler::ehTrueEnclosingTryIndexIL(unsigned regionIndex)
{
assert(regionIndex != EHblkDsc::NO_ENCLOSING_INDEX);
assert(!fgImportDone);

EHblkDsc* ehDscRoot = ehGetDsc(regionIndex);
EHblkDsc* HBtab = ehDscRoot;
Expand All @@ -832,6 +842,49 @@ unsigned Compiler::ehTrueEnclosingTryIndexIL(unsigned regionIndex)
return regionIndex;
}

//-------------------------------------------------------------
// ehTrueEnclosingTryIndex: find the closest enclosing try
// region that is not a mutual-protect try
//
// Arguments:
// regionIndex - index of interest
//
// Returns:
// Index of enclosng non-mutual protect try region, or EHblkDsc::NO_ENCLOSING_INDEX.
//
// Notes:
// Only safe to use after importation, once we have normalized the
// EH in the flow graph. For importation, use the IL version.
//
unsigned Compiler::ehTrueEnclosingTryIndex(unsigned regionIndex)
{
assert(regionIndex != EHblkDsc::NO_ENCLOSING_INDEX);
assert(fgImportDone);

EHblkDsc* ehDscRoot = ehGetDsc(regionIndex);
EHblkDsc* HBtab = ehDscRoot;

for (;;)
{
regionIndex = HBtab->ebdEnclosingTryIndex;
if (regionIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
// No enclosing 'try'; we're done
break;
}

HBtab = ehGetDsc(regionIndex);
if (!EHblkDsc::ebdIsSameTry(ehDscRoot, HBtab))
{
// Found an enclosing 'try' that has a different 'try' region (is not mutually-protect with the
// original region). Return it.
break;
}
}

return regionIndex;
}

unsigned Compiler::ehGetEnclosingRegionIndex(unsigned regionIndex, bool* inTryRegion)
{
assert(regionIndex != EHblkDsc::NO_ENCLOSING_INDEX);
Expand Down Expand Up @@ -3614,8 +3667,8 @@ void Compiler::fgVerifyHandlerTab()
// on the block.
for (XTnum = 0, HBtab = compHndBBtab; XTnum < compHndBBtabCount; XTnum++, HBtab++)
{
unsigned enclosingTryIndex = ehTrueEnclosingTryIndexIL(XTnum); // find the true enclosing try index,
// ignoring 'mutual protect' trys
unsigned enclosingTryIndex = ehTrueEnclosingTryIndex(XTnum); // find the true enclosing try index,
// ignoring 'mutual protect' trys
if (enclosingTryIndex != EHblkDsc::NO_ENCLOSING_INDEX)
{
// The handler funclet for 'XTnum' has a try index of 'enclosingTryIndex' (at least, the parts of the
Expand Down
37 changes: 6 additions & 31 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13388,47 +13388,22 @@ void Compiler::fgSetOptions()
codeGen->setFramePointerRequired(true);
}

// Assert that the EH table has been initialized by now. Note that
// compHndBBtabAllocCount never decreases; it is a high-water mark
// of table allocation. In contrast, compHndBBtabCount does shrink
// if we delete a dead EH region, and if it shrinks to zero, the
// table pointer compHndBBtab is unreliable.
assert(compHndBBtabAllocCount >= info.compXcptnsCount);

#ifdef TARGET_X86

// Note: this case, and the !X86 case below, should both use the
// !X86 path. This would require a few more changes for X86 to use
// compHndBBtabCount (the current number of EH clauses) instead of
// info.compXcptnsCount (the number of EH clauses in IL), such as
// in ehNeedsShadowSPslots(). This is because sometimes the IL has
// an EH clause that we delete as statically dead code before we
// get here, leaving no EH clauses left, and thus no requirement
// to use a frame pointer because of EH. But until all the code uses
// the same test, leave info.compXcptnsCount here. Also test for
// CORINFO_FLG_SYNCH methods which are converted into try-finally
// with Monitor helper calls in funclet ABI and need to be treated
// as methods with EH.
if (info.compXcptnsCount > 0 || (UsesFunclets() && (info.compFlags & CORINFO_FLG_SYNCH)))
// If there is EH, we need a frame pointer.
// Note this may premature... we can eliminate all EH after morph, sometimes.
//
if (compHndBBtabCount > 0)
{
codeGen->setFramePointerRequiredEH(true);

#ifdef TARGET_X86
if (UsesFunclets())
{
assert(!codeGen->isGCTypeFixed());
// Enforce fully interruptible codegen for funclet unwinding
SetInterruptible(true);
}
}

#else // !TARGET_X86

if (compHndBBtabCount > 0)
{
codeGen->setFramePointerRequiredEH(true);
}

#endif // TARGET_X86
}

if (compMethodRequiresPInvokeFrame())
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2866,7 +2866,7 @@ bool Compiler::optCreatePreheader(FlowGraphNaturalLoop* loop)
{
// Preheader should be in the true enclosing region of the header.
//
preheaderEHRegion = ehTrueEnclosingTryIndexIL(preheaderEHRegion);
preheaderEHRegion = ehTrueEnclosingTryIndex(preheaderEHRegion);
inSameRegionAsHeader = false;
break;
}
Expand Down Expand Up @@ -5208,7 +5208,7 @@ void Compiler::fgSetEHRegionForNewPreheaderOrExit(BasicBlock* block)
{
// `next` is the beginning of a try block. Figure out the EH region to use.
assert(next->hasTryIndex());
unsigned newTryIndex = ehTrueEnclosingTryIndexIL(next->getTryIndex());
unsigned newTryIndex = ehTrueEnclosingTryIndex(next->getTryIndex());
if (newTryIndex == EHblkDsc::NO_ENCLOSING_INDEX)
{
// No EH try index.
Expand Down

0 comments on commit bc50b97

Please sign in to comment.