Skip to content

Virtual stub indirect call profiling #116453

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
AndyAyersMS merged 13 commits into from
Jun 17, 2025
Merged

Virtual stub indirect call profiling #116453

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
23f432c
initial workup, can profile but can't use info to devirtualize
AndyAyersMS Feb 11, 2025
5d20e83
experimenting
AndyAyersMS Jun 1, 2025
870bc65
use interface search to find matching instantiated interface
AndyAyersMS Jun 9, 2025
7dc72f0
clean
AndyAyersMS Jun 9, 2025
b36537a
add extra checks to devirt; revert unneded jit changes
AndyAyersMS Jun 10, 2025
a7e6159
format
AndyAyersMS Jun 10, 2025
7f05b81
fixes
AndyAyersMS Jun 10, 2025
984e5db
check for shared base, not shared object
AndyAyersMS Jun 10, 2025
da1d9d3
fix IEnumerator<T> check in importer
AndyAyersMS Jun 11, 2025
ecde0e9
simplify the runtime side
AndyAyersMS Jun 13, 2025
efcf49f
Merge branch 'main' into VirtualStubIndirectCallProfiling
AndyAyersMS Jun 14, 2025
89bda64
review feedback
AndyAyersMS Jun 14, 2025
7ed3ba9
Update src/coreclr/jit/compiler.hpp
AndyAyersMS Jun 16, 2025
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/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11945,7 +11945,7 @@ class GenTreeVisitor

if (call->gtCallType == CT_INDIRECT)
{
if (call->gtCallCookie != nullptr)
if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub())
Copy link
Member

@jakobbotsch jakobbotsch Jun 14, 2025

Choose a reason for hiding this comment

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

If I understand correctly, the active member of the union is not gtCallCookie when call->IsVirtualStub(), but rather either gtInlineCandidateInfo or gtHandleHistogramProfileCandidateInfo. If so that makes it undefined behavior to access gtCallCookie in these cases.

I think all the call->IsVirtualStub() checks need to be first to avoid this UB.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Jun 14, 2025

Choose a reason for hiding this comment

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

Yep, revised all these.

