From 23f432c362a3ee9f58992913bdc44f717b991af1 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 11 Feb 2025 15:29:29 -0800 Subject: [PATCH 01/12] initial workup, can profile but can't use info to devirtualize --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/compiler.hpp | 2 +- src/coreclr/jit/gentree.cpp | 16 +++++++++---- src/coreclr/jit/importercalls.cpp | 25 ++++++++++++--------- src/coreclr/jit/indirectcalltransformer.cpp | 9 ++++++++ 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 2e2bf632fa9996..5c099a6cae8fb1 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -11945,7 +11945,7 @@ class GenTreeVisitor if (call->gtCallType == CT_INDIRECT) { - if (call->gtCallCookie != nullptr) + if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub()) { result = WalkTree(&call->gtCallCookie, call); if (result == fgWalkResult::WALK_ABORT) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 5c38f1bfa9a5d1..27693fdeb0b862 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4671,7 +4671,7 @@ 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; } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 085b1484e7d75b..4e07fe394053ea 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -5977,7 +5977,8 @@ 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()); GenTree* indirect = call->gtCallAddr; @@ -6725,7 +6726,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; @@ -9871,7 +9872,14 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree) /* Copy the union */ if (tree->gtCallType == CT_INDIRECT) { - copy->gtCallCookie = tree->gtCallCookie ? gtCloneExpr(tree->gtCallCookie) : 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 @@ -10613,7 +10621,7 @@ void GenTreeUseEdgeIterator::AdvanceCall() assert(call->gtCallType == CT_INDIRECT); m_advance = &GenTreeUseEdgeIterator::AdvanceCall; - if (call->gtCallCookie != nullptr) + if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub()) { m_edge = &call->gtCallCookie; return; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 588679a612fa92..ea1363c9c88f4a 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -326,6 +326,8 @@ var_types Compiler::impImportCall(OPCODE opcode, call = gtNewIndCallNode(stubAddr, callRetTyp, di); + // hack... we will need this later to try devirt... + call->AsCall()->gtStubCallStubAddr = methHnd; call->gtFlags |= GTF_EXCEPT | (stubAddr->gtFlags & GTF_GLOB_EFFECT); call->gtFlags |= GTF_CALL_VIRT_STUB; @@ -7711,8 +7713,8 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, } } - /* Ignore helper calls */ - + // Ignore helper calls + // if (call->IsHelperCall()) { assert(!call->IsGuardedDevirtualizationCandidate()); @@ -7720,11 +7722,19 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, 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). @@ -8942,11 +8952,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; diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 48c66692a42e8c..bbbfc552d45b28 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -506,6 +506,15 @@ class IndirectCallTransformer return; } + // Bail on CT_INDIRECT for now + // + if (origCall->gtCallType == CT_INDIRECT) + { + JITDUMP("*** %s Bailing on [%06u] -- can't handle CT_INDIRECT yet\n", Name(), compiler->dspTreeID(origCall)); + ClearFlag(); + return; + } + likelihood = origCall->GetGDVCandidateInfo(0)->likelihood; assert((likelihood >= 0) && (likelihood <= 100)); JITDUMP("Likelihood of correct guess is %u\n", likelihood); From 5d20e83033ab66a34a206d73564952d8affe0c14 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sun, 1 Jun 2025 08:33:25 -0700 Subject: [PATCH 02/12] experimenting --- src/coreclr/jit/importercalls.cpp | 3 ++- src/coreclr/vm/jitinterface.cpp | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ea1363c9c88f4a..dd6052b3094b72 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -7294,7 +7294,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; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 5817eca4883e20..c57915950ffbd9 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8629,6 +8629,34 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } } + else if (pBaseMT->IsSharedByGenericInstantiations() && !pObjMT->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; + while (it.Next()) + { + if (it.GetInterface(pObjMT)->GetCanonicalMethodTable() == pCanonBaseMT) + { + canonicallyMatchingInterfacesFound++; + if (canonicallyMatchingInterfacesFound > 1) + { + // Multiple canonically identical interfaces found when attempting to devirtualize an inexact interface dispatch + info->detail = CORINFO_DEVIRTUALIZATION_MULTIPLE_IMPL; + return false; + } + } + } + + if (canonicallyMatchingInterfacesFound == 0) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; + return false; + } + } else if (!pObjMT->CanCastToInterface(pBaseMT)) { info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; From 870bc650bb71d68078922462be7bd2d2703833d3 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 9 Jun 2025 15:35:44 -0700 Subject: [PATCH 03/12] use interface search to find matching instantiated interface --- src/coreclr/jit/compiler.hpp | 3 +- src/coreclr/jit/gentree.cpp | 4 +- src/coreclr/jit/importercalls.cpp | 2 +- src/coreclr/jit/indirectcalltransformer.cpp | 13 +--- src/coreclr/jit/inline.cpp | 2 +- src/coreclr/vm/jitinterface.cpp | 72 +++++++++------------ 6 files changed, 38 insertions(+), 58 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 27693fdeb0b862..7cd5bf5a9025f6 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4671,7 +4671,8 @@ void GenTree::VisitOperands(TVisitor visitor) if (call->gtCallType == CT_INDIRECT) { - if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub() && (visitor(call->gtCallCookie) == VisitResult::Abort)) + if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub() && + (visitor(call->gtCallCookie) == VisitResult::Abort)) { return; } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 4e07fe394053ea..54899d21aaa36b 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -9880,15 +9880,15 @@ GenTreeCall* Compiler::gtCloneExprCallHelper(GenTreeCall* tree) { copy->gtCallCookie = tree->gtCallCookie ? gtCloneExpr(tree->gtCallCookie) : nullptr; } - copy->gtCallAddr = tree->gtCallAddr ? gtCloneExpr(tree->gtCallAddr) : 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; diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index dd6052b3094b72..33f543ba658eb9 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -7295,7 +7295,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, if (!canResolve) { JITDUMP("Can't figure out which method would be invoked, sorry. [%s]\n", - devirtualizationDetailToString(dvInfo.detail)); + devirtualizationDetailToString(dvInfo.detail)); // Continue checking other candidates, maybe some of them will succeed. break; diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index bbbfc552d45b28..ca4bfb8a24aeeb 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -502,16 +502,7 @@ class IndirectCallTransformer if (!origCall->IsInlineCandidate()) { JITDUMP("*** %s Bailing on [%06u] -- not an inline candidate\n", Name(), compiler->dspTreeID(origCall)); - ClearFlag(); - return; - } - - // Bail on CT_INDIRECT for now - // - if (origCall->gtCallType == CT_INDIRECT) - { - JITDUMP("*** %s Bailing on [%06u] -- can't handle CT_INDIRECT yet\n", Name(), compiler->dspTreeID(origCall)); - ClearFlag(); + origCall->ClearGuardedDevirtualizationCandidate(); return; } @@ -580,7 +571,6 @@ class IndirectCallTransformer virtual void ClearFlag() { - // We remove the GDV flag from the call in the CreateElse } virtual UINT8 GetChecksCount() @@ -1056,7 +1046,6 @@ class IndirectCallTransformer JITDUMP("Devirtualization was unable to use the unboxed entry; so marking call (to boxed entry) as not " "inlineable\n"); - call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; call->ClearInlineInfo(); if (returnTemp != BAD_VAR_NUM) diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 7dfa4ba474ab30..89e05f7f2a5af9 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -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); context->m_Callee = call->gtCallMethHnd; #if defined(DEBUG) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index c57915950ffbd9..42473f1c201ba8 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8629,70 +8629,60 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } } - else if (pBaseMT->IsSharedByGenericInstantiations() && !pObjMT->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; - while (it.Next()) - { - if (it.GetInterface(pObjMT)->GetCanonicalMethodTable() == pCanonBaseMT) - { - canonicallyMatchingInterfacesFound++; - if (canonicallyMatchingInterfacesFound > 1) - { - // Multiple canonically identical interfaces found when attempting to devirtualize an inexact interface dispatch - info->detail = CORINFO_DEVIRTUALIZATION_MULTIPLE_IMPL; - return false; - } - } - } - - if (canonicallyMatchingInterfacesFound == 0) - { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; - return false; - } - } - else if (!pObjMT->CanCastToInterface(pBaseMT)) - { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; - return false; - } // For generic interface methods we must have context to // safely devirtualize. if (info->context != nullptr) { - // If the derived class is a shared class, make sure the - // owner class is too. - if (pObjMT->IsSharedByGenericInstantiations()) + if (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; + } + + MethodDesc* interfaceMD = interfaceMT->GetParallelMethodDesc(pBaseMD); - pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(pBaseMT), pBaseMD, FALSE /* throwOnConflict */); + 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()) { From 7dc72f084c47d48c85ca87f17a81f653a5c33474 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 9 Jun 2025 15:47:55 -0700 Subject: [PATCH 04/12] clean --- src/coreclr/jit/importercalls.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 33f543ba658eb9..54d004652036c3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -326,8 +326,6 @@ var_types Compiler::impImportCall(OPCODE opcode, call = gtNewIndCallNode(stubAddr, callRetTyp, di); - // hack... we will need this later to try devirt... - call->AsCall()->gtStubCallStubAddr = methHnd; call->gtFlags |= GTF_EXCEPT | (stubAddr->gtFlags & GTF_GLOB_EFFECT); call->gtFlags |= GTF_CALL_VIRT_STUB; From b36537aa3c06914be9ff74477c2700f2f70f013e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 9 Jun 2025 18:48:19 -0700 Subject: [PATCH 05/12] add extra checks to devirt; revert unneded jit changes --- src/coreclr/jit/indirectcalltransformer.cpp | 3 ++- src/coreclr/vm/jitinterface.cpp | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index ca4bfb8a24aeeb..138d47d5b76e99 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -502,7 +502,7 @@ class IndirectCallTransformer if (!origCall->IsInlineCandidate()) { JITDUMP("*** %s Bailing on [%06u] -- not an inline candidate\n", Name(), compiler->dspTreeID(origCall)); - origCall->ClearGuardedDevirtualizationCandidate(); + ClearFlag(); return; } @@ -1046,6 +1046,7 @@ class IndirectCallTransformer JITDUMP("Devirtualization was unable to use the unboxed entry; so marking call (to boxed entry) as not " "inlineable\n"); + call->gtFlags &= ~GTF_CALL_INLINE_CANDIDATE; call->ClearInlineInfo(); if (returnTemp != BAD_VAR_NUM) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 42473f1c201ba8..b809737b40697a 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8634,7 +8634,9 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) // safely devirtualize. if (info->context != nullptr) { - if (pBaseMT->IsSharedByGenericInstantiations()) + MethodTable* interfaceMT = nullptr; + + if (pObjMT->IsSharedByGenericInstantiations() || pBaseMT->IsSharedByGenericInstantiations()) { MethodTable* pCanonBaseMT = pBaseMT->GetCanonicalMethodTable(); @@ -8668,6 +8670,17 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; return false; } + } + + if (interfaceMT != nullptr) + { + // why do we need this... + // + if (!pObjMT->IsArray() && !pObjMT->ImplementsEquivalentInterface(interfaceMT)) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_LOOKUP; + return false; + } MethodDesc* interfaceMD = interfaceMT->GetParallelMethodDesc(pBaseMD); From a7e6159044a745fef2aca7a4de2c086716db017c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 9 Jun 2025 19:01:00 -0700 Subject: [PATCH 06/12] format --- src/coreclr/jit/gentree.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 54899d21aaa36b..a8d1998e2ccc30 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -5978,7 +5978,8 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) { // pinvoke-calli cookie is a constant, or constant indirection // or a non-tree if this is a managed call. - assert(call->gtCallCookie == nullptr || call->gtCallCookie->OperIs(GT_CNS_INT, GT_IND) || call->IsVirtualStub()); + assert(call->gtCallCookie == nullptr || call->gtCallCookie->OperIs(GT_CNS_INT, GT_IND) || + call->IsVirtualStub()); GenTree* indirect = call->gtCallAddr; From 7f05b8172fbb4abdbcae6f824a97f90272a2793b Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 10 Jun 2025 11:56:45 -0700 Subject: [PATCH 07/12] fixes --- src/coreclr/jit/gentree.cpp | 5 +++++ src/coreclr/jit/morph.cpp | 4 +++- src/coreclr/vm/jitinterface.cpp | 20 +++++++------------- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index a8d1998e2ccc30..51e5fbcc569f9e 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10945,6 +10945,11 @@ void Compiler::gtDispNodeName(GenTree* tree) } else if (tree->AsCall()->gtCallType == CT_INDIRECT) { + if (tree->AsCall()->IsVirtual()) + { + callType = "CALLV"; + } + ctType = " ind"; } else diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index fc9dd838c24161..b3f95e0fae9bf3 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -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()); @@ -2102,6 +2102,8 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) continue; } + assert(!argx->OperIs(GT_NONE)); + argx = fgMorphTree(argx); *parentArgx = argx; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index b809737b40697a..b5706f46cfc262 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8629,6 +8629,11 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } } + else if (!pObjMT->IsSharedByGenericInstantiations() && !pObjMT->CanCastToInterface(pBaseMT)) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; + return false; + } // For generic interface methods we must have context to // safely devirtualize. @@ -8670,26 +8675,15 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; return false; } - } - - if (interfaceMT != nullptr) - { - // why do we need this... - // - if (!pObjMT->IsArray() && !pObjMT->ImplementsEquivalentInterface(interfaceMT)) - { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_LOOKUP; - return false; - } MethodDesc* interfaceMD = interfaceMT->GetParallelMethodDesc(pBaseMD); - + if (interfaceMD == nullptr) { info->detail = CORINFO_DEVIRTUALIZATION_FAILED_LOOKUP; return false; } - + pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(interfaceMT), interfaceMD, FALSE /* throwOnConflict */); } else From 984e5db7904a683a4074be7262ae6812d23b8c38 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 10 Jun 2025 15:09:34 -0700 Subject: [PATCH 08/12] check for shared base, not shared object --- src/coreclr/vm/jitinterface.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index b5706f46cfc262..919133657b3a73 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8629,7 +8629,7 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } } - else if (!pObjMT->IsSharedByGenericInstantiations() && !pObjMT->CanCastToInterface(pBaseMT)) + else if (!pBaseMT->IsSharedByGenericInstantiations() && !pObjMT->CanCastToInterface(pBaseMT)) { info->detail = CORINFO_DEVIRTUALIZATION_FAILED_CAST; return false; From da1d9d362c8cb1f78addfa971f991998cf2948c1 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 10 Jun 2025 17:12:55 -0700 Subject: [PATCH 09/12] fix IEnumerator check in importer --- src/coreclr/jit/importer.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 2c46b32ba7ab5f..1a387cf6712590 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -6928,8 +6928,20 @@ void Compiler::impImportBlockCode(BasicBlock* block) { JITDUMP(".... checking for GDV of IEnumerable...\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 check for [%06u]\n", dspTreeID(call)); + } if (ni == NI_System_Collections_Generic_IEnumerable_GetEnumerator) { From ecde0e94fd46112a4d98ea217613569860732ba8 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 13 Jun 2025 10:04:59 -0700 Subject: [PATCH 10/12] simplify the runtime side --- src/coreclr/vm/jitinterface.cpp | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 919133657b3a73..6eb08a0075132b 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8676,15 +8676,7 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } - MethodDesc* interfaceMD = interfaceMT->GetParallelMethodDesc(pBaseMD); - - if (interfaceMD == nullptr) - { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_LOOKUP; - return false; - } - - pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(interfaceMT), interfaceMD, FALSE /* throwOnConflict */); + pDevirtMD = pObjMT->GetMethodDescForInterfaceMethod(TypeHandle(interfaceMT), pBaseMD, FALSE /* throwOnConflict */); } else { From 89bda64c8d6313e6c8174d0d15f57c7d48443fb0 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 14 Jun 2025 08:29:55 -0700 Subject: [PATCH 11/12] review feedback --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/gentree.cpp | 8 ++++---- src/coreclr/jit/indirectcalltransformer.cpp | 1 + src/coreclr/jit/inline.cpp | 4 +--- src/coreclr/jit/morph.cpp | 4 +--- 5 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 08fdac52febfaa..635dda470dd7e1 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -11888,7 +11888,7 @@ class GenTreeVisitor if (call->gtCallType == CT_INDIRECT) { - if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub()) + if (!call->IsVirtualStub() && (call->gtCallCookie != nullptr)) { result = WalkTree(&call->gtCallCookie, call); if (result == fgWalkResult::WALK_ABORT) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 24a6f665701fbb..3dc4a00604f61f 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -5978,8 +5978,8 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) { // pinvoke-calli cookie is a constant, or constant indirection // or a non-tree if this is a managed call. - assert(call->gtCallCookie == nullptr || call->gtCallCookie->OperIs(GT_CNS_INT, GT_IND) || - call->IsVirtualStub()); + assert(call->IsVirtualStub() || (call->gtCallCookie == nullptr) || + call->gtCallCookie->OperIs(GT_CNS_INT, GT_IND)); GenTree* indirect = call->gtCallAddr; @@ -6727,7 +6727,7 @@ bool GenTree::TryGetUse(GenTree* operand, GenTree*** pUse) } if (call->gtCallType == CT_INDIRECT) { - if ((operand == call->gtCallCookie) && !call->IsVirtualStub()) + if (!call->IsVirtualStub() && (operand == call->gtCallCookie)) { *pUse = &call->gtCallCookie; return true; @@ -10650,7 +10650,7 @@ void GenTreeUseEdgeIterator::AdvanceCall() assert(call->gtCallType == CT_INDIRECT); m_advance = &GenTreeUseEdgeIterator::AdvanceCall; - if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub()) + if (!call->IsVirtualStub() && (call->gtCallCookie != nullptr)) { m_edge = &call->gtCallCookie; return; diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 138d47d5b76e99..48c66692a42e8c 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -571,6 +571,7 @@ class IndirectCallTransformer virtual void ClearFlag() { + // We remove the GDV flag from the call in the CreateElse } virtual UINT8 GetChecksCount() diff --git a/src/coreclr/jit/inline.cpp b/src/coreclr/jit/inline.cpp index 89e05f7f2a5af9..7c862c8744d130 100644 --- a/src/coreclr/jit/inline.cpp +++ b/src/coreclr/jit/inline.cpp @@ -1344,9 +1344,7 @@ InlineContext* InlineStrategy::NewContext(InlineContext* parentContext, Statemen // which becomes a single statement where the IL location points to the // ldarg instruction. context->m_Location = stmt->GetDebugInfo().GetLocation(); - - // assert(call->gtCallType == CT_USER_FUNC); - context->m_Callee = call->gtCallMethHnd; + context->m_Callee = call->gtCallMethHnd; #if defined(DEBUG) context->m_Devirtualized = call->IsDevirtualized(); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 10fa41f169b0d3..29beafdb321e85 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1773,7 +1773,7 @@ void CallArgs::AddFinalArgsAndDetermineABIInfo(Compiler* comp, GenTreeCall* call // add as a non-standard arg. } } - else if (call->gtCallType == CT_INDIRECT && (call->gtCallCookie != nullptr) && !call->IsVirtualStub()) + else if ((call->gtCallType == CT_INDIRECT) && !call->IsVirtualStub() && (call->gtCallCookie != nullptr)) { assert(!call->IsUnmanaged()); @@ -2081,8 +2081,6 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) continue; } - assert(!argx->OperIs(GT_NONE)); - argx = fgMorphTree(argx); *parentArgx = argx; From 7ed3ba9679439a0af814a97a9d07eb78917bbbf7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 16 Jun 2025 07:32:48 -0700 Subject: [PATCH 12/12] Update src/coreclr/jit/compiler.hpp Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/compiler.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 6443cfed4c16c6..74c5e2baaf61c4 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4683,7 +4683,7 @@ void GenTree::VisitOperands(TVisitor visitor) if (call->gtCallType == CT_INDIRECT) { - if ((call->gtCallCookie != nullptr) && !call->IsVirtualStub() && + if (!call->IsVirtualStub() && (call->gtCallCookie != nullptr) && (visitor(call->gtCallCookie) == VisitResult::Abort)) { return;