Skip to content

Fixes for partial compilation of methods #80635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Mar 6, 2023
Merged

Fixes for partial compilation of methods #80635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
85716fa
Enable partial compilation of methods
AndyAyersMS Jan 13, 2023
d764041
Merge remote-tracking branch 'local0/main' into EnablePartialCompilation
AndyAyersMS Feb 3, 2023
3e91974
Merge branch 'main' into EnablePartialCompilation
AndyAyersMS Feb 6, 2023
940614c
Merge branch 'main' into EnablePartialCompilation
AndyAyersMS Feb 16, 2023
2df5068
Merge remote-tracking branch 'upstream/main' into EnablePartialCompil…
AndyAyersMS Feb 16, 2023
805d488
Merge remote-tracking branch 'upstream/main' into EnablePartialCompil…
AndyAyersMS Feb 23, 2023
e961715
don't block GC while waiting for PC continuation to jit
AndyAyersMS Feb 24, 2023
18283dc
review feedback
AndyAyersMS Feb 24, 2023
52f729f
revise to fix contract violations
AndyAyersMS Feb 25, 2023
1a7791f
reorder endforbidgc and setlasterror
AndyAyersMS Feb 25, 2023
7ff6ed7
fix flow cleanup at partial compilation point
AndyAyersMS Feb 26, 2023
1be7a6f
Merge remote-tracking branch 'upstream/main' into EnablePartialCompil…
AndyAyersMS Mar 4, 2023
2acc2d9
revert enabling by default
AndyAyersMS Mar 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6386,7 +6386,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)