{
result = WalkTree(&call->gtCallCookie, call);
if (result == fgWalkResult::WALK_ABORT)
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4671,7 +4671,8 @@ void GenTree::VisitOperands(TVisitor visitor)

if (call->gtCallType == CT_INDIRECT)
{
if ((call->gtCallCookie != nullptr) && (visitor(call->gtCallCookie) == VisitResult::Abort))
if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub() &&
(visitor(call->gtCallCookie) == VisitResult::Abort))
{
return;
}
Expand Down
26 changes: 20 additions & 6 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5977,7 +5977,9 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
if (call->gtCallType == CT_INDIRECT)
{
// pinvoke-calli cookie is a constant, or constant indirection
assert(call->gtCallCookie == nullptr || call->gtCallCookie->OperIs(GT_CNS_INT, GT_IND));
// or a non-tree if this is a managed call.
assert(call->gtCallCookie == nullptr || call->gtCallCookie->OperIs(GT_CNS_INT, GT_IND) ||
call->IsVirtualStub());
Copy link
Member

@jakobbotsch jakobbotsch Jun 14, 2025

Choose a reason for hiding this comment

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

This one seems especially problematic, aren't we going to be treating a HandleHistogramProfileCandidateInfo* or InlineCandidateInfo* as a GenTree* and calling OperIs on it?


GenTree* indirect = call->gtCallAddr;

Expand Down Expand Up @@ -6725,7 +6727,7 @@ bool GenTree::TryGetUse(GenTree* operand, GenTree*** pUse)
}
if (call->gtCallType == CT_INDIRECT)
{
if (operand == call->gtCallCookie)
if ((operand == call->gtCallCookie) && !call->IsVirtualStub())
{
*pUse = &call->gtCallCookie;
return true;
Expand Down Expand Up @@ -9871,16 +9873,23 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree)
/* Copy the union */
if (tree->gtCallType == CT_INDIRECT)
{
copy->gtCallCookie = tree->gtCallCookie ? gtCloneExpr(tree->gtCallCookie) : nullptr;
copy->gtCallAddr = tree->gtCallAddr ? gtCloneExpr(tree->gtCallAddr) : nullptr;
if (tree->IsVirtualStub())
{
copy->gtCallCookie = tree->gtCallCookie;
}
else
{
copy->gtCallCookie = tree->gtCallCookie ? gtCloneExpr(tree->gtCallCookie) : nullptr;
}
copy->gtCallAddr = tree->gtCallAddr ? gtCloneExpr(tree->gtCallAddr) : nullptr;
}
else
{
copy->gtCallMethHnd = tree->gtCallMethHnd;
copy->gtInlineCandidateInfo = tree->gtInlineCandidateInfo;
copy->gtInlineInfoCount = tree->gtInlineInfoCount;
}

copy->gtInlineInfoCount = tree->gtInlineInfoCount;
copy->gtLateDevirtualizationInfo = tree->gtLateDevirtualizationInfo;

copy->gtCallType = tree->gtCallType;
Expand Down Expand Up @@ -10613,7 +10622,7 @@ void GenTreeUseEdgeIterator::AdvanceCall()
assert(call->gtCallType == CT_INDIRECT);

m_advance = &GenTreeUseEdgeIterator::AdvanceCall<CALL_ADDRESS>;
if (call->gtCallCookie != nullptr)
if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub())
{
m_edge = &call->gtCallCookie;
return;
Expand Down Expand Up @@ -10936,6 +10945,11 @@ void Compiler::gtDispNodeName(GenTree* tree)
}
else if (tree->AsCall()->gtCallType == CT_INDIRECT)
{
if (tree->AsCall()->IsVirtual())
{
callType = "CALLV";
}

ctType = " ind";
}
else
Expand Down
16 changes: 14 additions & 2 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6928,8 +6928,20 @@ void Compiler::impImportBlockCode(BasicBlock* block)
{
JITDUMP(".... checking for GDV of IEnumerable<T>...\n");

GenTreeCall* const call = op1->AsRetExpr()->gtInlineCandidate;
NamedIntrinsic const ni = lookupNamedIntrinsic(call->gtCallMethHnd);
GenTreeCall* const call = op1->AsRetExpr()->gtInlineCandidate;
NamedIntrinsic ni = NI_Illegal;

// TODO -- handle CT_INDIRECT virtuals here too
// but we don't have the right method handle
//
if (call->gtCallType == CT_USER_FUNC)
{
ni = lookupNamedIntrinsic(call->gtCallMethHnd);
}
else if (call->IsGuardedDevirtualizationCandidate())
{
JITDUMP("No GDV IEnumerable<T> check for [%06u]\n", dspTreeID(call));
}

if (ni == NI_System_Collections_Generic_IEnumerable_GetEnumerator)
{
Expand Down
26 changes: 15 additions & 11 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7292,7 +7292,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call,

if (!canResolve)
{
JITDUMP("Can't figure out which method would be invoked, sorry\n");
JITDUMP("Can't figure out which method would be invoked, sorry. [%s]\n",
devirtualizationDetailToString(dvInfo.detail));

// Continue checking other candidates, maybe some of them will succeed.
break;
Expand Down Expand Up @@ -7711,20 +7712,28 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call,
}
}

/* Ignore helper calls */

// Ignore helper calls
//
if (call->IsHelperCall())
{
assert(!call->IsGuardedDevirtualizationCandidate());
inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_CALL_TO_HELPER);
return;
}

/* Ignore indirect calls */
// Ignore indirect calls, unless they are indirect virtual stub calls with profile info.
//
if (call->gtCallType == CT_INDIRECT)
{
inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT_MANAGED);
return;
if (!call->IsGuardedDevirtualizationCandidate())
{
inlineResult->NoteFatal(InlineObservation::CALLSITE_IS_NOT_DIRECT_MANAGED);
return;
}
else
{
assert(call->IsVirtualStub());
}
}

// The inliner gets confused when the unmanaged convention reverses arg order (like x86).
Expand Down Expand Up @@ -8942,11 +8951,6 @@ bool Compiler::impConsiderCallProbe(GenTreeCall* call, IL_OFFSET ilOffset)
//
Compiler::GDVProbeType Compiler::compClassifyGDVProbeType(GenTreeCall* call)
{
if (call->gtCallType == CT_INDIRECT)
{
return GDVProbeType::None;
}

if (!opts.jitFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR) || IsAot())
{
return GDVProbeType::None;
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/jit/indirectcalltransformer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,6 @@ class IndirectCallTransformer

virtual void ClearFlag()
{
// We remove the GDV flag from the call in the CreateElse
}

virtual UINT8 GetChecksCount()
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/inline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1345,7 +1345,7 @@ InlineContext* InlineStrategy::NewContext(InlineContext* parentContext, Statemen
// ldarg instruction.
context->m_Location = stmt->GetDebugInfo().GetLocation();

assert(call->gtCallType == CT_USER_FUNC);
// assert(call->gtCallType == CT_USER_FUNC);
Copy link
Member

@jakobbotsch jakobbotsch Jun 14, 2025

Choose a reason for hiding this comment

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

Delete this?

context->m_Callee = call->gtCallMethHnd;

#if defined(DEBUG)
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1794,7 +1794,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call
// add as a non-standard arg.
}
}
else if (call->gtCallType == CT_INDIRECT && (call->gtCallCookie != nullptr))
else if (call->gtCallType == CT_INDIRECT && (call->gtCallCookie != nullptr) && !call->IsVirtualStub())
{
assert(!call->IsUnmanaged());

Expand Down Expand Up @@ -2102,6 +2102,8 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call)
continue;
}

