From 81df1616c31cf700d5224711a514e77874f94a97 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 13 Jun 2023 02:04:16 +0200 Subject: [PATCH 01/19] Slightly faster atomic increment --- src/coreclr/jit/codegenxarch.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 8dea6b12769bba..75d8e5432c4ae8 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -4263,10 +4263,25 @@ void CodeGen::genCodeForLockAdd(GenTreeOp* node) { int imm = static_cast(data->AsIntCon()->IconValue()); assert(imm == data->AsIntCon()->IconValue()); - GetEmitter()->emitIns_I_AR(INS_add, size, imm, addr->GetRegNum(), 0); + if (imm == 1) + { + // inc [addr] + GetEmitter()->emitIns_AR(INS_inc, size, addr->GetRegNum(), 0); + } + else if (imm == -1) + { + // dec [addr] + GetEmitter()->emitIns_AR(INS_dec, size, addr->GetRegNum(), 0); + } + else + { + // add [addr], imm + GetEmitter()->emitIns_I_AR(INS_add, size, imm, addr->GetRegNum(), 0); + } } else { + // add [addr], data GetEmitter()->emitIns_AR_R(INS_add, size, data->GetRegNum(), addr->GetRegNum(), 0); } } From c8e0dd0983c736801a093e2c6e9dcbabe04ca832 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 21 Jun 2023 00:43:46 +0200 Subject: [PATCH 02/19] use proper context in GDV --- src/coreclr/jit/importercalls.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index eb67240252e6ae..bfc02302b15e04 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -7757,7 +7757,10 @@ void Compiler::impCheckCanInline(GenTreeCall* call, // Speculatively check if initClass() can be done. // If it can be done, we will try to inline the method. CorInfoInitClassResult const initClassResult = - compCompHnd->initClass(nullptr /* field */, ftn /* method */, pParam->exactContextHnd /* context */); + compCompHnd->initClass(nullptr /* field */, ftn /* method */, + pParam->call->IsGuardedDevirtualizationCandidate() + ? MAKE_METHODCONTEXT(ftn) + : pParam->exactContextHnd /* context */); if (initClassResult & CORINFO_INITCLASS_DONT_INLINE) { From bee352f004c00392fbb9aba34cdfc1b269fd4b41 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 21 Jun 2023 19:15:06 +0200 Subject: [PATCH 03/19] test --- src/coreclr/jit/compiler.h | 13 +++++----- src/coreclr/jit/importercalls.cpp | 43 +++++++++++++++++++------------ 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 0d24478ae115d8..b12b50921a6bf6 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7152,12 +7152,13 @@ class Compiler bool isCompatibleMethodGDV(GenTreeCall* call, CORINFO_METHOD_HANDLE gdvTarget); - void addGuardedDevirtualizationCandidate(GenTreeCall* call, - CORINFO_METHOD_HANDLE methodHandle, - CORINFO_CLASS_HANDLE classHandle, - unsigned methodAttr, - unsigned classAttr, - unsigned likelihood); + void addGuardedDevirtualizationCandidate(GenTreeCall* call, + CORINFO_METHOD_HANDLE methodHandle, + CORINFO_CLASS_HANDLE classHandle, + CORINFO_CONTEXT_HANDLE contextHandle, + unsigned methodAttr, + unsigned classAttr, + unsigned likelihood); int getGDVMaxTypeChecks() { diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index bfc02302b15e04..e7a33a465931ba 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5931,8 +5931,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, likelyHood += 100 - likelyHood * numExactClasses; } - addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, exactMethodAttrs, clsAttrs, - likelyHood); + addGuardedDevirtualizationCandidate(call, exactMethod, exactCls, dvInfo.exactContext, exactMethodAttrs, + clsAttrs, likelyHood); } if (call->GetInlineCandidatesCount() == numExactClasses) @@ -5959,6 +5959,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, CORINFO_METHOD_HANDLE likelyMethod = likelyMethodes[candidateId]; unsigned likelihood = likelihoods[candidateId]; + CORINFO_CONTEXT_HANDLE likelyContext = NULL; + uint32_t likelyClassAttribs = 0; if (likelyClass != NO_CLASS_HANDLE) { @@ -5993,8 +5995,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // Continue checking other candidates, maybe some of them will succeed. break; } - - likelyMethod = dvInfo.devirtualizedMethod; + likelyContext = dvInfo.exactContext; + likelyMethod = dvInfo.devirtualizedMethod; } uint32_t likelyMethodAttribs = info.compCompHnd->getMethodAttribs(likelyMethod); @@ -6062,8 +6064,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // Add this as a potential candidate. // - addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyMethodAttribs, likelyClassAttribs, - likelihood); + addGuardedDevirtualizationCandidate(call, likelyMethod, likelyClass, likelyContext, likelyMethodAttribs, + likelyClassAttribs, likelihood); } } @@ -6088,12 +6090,13 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // classAttr - attributes of the class // likelihood - odds that this class is the class seen at runtime // -void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, - CORINFO_METHOD_HANDLE methodHandle, - CORINFO_CLASS_HANDLE classHandle, - unsigned methodAttr, - unsigned classAttr, - unsigned likelihood) +void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, + CORINFO_METHOD_HANDLE methodHandle, + CORINFO_CLASS_HANDLE classHandle, + CORINFO_CONTEXT_HANDLE contextHandle, + unsigned methodAttr, + unsigned classAttr, + unsigned likelihood) { // This transformation only makes sense for delegate and virtual calls assert(call->IsDelegateInvoke() || call->IsVirtual()); @@ -6166,6 +6169,7 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, pInfo->guardedClassHandle = classHandle; pInfo->likelihood = likelihood; pInfo->requiresInstMethodTableArg = false; + pInfo->exactContextHnd = contextHandle; // If the guarded class is a value class, look for an unboxed entry point. // @@ -7754,13 +7758,20 @@ void Compiler::impCheckCanInline(GenTreeCall* call, return; } + CORINFO_CONTEXT_HANDLE exactContext = pParam->exactContextHnd; + if (pParam->call->IsGuardedDevirtualizationCandidate()) + { + auto tmp = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex)->exactContextHnd; + if (tmp != NULL) + { + exactContext = tmp; + } + } + // Speculatively check if initClass() can be done. // If it can be done, we will try to inline the method. CorInfoInitClassResult const initClassResult = - compCompHnd->initClass(nullptr /* field */, ftn /* method */, - pParam->call->IsGuardedDevirtualizationCandidate() - ? MAKE_METHODCONTEXT(ftn) - : pParam->exactContextHnd /* context */); + compCompHnd->initClass(nullptr /* field */, ftn /* method */, exactContext /* context */); if (initClassResult & CORINFO_INITCLASS_DONT_INLINE) { From fa14183c43f133dc95ac2dbeeb34f458cb18d790 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 22 Jun 2023 02:17:16 +0200 Subject: [PATCH 04/19] test2 --- .../tools/superpmi/superpmi-shared/methodcontext.cpp | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp index 8f11b2cb71c962..6878df4a77c8ef 100644 --- a/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp @@ -1015,12 +1015,6 @@ CorInfoInitClassResult MethodContext::repInitClass(CORINFO_FIELD_HANDLE field, key.method = CastHandle(method); key.context = CastHandle(context); - if ((InitClass == nullptr) || (InitClass->GetIndex(key) == -1)) - { - // We could try additional inlines with stress modes, just reject them. - return CORINFO_INITCLASS_DONT_INLINE; - } - DWORD value = InitClass->Get(key); DEBUG_REP(dmpInitClass(key, value)); CorInfoInitClassResult result = (CorInfoInitClassResult)value; From c35c337b23150e4317a091c0975554a917268e87 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 24 Jun 2023 23:35:50 +0200 Subject: [PATCH 05/19] Test fix --- src/coreclr/jit/importercalls.cpp | 6 +----- src/coreclr/vm/jitinterface.cpp | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index e7a33a465931ba..b173a7c737c105 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -7761,11 +7761,7 @@ void Compiler::impCheckCanInline(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContext = pParam->exactContextHnd; if (pParam->call->IsGuardedDevirtualizationCandidate()) { - auto tmp = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex)->exactContextHnd; - if (tmp != NULL) - { - exactContext = tmp; - } + exactContext = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex)->exactContextHnd; } // Speculatively check if initClass() can be done. diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 388b5156f4fbaf..6d2c5f027695ca 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8425,22 +8425,22 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } - if (pBaseMT->IsInterface()) - { - #ifdef FEATURE_COMINTEROP - // Don't try and devirtualize com interface calls. - if (pObjMT->IsComObjectType()) - { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; - return false; - } + // Don't try and devirtualize com interface calls. + if (pBaseMT->IsInterface() && pObjMT->IsComObjectType()) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; + return false; + } #endif // FEATURE_COMINTEROP - if (info->context != nullptr) - { - pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); - } + if (info->context != nullptr) + { + pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); + } + + if (pBaseMT->IsInterface()) + { // Interface call devirtualization. // From 67160598dcbed71f13dfc6c465fa6e8319f2ddb5 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Sun, 25 Jun 2023 02:02:44 +0200 Subject: [PATCH 06/19] Update jitinterface.cpp --- src/coreclr/vm/jitinterface.cpp | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 6d2c5f027695ca..30ca6909b0ef08 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8425,22 +8425,21 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } -#ifdef FEATURE_COMINTEROP - // Don't try and devirtualize com interface calls. - if (pBaseMT->IsInterface() && pObjMT->IsComObjectType()) + if (pBaseMT->IsInterface()) { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; - return false; - } +#ifdef FEATURE_COMINTEROP + // Don't try and devirtualize com interface calls. + if (pObjMT->IsComObjectType()) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; + return false; + } #endif // FEATURE_COMINTEROP - if (info->context != nullptr) - { - pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); - } - - if (pBaseMT->IsInterface()) - { + if (info->context != nullptr) + { + pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); + } // Interface call devirtualization. // From 7ef4563c380c4fd7ee9c9fe94fe1c90c563f9c59 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 25 Jun 2023 21:20:32 +0200 Subject: [PATCH 07/19] test 3 --- src/coreclr/jit/importercalls.cpp | 7 ++++++- src/coreclr/vm/jitinterface.cpp | 24 ++++++++++++------------ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b173a7c737c105..05501af65cbb42 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -7761,7 +7761,12 @@ void Compiler::impCheckCanInline(GenTreeCall* call, CORINFO_CONTEXT_HANDLE exactContext = pParam->exactContextHnd; if (pParam->call->IsGuardedDevirtualizationCandidate()) { - exactContext = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex)->exactContextHnd; + InlineCandidateInfo* candidate = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex); + CORINFO_CONTEXT_HANDLE moreExactContext = candidate->exactContextHnd; + if (moreExactContext != NULL) + { + exactContext = moreExactContext; + } } // Speculatively check if initClass() can be done. diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 2038142af4517c..86ed7a128f4fde 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8476,22 +8476,22 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } - if (pBaseMT->IsInterface()) - { #ifdef FEATURE_COMINTEROP - // Don't try and devirtualize com interface calls. - if (pObjMT->IsComObjectType()) - { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; - return false; - } + // Don't try and devirtualize com interface calls. + if (pBaseMT->IsInterface() && pObjMT->IsComObjectType()) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; + return false; + } #endif // FEATURE_COMINTEROP - if (info->context != nullptr) - { - pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); - } + if (info->context != nullptr) + { + pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); + } + if (pBaseMT->IsInterface()) + { // Interface call devirtualization. // // We must ensure that pObjMT actually implements the From 632efc9ffb5fac60f882958f69445d6bfa4883cb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 30 Jun 2023 02:21:51 +0200 Subject: [PATCH 08/19] Test --- src/coreclr/jit/importercalls.cpp | 20 ++++++++------------ src/coreclr/vm/jitinterface.cpp | 25 +++++++++++++------------ 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 0de6161abdf667..af4930d2cca01a 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6043,6 +6043,13 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, // Continue checking other candidates, maybe some of them will succeed. break; } + + if (*pContextHandle != dvInfo.exactContext) + { + JITDUMP("Exact context has changed - we don't yet support that, sorry\n"); + break; + } + likelyContext = dvInfo.exactContext; likelyMethod = dvInfo.devirtualizedMethod; } @@ -7806,21 +7813,10 @@ void Compiler::impCheckCanInline(GenTreeCall* call, return; } - CORINFO_CONTEXT_HANDLE exactContext = pParam->exactContextHnd; - if (pParam->call->IsGuardedDevirtualizationCandidate()) - { - InlineCandidateInfo* candidate = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex); - CORINFO_CONTEXT_HANDLE moreExactContext = candidate->exactContextHnd; - if (moreExactContext != NULL) - { - exactContext = moreExactContext; - } - } - // Speculatively check if initClass() can be done. // If it can be done, we will try to inline the method. CorInfoInitClassResult const initClassResult = - compCompHnd->initClass(nullptr /* field */, ftn /* method */, exactContext /* context */); + compCompHnd->initClass(nullptr /* field */, ftn /* method */, pParam->exactContextHnd /* context */); if (initClassResult & CORINFO_INITCLASS_DONT_INLINE) { diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 76b3322e405c9e..7151d257d4ead2 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -8538,22 +8538,23 @@ bool CEEInfo::resolveVirtualMethodHelper(CORINFO_DEVIRTUALIZATION_INFO * info) return false; } -#ifdef FEATURE_COMINTEROP - // Don't try and devirtualize com interface calls. - if (pBaseMT->IsInterface() && pObjMT->IsComObjectType()) + if (pBaseMT->IsInterface()) { - info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; - return false; - } + +#ifdef FEATURE_COMINTEROP + // Don't try and devirtualize com interface calls. + if (pObjMT->IsComObjectType()) + { + info->detail = CORINFO_DEVIRTUALIZATION_FAILED_COM; + return false; + } #endif // FEATURE_COMINTEROP - if (info->context != nullptr) - { - pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); - } + if (info->context != nullptr) + { + pBaseMT = GetTypeFromContext(info->context).GetMethodTable(); + } - if (pBaseMT->IsInterface()) - { // Interface call devirtualization. // // We must ensure that pObjMT actually implements the From 53475989a75bd3508695da337cda86853b4ae70a Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 30 Jun 2023 10:56:06 +0200 Subject: [PATCH 09/19] Update importercalls.cpp --- src/coreclr/jit/importercalls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index af4930d2cca01a..27c5fc7a61d376 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6044,7 +6044,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, break; } - if (*pContextHandle != dvInfo.exactContext) + if ((dvInfo.exactContext != NULL) && (*pContextHandle != dvInfo.exactContext)) { JITDUMP("Exact context has changed - we don't yet support that, sorry\n"); break; From 39478fa8a86fe730ea0224667fea3ab69b2ee4dc Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 30 Jun 2023 19:54:12 +0200 Subject: [PATCH 10/19] fix regressions --- src/coreclr/jit/importercalls.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 27c5fc7a61d376..aafb3181312362 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6044,7 +6044,8 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, break; } - if ((dvInfo.exactContext != NULL) && (*pContextHandle != dvInfo.exactContext)) + if ((dvInfo.exactContext != NULL) && (*pContextHandle != dvInfo.exactContext) && + (dvInfo.exactContext != MAKE_METHODCONTEXT(dvInfo.devirtualizedMethod))) { JITDUMP("Exact context has changed - we don't yet support that, sorry\n"); break; From 848d4ff8d8dc6ac56259038b2a4cfbcb8ba6fcef Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 30 Jun 2023 22:32:04 +0200 Subject: [PATCH 11/19] Update importercalls.cpp --- src/coreclr/jit/importercalls.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index aafb3181312362..6560af98f372c3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6045,6 +6045,7 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, } if ((dvInfo.exactContext != NULL) && (*pContextHandle != dvInfo.exactContext) && + (METHOD_BEING_COMPILED_CONTEXT() != dvInfo.exactContext) && (dvInfo.exactContext != MAKE_METHODCONTEXT(dvInfo.devirtualizedMethod))) { JITDUMP("Exact context has changed - we don't yet support that, sorry\n"); From 19b8e25d5dc358a79481b6fa4ea8a8c975e8f84c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 3 Jul 2023 02:35:32 +0200 Subject: [PATCH 12/19] revert --- src/coreclr/jit/importercalls.cpp | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 6560af98f372c3..3b498477dc1594 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6044,14 +6044,6 @@ void Compiler::considerGuardedDevirtualization(GenTreeCall* call, break; } - if ((dvInfo.exactContext != NULL) && (*pContextHandle != dvInfo.exactContext) && - (METHOD_BEING_COMPILED_CONTEXT() != dvInfo.exactContext) && - (dvInfo.exactContext != MAKE_METHODCONTEXT(dvInfo.devirtualizedMethod))) - { - JITDUMP("Exact context has changed - we don't yet support that, sorry\n"); - break; - } - likelyContext = dvInfo.exactContext; likelyMethod = dvInfo.devirtualizedMethod; } @@ -7815,10 +7807,26 @@ void Compiler::impCheckCanInline(GenTreeCall* call, return; } + CORINFO_CONTEXT_HANDLE exactContextHnd = pParam->exactContextHnd; + if (pParam->call->IsGuardedDevirtualizationCandidate()) + { + InlineCandidateInfo* candidateInfo = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex); + if (candidateInfo->exactContextHnd != nullptr) + { + // exactContextHnd represents the exact class the method is defined in. + exactContextHnd = candidateInfo->exactContextHnd; + } + else + { + // exactContextHnd can't be null for normal GDV class guesses + assert(candidateInfo->guardedClassHandle == nullptr); + } + } + // Speculatively check if initClass() can be done. // If it can be done, we will try to inline the method. CorInfoInitClassResult const initClassResult = - compCompHnd->initClass(nullptr /* field */, ftn /* method */, pParam->exactContextHnd /* context */); + compCompHnd->initClass(nullptr /* field */, ftn /* method */, exactContextHnd /* context */); if (initClassResult & CORINFO_INITCLASS_DONT_INLINE) { From 0d4fddbe4e91a98cb0374767b83be209af465519 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 15 Jul 2023 02:07:42 +0200 Subject: [PATCH 13/19] test fix --- src/coreclr/jit/fginline.cpp | 3 ++- src/coreclr/jit/importercalls.cpp | 26 +++++++-------------- src/coreclr/jit/indirectcalltransformer.cpp | 5 ++-- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 82c3a3328b93f2..1d9e3f29630ab2 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -475,7 +475,8 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitorimpDevirtualizeCall(call, nullptr, &method, &methodFlags, &context, nullptr, + CORINFO_CONTEXT_HANDLE contextInput = context; + m_compiler->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &contextInput, &context, isLateDevirtualization, explicitTailCall); m_madeChanges = true; } diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index d753d9163bfd7e..c8bf7be68a4587 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -6280,8 +6280,14 @@ void Compiler::impMarkInlineCandidate(GenTree* callNode, { InlineResult inlineResult(this, call, nullptr, "impMarkInlineCandidate for GDV"); + CORINFO_CONTEXT_HANDLE moreExactContext = call->GetGDVCandidateInfo(candidateId)->exactContextHnd; + if (moreExactContext == NULL) + { + moreExactContext = exactContextHnd; + } + // Do the actual evaluation - impMarkInlineCandidateHelper(call, candidateId, exactContextHnd, exactContextNeedsRuntimeLookup, callInfo, + impMarkInlineCandidateHelper(call, candidateId, moreExactContext, exactContextNeedsRuntimeLookup, callInfo, ilOffset, &inlineResult); // Ignore non-inlineable candidates // TODO: Consider keeping them to just devirtualize without inlining, at least for interface @@ -7815,26 +7821,10 @@ void Compiler::impCheckCanInline(GenTreeCall* call, return; } - CORINFO_CONTEXT_HANDLE exactContextHnd = pParam->exactContextHnd; - if (pParam->call->IsGuardedDevirtualizationCandidate()) - { - InlineCandidateInfo* candidateInfo = pParam->call->GetGDVCandidateInfo(pParam->candidateIndex); - if (candidateInfo->exactContextHnd != nullptr) - { - // exactContextHnd represents the exact class the method is defined in. - exactContextHnd = candidateInfo->exactContextHnd; - } - else - { - // exactContextHnd can't be null for normal GDV class guesses - assert(candidateInfo->guardedClassHandle == nullptr); - } - } - // Speculatively check if initClass() can be done. // If it can be done, we will try to inline the method. CorInfoInitClassResult const initClassResult = - compCompHnd->initClass(nullptr /* field */, ftn /* method */, exactContextHnd /* context */); + compCompHnd->initClass(nullptr /* field */, ftn /* method */, pParam->exactContextHnd /* context */); if (initClassResult & CORINFO_INITCLASS_DONT_INLINE) { diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 8b0318b935c82b..37f0d626cbbc30 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -862,7 +862,7 @@ class IndirectCallTransformer JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), block->bbNum); - CORINFO_METHOD_HANDLE methodHnd = call->gtCallMethHnd; + CORINFO_METHOD_HANDLE methodHnd = inlineInfo->guardedMethodHandle; CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; if (clsHnd != NO_CLASS_HANDLE) { @@ -872,7 +872,8 @@ class IndirectCallTransformer unsigned methodFlags = compiler->info.compCompHnd->getMethodAttribs(methodHnd); const bool isLateDevirtualization = true; const bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; - compiler->impDevirtualizeCall(call, nullptr, &methodHnd, &methodFlags, &context, nullptr, + CORINFO_CONTEXT_HANDLE contextInput = context; + compiler->impDevirtualizeCall(call, nullptr, &methodHnd, &methodFlags, &contextInput, &context, isLateDevirtualization, explicitTailCall); } else From c49687be7bde15e5d193e0f53a9d5ba4b00bf6ad Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 16 Jul 2023 17:31:25 +0200 Subject: [PATCH 14/19] temp fix for r2r --- src/coreclr/jit/indirectcalltransformer.cpp | 9 +++++---- .../tools/Common/JitInterface/CorInfoImpl.cs | 13 ------------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 37f0d626cbbc30..536e8ff4b6aeeb 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -905,10 +905,11 @@ class IndirectCallTransformer context = MAKE_METHODCONTEXT(methodHnd); } - // We know this call can devirtualize or we would not have set up GDV here. - // So above code should succeed in devirtualizing. - // - assert(!call->IsVirtual() && !call->IsDelegateInvoke()); + if (call->IsVirtual() || call->IsDelegateInvoke()) + { + // Rare case currently happening only in R2R/ILC + JITDUMP("Failed to devirtualize the candidate, sorry."); + } // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 453e80c381e5c0..7ebf40111eb956 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1293,19 +1293,6 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) if (originalImpl == null) { - // If this assert fires, we failed to devirtualize, probably due to a failure to resolve the - // virtual to an exact target. This should never happen in practice if the input IL is valid, - // and the algorithm for virtual function resolution is correct; however, if it does, this is - // a safe condition, and we could delete this assert. This assert exists in order to help identify - // cases where the virtual function resolution algorithm either does not function, or is not used - // correctly. -#if DEBUG - if (info->detail == CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_UNKNOWN) - { - Console.Error.WriteLine($"Failed devirtualization with unexpected unknown failure while compiling {MethodBeingCompiled} with decl {decl} targeting type {objType}"); - Debug.Assert(info->detail != CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_UNKNOWN); - } -#endif return false; } From 361d9dd1d576447eac2e83dfb44bb2f40bb31c37 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 17 Jul 2023 22:16:32 +0200 Subject: [PATCH 15/19] Revert r2r workaround --- src/coreclr/jit/indirectcalltransformer.cpp | 9 ++++----- .../tools/Common/JitInterface/CorInfoImpl.cs | 13 +++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index 536e8ff4b6aeeb..37f0d626cbbc30 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -905,11 +905,10 @@ class IndirectCallTransformer context = MAKE_METHODCONTEXT(methodHnd); } - if (call->IsVirtual() || call->IsDelegateInvoke()) - { - // Rare case currently happening only in R2R/ILC - JITDUMP("Failed to devirtualize the candidate, sorry."); - } + // We know this call can devirtualize or we would not have set up GDV here. + // So above code should succeed in devirtualizing. + // + assert(!call->IsVirtual() && !call->IsDelegateInvoke()); // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 7ebf40111eb956..453e80c381e5c0 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1293,6 +1293,19 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) if (originalImpl == null) { + // If this assert fires, we failed to devirtualize, probably due to a failure to resolve the + // virtual to an exact target. This should never happen in practice if the input IL is valid, + // and the algorithm for virtual function resolution is correct; however, if it does, this is + // a safe condition, and we could delete this assert. This assert exists in order to help identify + // cases where the virtual function resolution algorithm either does not function, or is not used + // correctly. +#if DEBUG + if (info->detail == CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_UNKNOWN) + { + Console.Error.WriteLine($"Failed devirtualization with unexpected unknown failure while compiling {MethodBeingCompiled} with decl {decl} targeting type {objType}"); + Debug.Assert(info->detail != CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_UNKNOWN); + } +#endif return false; } From 8ff5b0a64790c41e3abf5d19c0d348708203c047 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 17 Jul 2023 18:13:19 -0700 Subject: [PATCH 16/19] Tweak behavior of resolveVirtualMethod in the managed compilers to handle various unboxingStub scenarios better --- .../tools/Common/JitInterface/CorInfoImpl.cs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 453e80c381e5c0..061ff56f14f65a 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1277,6 +1277,10 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) } MethodDesc decl = HandleToObject(info->virtualMethod); + + // Transform from the unboxing thunk to the normal method + decl = decl.IsUnboxingThunk() ? decl.GetUnboxedMethod() : decl; + Debug.Assert(!decl.HasInstantiation); if ((info->context != null) && decl.OwningType.IsInterface) @@ -1369,7 +1373,6 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) #endif ); } - info->resolvedTokenDevirtualizedUnboxedMethod = default(CORINFO_RESOLVED_TOKEN); } else { @@ -1382,17 +1385,17 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) , methodWithTokenImpl #endif ); + } - if (unboxingStub) - { - info->resolvedTokenDevirtualizedUnboxedMethod = info->resolvedTokenDevirtualizedMethod; - info->resolvedTokenDevirtualizedUnboxedMethod.tokenContext = contextFromMethod(nonUnboxingImpl); - info->resolvedTokenDevirtualizedUnboxedMethod.hMethod = ObjectToHandle(nonUnboxingImpl); - } - else - { - info->resolvedTokenDevirtualizedUnboxedMethod = default(CORINFO_RESOLVED_TOKEN); - } + if (unboxingStub) + { + info->resolvedTokenDevirtualizedUnboxedMethod = info->resolvedTokenDevirtualizedMethod; + info->resolvedTokenDevirtualizedUnboxedMethod.tokenContext = contextFromMethod(nonUnboxingImpl); + info->resolvedTokenDevirtualizedUnboxedMethod.hMethod = ObjectToHandle(nonUnboxingImpl); + } + else + { + info->resolvedTokenDevirtualizedUnboxedMethod = default(CORINFO_RESOLVED_TOKEN); } #if READYTORUN From bb7009ddaf8409480a44d3d50e928b8b6c60117e Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 18 Jul 2023 16:00:19 -0700 Subject: [PATCH 17/19] Be a bit more permissive around unboxing stubs in checking virtual override behavior --- src/coreclr/vm/jitinterface.cpp | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index 840b2b9cd390ab..88b1452021dee7 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -13860,7 +13860,8 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, { pImplMethodRuntime = NULL; } - else if (IsMdFinal(pDeclMethod->GetAttrs())) + else if (IsMdFinal(pDeclMethod->GetAttrs()) || + pDeclMethod->GetMethodTable()->IsValueType() && !pDeclMethod->IsUnboxingStub()) { pImplMethodRuntime = pDeclMethod; } @@ -13871,6 +13872,35 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, } } + if (pImplMethodRuntime != pImplMethodCompiler) + { + // Strip unboxing stub differences if they exist. For various reasons the compiler will sometimes + // generate one or the other, and preventing that from hitting this fixup is not worth it + if (pImplMethodRuntime->IsUnboxingStub() != pImplMethodCompiler->IsUnboxingStub()) + { + if (pImplMethodCompiler->IsUnboxingStub()) + { + pImplMethodCompiler = MethodDesc::FindOrCreateAssociatedMethodDesc(pImplMethodCompiler, + pImplMethodCompiler->GetMethodTable(), + FALSE, + pImplMethodCompiler->GetMethodInstantiation(), + FALSE); + + _ASSERTE(!pImplMethodCompiler->IsUnboxingStub()); + } + if (pImplMethodRuntime->IsUnboxingStub()) + { + pImplMethodRuntime = MethodDesc::FindOrCreateAssociatedMethodDesc(pImplMethodRuntime, + pImplMethodRuntime->GetMethodTable(), + FALSE, + pImplMethodRuntime->GetMethodInstantiation(), + FALSE); + + _ASSERTE(!pImplMethodRuntime->IsUnboxingStub()); + } + } + } + if (pImplMethodRuntime != pImplMethodCompiler) { if (kind == ENCODE_CHECK_VIRTUAL_FUNCTION_OVERRIDE) From 254a8431df9979110734acde4bb376724edd503f Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 19 Jul 2023 11:05:03 -0700 Subject: [PATCH 18/19] Fix Unix build break --- 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 88b1452021dee7..bcafaff7b78b6a 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -13861,7 +13861,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, pImplMethodRuntime = NULL; } else if (IsMdFinal(pDeclMethod->GetAttrs()) || - pDeclMethod->GetMethodTable()->IsValueType() && !pDeclMethod->IsUnboxingStub()) + (pDeclMethod->GetMethodTable()->IsValueType() && !pDeclMethod->IsUnboxingStub())) { pImplMethodRuntime = pDeclMethod; } From fbee1a666a173583227424c19f3ded7ad35b06a7 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 21 Jul 2023 10:27:10 -0700 Subject: [PATCH 19/19] This is a cleaner fix --- .../tools/Common/JitInterface/CorInfoImpl.cs | 7 ++-- src/coreclr/vm/jitinterface.cpp | 32 +------------------ 2 files changed, 6 insertions(+), 33 deletions(-) diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs index 061ff56f14f65a..4a65778676feb4 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs @@ -1403,8 +1403,11 @@ private bool resolveVirtualMethod(CORINFO_DEVIRTUALIZATION_INFO* info) // Only generate verification for builds with the stress mode enabled if (_compilation.SymbolNodeFactory.VerifyTypeAndFieldLayout) { - ISymbolNode virtualResolutionNode = _compilation.SymbolNodeFactory.CheckVirtualFunctionOverride(methodWithTokenDecl, objType, methodWithTokenImpl); - AddPrecodeFixup(virtualResolutionNode); + if (!methodWithTokenDecl.Method.OwningType.IsValueType || !methodWithTokenImpl.Method.OwningType.IsValueType) + { + ISymbolNode virtualResolutionNode = _compilation.SymbolNodeFactory.CheckVirtualFunctionOverride(methodWithTokenDecl, objType, methodWithTokenImpl); + AddPrecodeFixup(virtualResolutionNode); + } } #endif info->detail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_SUCCESS; diff --git a/src/coreclr/vm/jitinterface.cpp b/src/coreclr/vm/jitinterface.cpp index bcafaff7b78b6a..840b2b9cd390ab 100644 --- a/src/coreclr/vm/jitinterface.cpp +++ b/src/coreclr/vm/jitinterface.cpp @@ -13860,8 +13860,7 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, { pImplMethodRuntime = NULL; } - else if (IsMdFinal(pDeclMethod->GetAttrs()) || - (pDeclMethod->GetMethodTable()->IsValueType() && !pDeclMethod->IsUnboxingStub())) + else if (IsMdFinal(pDeclMethod->GetAttrs())) { pImplMethodRuntime = pDeclMethod; } @@ -13872,35 +13871,6 @@ BOOL LoadDynamicInfoEntry(Module *currentModule, } } - if (pImplMethodRuntime != pImplMethodCompiler) - { - // Strip unboxing stub differences if they exist. For various reasons the compiler will sometimes - // generate one or the other, and preventing that from hitting this fixup is not worth it - if (pImplMethodRuntime->IsUnboxingStub() != pImplMethodCompiler->IsUnboxingStub()) - { - if (pImplMethodCompiler->IsUnboxingStub()) - { - pImplMethodCompiler = MethodDesc::FindOrCreateAssociatedMethodDesc(pImplMethodCompiler, - pImplMethodCompiler->GetMethodTable(), - FALSE, - pImplMethodCompiler->GetMethodInstantiation(), - FALSE); - - _ASSERTE(!pImplMethodCompiler->IsUnboxingStub()); - } - if (pImplMethodRuntime->IsUnboxingStub()) - { - pImplMethodRuntime = MethodDesc::FindOrCreateAssociatedMethodDesc(pImplMethodRuntime, - pImplMethodRuntime->GetMethodTable(), - FALSE, - pImplMethodRuntime->GetMethodInstantiation(), - FALSE); - - _ASSERTE(!pImplMethodRuntime->IsUnboxingStub()); - } - } - } - if (pImplMethodRuntime != pImplMethodCompiler) { if (kind == ENCODE_CHECK_VIRTUAL_FUNCTION_OVERRIDE)