// Block will no longer flow to any of its successors.
//
for (BasicBlock* const succ : block->Succs())
for (BasicBlock* const succ : block->Succs(this))
{
// We may have degenerate flow, make sure to fully remove
fgRemoveAllRefPreds(succ, block);
Expand Down
147 changes: 81 additions & 66 deletions src/coreclr/vm/jithelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4844,10 +4844,9 @@ HCIMPLEND
// Returns NULL if osr method can't be created.
static PCODE JitPatchpointWorker(MethodDesc* pMD, EECodeInfo& codeInfo, int ilOffset)
{
STANDARD_VM_CONTRACT;
PCODE osrVariant = NULL;

GCX_PREEMP();

// Fetch the patchpoint info for the current method
EEJitManager* jitMgr = ExecutionManager::GetEEJitManager();
CodeHeader* codeHdr = jitMgr->GetCodeHeaderFromStartAddress(codeInfo.GetStartAddress());
Expand Down Expand Up @@ -4895,6 +4894,7 @@ HCIMPL3(PCODE, JIT_Patchpoint_Framed, MethodDesc* pMD, EECodeInfo& codeInfo, int

HELPER_METHOD_FRAME_BEGIN_RET_0();

GCX_PREEMP();
result = JitPatchpointWorker(pMD, codeInfo, ilOffset);

HELPER_METHOD_FRAME_END();
Expand Down Expand Up @@ -5217,93 +5217,106 @@ void JIT_Patchpoint(int* counter, int ilOffset)
// was never jitted (eg an exceptional path).
//
// Unlike regular patchpoints, partial compilation patchpoints
// must always transitio.
// must always transition.
//
void JIT_PartialCompilationPatchpoint(int ilOffset)
HCIMPL1(VOID, JIT_PartialCompilationPatchpoint, int ilOffset)
Copy link
Member

@jkotas jkotas Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does JIT_Patchpoint need the same treatment?

Copy link
Member Author

@AndyAyersMS AndyAyersMS Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't loop, so (at least so far) I've never seen it cause problems.

If it does end up jitting a method, that thread switches to preemptive, and any other threads that hit the patchpoint don't need to wait, they just keep running the Tier0 code.

Copy link
Member

@jkotas jkotas Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the HELPER_METHOD_FRAME_BEGIN_0 setup need to be moved in front of EECodeInfo codeInfo(ip); for the same reason you had to move here?

{
FCALL_CONTRACT;

// BEGIN_PRESERVE_LAST_ERROR;
DWORD dwLastError = ::GetLastError();

// This method will not return normally
STATIC_CONTRACT_GC_NOTRIGGER;
STATIC_CONTRACT_MODE_COOPERATIVE;
PerPatchpointInfo* ppInfo = NULL;
bool isNewMethod = false;
CONTEXT frameContext;

// Patchpoint identity is the helper return address
PCODE ip = (PCODE)_ReturnAddress();

#if _DEBUG
// Friendly ID number
int ppId = 0;
#endif

HELPER_METHOD_FRAME_BEGIN_0();

// Fetch or setup patchpoint info for this patchpoint.
EECodeInfo codeInfo(ip);
MethodDesc* pMD = codeInfo.GetMethodDesc();
LoaderAllocator* allocator = pMD->GetLoaderAllocator();
OnStackReplacementManager* manager = allocator->GetOnStackReplacementManager();
PerPatchpointInfo * ppInfo = manager->GetPerPatchpointInfo(ip);
ppInfo = manager->GetPerPatchpointInfo(ip);

#if _DEBUG
const int ppId = ppInfo->m_patchpointId;
ppId = ppInfo->m_patchpointId;
#endif

// See if we have an OSR method for this patchpoint.
bool isNewMethod = false;
DWORD backoffs = 0;
while (ppInfo->m_osrMethodCode == NULL)

// Enable GC while we jit or wait for the continuation to be jitted.
{
// Invalid patchpoints are fatal, for partial compilation patchpoints
//
if ((ppInfo->m_flags & PerPatchpointInfo::patchpoint_invalid) == PerPatchpointInfo::patchpoint_invalid)
{
LOG((LF_TIEREDCOMPILATION, LL_FATALERROR, "Jit_PartialCompilationPatchpoint: invalid patchpoint [%d] (0x%p) in Method=0x%pM (%s::%s) at offset %d\n",
ppId, ip, pMD, pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, ilOffset));
EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);
}
GCX_PREEMP();

// Make sure no other thread is trying to create the OSR method.
//
LONG oldFlags = ppInfo->m_flags;
if ((oldFlags & PerPatchpointInfo::patchpoint_triggered) == PerPatchpointInfo::patchpoint_triggered)
while (ppInfo->m_osrMethodCode == NULL)
{
LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_PartialCompilationPatchpoint: AWAITING OSR method for patchpoint [%d] (0x%p)\n", ppId, ip));
__SwitchToThread(0, backoffs++);
continue;
}
// Invalid patchpoints are fatal, for partial compilation patchpoints
//
if ((ppInfo->m_flags & PerPatchpointInfo::patchpoint_invalid) == PerPatchpointInfo::patchpoint_invalid)
{
LOG((LF_TIEREDCOMPILATION, LL_FATALERROR, "Jit_PartialCompilationPatchpoint: invalid patchpoint [%d] (0x%p) in Method=0x%pM (%s::%s) at offset %d\n",
ppId, ip, pMD, pMD->m_pszDebugClassName, pMD->m_pszDebugMethodName, ilOffset));
EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);
}

// Make sure we win the race to create the OSR method
//
LONG newFlags = ppInfo->m_flags | PerPatchpointInfo::patchpoint_triggered;
BOOL triggerTransition = InterlockedCompareExchange(&ppInfo->m_flags, newFlags, oldFlags) == oldFlags;
// Make sure no other thread is trying to create the OSR method.
//
LONG oldFlags = ppInfo->m_flags;
if ((oldFlags & PerPatchpointInfo::patchpoint_triggered) == PerPatchpointInfo::patchpoint_triggered)
{
LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_PartialCompilationPatchpoint: AWAITING OSR method for patchpoint [%d] (0x%p)\n", ppId, ip));
__SwitchToThread(0, backoffs++);
continue;
}

if (!triggerTransition)
{
LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_PartialCompilationPatchpoint: (lost race) AWAITING OSR method for patchpoint [%d] (0x%p)\n", ppId, ip));
__SwitchToThread(0, backoffs++);
continue;
}
// Make sure we win the race to create the OSR method
//
LONG newFlags = ppInfo->m_flags | PerPatchpointInfo::patchpoint_triggered;
BOOL triggerTransition = InterlockedCompareExchange(&ppInfo->m_flags, newFlags, oldFlags) == oldFlags;

// Invoke the helper to build the OSR method
//
// TODO: may not want to optimize this part of the method, if it's truly partial compilation
// and can't possibly rejoin into the main flow.
//
// (but consider: throw path in method with try/catch, OSR method will contain more than just the throw?)
//
LOG((LF_TIEREDCOMPILATION, LL_INFO10, "Jit_PartialCompilationPatchpoint: patchpoint [%d] (0x%p) TRIGGER\n", ppId, ip));
PCODE newMethodCode = HCCALL3(JIT_Patchpoint_Framed, pMD, codeInfo, ilOffset);
if (!triggerTransition)
{
LOG((LF_TIEREDCOMPILATION, LL_INFO1000, "Jit_PartialCompilationPatchpoint: (lost race) AWAITING OSR method for patchpoint [%d] (0x%p)\n", ppId, ip));
__SwitchToThread(0, backoffs++);
continue;
}

// If that failed, mark the patchpoint as invalid.
// This is fatal, for partial compilation patchpoints
//
if (newMethodCode == NULL)
{
STRESS_LOG3(LF_TIEREDCOMPILATION, LL_WARNING, "Jit_PartialCompilationPatchpoint: patchpoint (0x%p) OSR method creation failed,"
" marking patchpoint invalid for Method=0x%pM il offset %d\n", ip, pMD, ilOffset);
InterlockedOr(&ppInfo->m_flags, (LONG)PerPatchpointInfo::patchpoint_invalid);
EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);
break;
}
// Invoke the helper to build the OSR method
//
// TODO: may not want to optimize this part of the method, if it's truly partial compilation
// and can't possibly rejoin into the main flow.
//
// (but consider: throw path in method with try/catch, OSR method will contain more than just the throw?)
//
LOG((LF_TIEREDCOMPILATION, LL_INFO10, "Jit_PartialCompilationPatchpoint: patchpoint [%d] (0x%p) TRIGGER\n", ppId, ip));
PCODE newMethodCode = JitPatchpointWorker(pMD, codeInfo, ilOffset);
Copy link
Member

@jkotas jkotas Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to also add STANDARD_VM_CONTRACT; annotation to JitPatchpointWorker, and make callers switch to preemptive mode before calling it.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.


// If that failed, mark the patchpoint as invalid.
// This is fatal, for partial compilation patchpoints
//
if (newMethodCode == NULL)
{
STRESS_LOG3(LF_TIEREDCOMPILATION, LL_WARNING, "Jit_PartialCompilationPatchpoint: patchpoint (0x%p) OSR method creation failed,"
" marking patchpoint invalid for Method=0x%pM il offset %d\n", ip, pMD, ilOffset);
InterlockedOr(&ppInfo->m_flags, (LONG)PerPatchpointInfo::patchpoint_invalid);
EEPOLICY_HANDLE_FATAL_ERROR(COR_E_EXECUTIONENGINE);
break;
}

// We've successfully created the osr method; make it available.
_ASSERTE(ppInfo->m_osrMethodCode == NULL);
ppInfo->m_osrMethodCode = newMethodCode;
isNewMethod = true;
// We've successfully created the osr method; make it available.
_ASSERTE(ppInfo->m_osrMethodCode == NULL);
ppInfo->m_osrMethodCode = newMethodCode;
isNewMethod = true;
}
}