assert(!argx->OperIs(GT_NONE));
Copy link
Member

@jakobbotsch jakobbotsch Jun 14, 2025

Choose a reason for hiding this comment

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

Seems like an odd assert to have in this specific place.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Jun 14, 2025

Choose a reason for hiding this comment

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

Ah, this was a leftover from tracking down a missing check for a valid cookie. Removed.


argx = fgMorphTree(argx);
*parentArgx = argx;

Expand Down
43 changes: 34 additions & 9 deletions src/coreclr/vm/jitinterface.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8629,7 +8629,7 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info)
return false;
}
}
else if (!pObjMT->CanCastToInterface(pBaseMT))
else if (!pBaseMT->IsSharedByGenericInstantiations() && !pObjMT->CanCastToInterface(pBaseMT))
{
info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST;
return false;
Expand All @@ -8639,32 +8639,57 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info)
// safely devirtualize.
if (info->context != nullptr)
{
// If the derived class is a shared class, make sure the
// owner class is too.
if (pObjMT->IsSharedByGenericInstantiations())
MethodTable* interfaceMT = nullptr;

if (pObjMT->IsSharedByGenericInstantiations() || pBaseMT->IsSharedByGenericInstantiations())
{
MethodTable* pCanonBaseMT = pBaseMT->GetCanonicalMethodTable();

// Check to see if the derived class implements multiple variants of a matching interface.
// If so, we cannot predict exactly which implementation is in use here.
MethodTable::InterfaceMapIterator it = pObjMT->IterateInterfaceMap();
int canonicallyMatchingInterfacesFound = 0;
MethodTable* interfaceMT = nullptr;
while (it.Next())
{
if (it.GetInterface(pObjMT)->GetCanonicalMethodTable() == pCanonBaseMT)
MethodTable* mt = it.GetInterface(pObjMT);
if (mt->GetCanonicalMethodTable() == pCanonBaseMT)
{
interfaceMT = mt;
canonicallyMatchingInterfacesFound++;

if (canonicallyMatchingInterfacesFound > 1)
{
// Multiple canonically identical interfaces found when attempting to devirtualize an inexact interface dispatch
// Multiple canonically identical interfaces found.
//
info->detail = CORINFO_DEVIRTUALIZATION_MULTIPLE_IMPL;
return false;
}
}
}
}

if (canonicallyMatchingInterfacesFound == 0)
{
// The object doesn't implement the interface...
//
info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST;
return false;
}

pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(pBaseMT), pBaseMD, FALSE /* throwOnConflict */);
MethodDesc* interfaceMD = interfaceMT->GetParallelMethodDesc(pBaseMD);
Copy link
Member

@davidwrighton davidwrighton Jun 11, 2025

Choose a reason for hiding this comment

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

What are the conditions which made you do the parallel methoddesc lookup for interfaceMD instead of just using pBaseMD?

Copy link
Member Author

@AndyAyersMS AndyAyersMS Jun 11, 2025

Choose a reason for hiding this comment

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

This may not answer your question, but this sort of lookup would fail

impDevirtualizeCall: Trying to devirtualize interface call:
    class for 'this' is System.Collections.Generic.IEqualityComparer`1[System.__Canon] (attrib 20220400)
    base method is System.Collections.Generic.IEqualityComparer`1[System.__Canon]::Equals
Considering guarded devirtualization at IL offset 3 (0x3)
Likely classes for call [000010] on class 00007FF819435700 (System.Collections.Generic.IEqualityComparer`1[System.__Canon])
  1) 00007FF819518608 (System.Collections.Generic.StringEqualityComparer) [likelihood:100%]
Accepting type System.Collections.Generic.StringEqualityComparer with likelihood 100 as a candidate
GDV likely: resolveVirtualMethod (method 00007FF819435660 class 00007FF819518608 context 00007FF819435701)

First because the old code's else if (!pObjMT->CanCastToInterface(pBaseMT)) would report a failed cast, and bypassing that (as this PR does), the subsequent GetMethodDescForInterfaceMethod would fail.

I don't think I tried

pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(interfaceMT), pBaseMD, FALSE /* throwOnConflict */);

if that's what you are suggesting.

Copy link
Member Author

@AndyAyersMS AndyAyersMS Jun 13, 2025

Choose a reason for hiding this comment

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

Yeah it looks like the parallel projection isn't needed. Will push a PR to remove that.


if (interfaceMD == nullptr)
{
info->detail = CORINFO_DEVIRTUALIZATION_FAILED_LOOKUP;
return false;
}

pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(interfaceMT), interfaceMD, FALSE /* throwOnConflict */);
}
else
{
pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(pBaseMT), pBaseMD, FALSE /* throwOnConflict */);
}
}
else if (!pBaseMD->HasClassOrMethodInstantiation())
{
Expand Down
Loading