// If we get here, we have code to transition to...
Expand All @@ -5320,7 +5333,6 @@ void JIT_PartialCompilationPatchpoint(int ilOffset)
#endif

// Find context for the original method
CONTEXT frameContext;
frameContext.ContextFlags = CONTEXT_FULL;
RtlCaptureContext(&frameContext);

Expand Down Expand Up @@ -5375,13 +5387,15 @@ void JIT_PartialCompilationPatchpoint(int ilOffset)
// Install new entry point as IP
SetIP(&frameContext, osrMethodCode);

// Restore last error (since call below does not return)
// END_PRESERVE_LAST_ERROR;
// This method doesn't return normally so we have to manually restore things.
HELPER_METHOD_FRAME_END();
ENDFORBIDGC();
Copy link
Member

@jkotas jkotas Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENDFORBIDGC may need to be before SetLastError.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Feb 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -- will reorder these.

::SetLastError(dwLastError);

// Transition!
RtlRestoreContext(&frameContext, NULL);
}
HCIMPLEND

#else

Expand All @@ -5394,14 +5408,15 @@ void JIT_Patchpoint(int* counter, int ilOffset)
UNREACHABLE();
}

void JIT_PartialCompilationPatchpoint(int* counter, int ilOffset)
HCIMPL1(VOID, JIT_PartialCompilationPatchpoint, int ilOffset)
{
// Stub version if OSR feature is disabled
//
// Should not be called.

UNREACHABLE();
}
HCIMPLEND

#endif // FEATURE_ON_STACK_REPLACEMENT

Expand Down