From d5d98d78f8c1e47623b4afcce6ea4e3e92d375bc Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 15 Feb 2023 15:00:53 -0800 Subject: [PATCH 1/4] JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR The jit currently replaces calls to `CORINFO_HELP_VIRTUAL_FUNC_PTR` whose results are unused with null pointer checks. But this helper can now throw a variety of exceptions, so this optimization is incorrect. Fixes #82127. --- src/coreclr/jit/morph.cpp | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index a34552aa1f197..bfe474334be7d 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -7802,25 +7802,6 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call) #endif } - if ((call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) == 0 && - (call->gtCallMethHnd == eeFindHelper(CORINFO_HELP_VIRTUAL_FUNC_PTR) -#ifdef FEATURE_READYTORUN - || call->gtCallMethHnd == eeFindHelper(CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR) -#endif - ) && - (call == fgMorphStmt->GetRootNode())) - { - // This is call to CORINFO_HELP_VIRTUAL_FUNC_PTR with ignored result. - // Transform it into a null check. - - assert(call->gtArgs.CountArgs() >= 1); - GenTree* objPtr = call->gtArgs.GetArgByIndex(0)->GetNode(); - - GenTree* nullCheck = gtNewNullCheck(objPtr, compCurBB); - - return fgMorphTree(nullCheck); - } - noway_assert(call->gtOper == GT_CALL); // From 4971a053dcaa9b38cdf30223a2e8732db22833e5 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 16 Feb 2023 08:58:32 -0800 Subject: [PATCH 2/4] Revert "JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR" This reverts commit d5d98d78f8c1e47623b4afcce6ea4e3e92d375bc. --- src/coreclr/jit/morph.cpp | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bfe474334be7d..a34552aa1f197 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -7802,6 +7802,25 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call) #endif } + if ((call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) == 0 && + (call->gtCallMethHnd == eeFindHelper(CORINFO_HELP_VIRTUAL_FUNC_PTR) +#ifdef FEATURE_READYTORUN + || call->gtCallMethHnd == eeFindHelper(CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR) +#endif + ) && + (call == fgMorphStmt->GetRootNode())) + { + // This is call to CORINFO_HELP_VIRTUAL_FUNC_PTR with ignored result. + // Transform it into a null check. + + assert(call->gtArgs.CountArgs() >= 1); + GenTree* objPtr = call->gtArgs.GetArgByIndex(0)->GetNode(); + + GenTree* nullCheck = gtNewNullCheck(objPtr, compCurBB); + + return fgMorphTree(nullCheck); + } + noway_assert(call->gtOper == GT_CALL); // From 32d3ea2bd37e0a12f253a95178109f7593614dff Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Thu, 16 Feb 2023 10:07:21 -0800 Subject: [PATCH 3/4] revise to keep the opt for virtual class cases --- src/coreclr/jit/gentree.h | 1 + src/coreclr/jit/importer.cpp | 61 +++++++++++++++++++++++------------- src/coreclr/jit/morph.cpp | 2 +- 3 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 7375e564a1568..b01fe20c7a204 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -4185,6 +4185,7 @@ enum GenTreeCallFlags : unsigned int GTF_CALL_M_STRESS_TAILCALL = 0x04000000, // the call is NOT "tail" prefixed but GTF_CALL_M_EXPLICIT_TAILCALL was added because of tail call stress mode GTF_CALL_M_EXPANDED_EARLY = 0x08000000, // the Virtual Call target address is expanded and placed in gtControlExpr in Morph rather than in Lower GTF_CALL_M_HAS_LATE_DEVIRT_INFO = 0x10000000, // this call has late devirtualzation info + GTF_CALL_M_LDVIRTFTN_INTERFACE = 0x20000000, // ldvirtftn on an interface type }; inline constexpr GenTreeCallFlags operator ~(GenTreeCallFlags a) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5f67160555849..dc9b9f13cdd84 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3116,58 +3116,75 @@ GenTree* Compiler::impImportLdvirtftn(GenTree* thisPtr, CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_CALL_INFO* pCallInfo) { - if ((pCallInfo->methodFlags & CORINFO_FLG_EnC) && !(pCallInfo->classFlags & CORINFO_FLG_INTERFACE)) + const bool isInterface = (pCallInfo->classFlags & CORINFO_FLG_INTERFACE) == CORINFO_FLG_INTERFACE; + + if ((pCallInfo->methodFlags & CORINFO_FLG_EnC) && !isInterface) { NO_WAY("Virtual call to a function added via EnC is not supported"); } + GenTreeCall* call = nullptr; + // NativeAOT generic virtual method if ((pCallInfo->sig.sigInst.methInstCount != 0) && IsTargetAbi(CORINFO_NATIVEAOT_ABI)) { GenTree* runtimeMethodHandle = impLookupToTree(pResolvedToken, &pCallInfo->codePointerLookup, GTF_ICON_METHOD_HDL, pCallInfo->hMethod); - return gtNewHelperCallNode(CORINFO_HELP_GVMLOOKUP_FOR_SLOT, TYP_I_IMPL, thisPtr, runtimeMethodHandle); + call = gtNewHelperCallNode(CORINFO_HELP_GVMLOOKUP_FOR_SLOT, TYP_I_IMPL, thisPtr, runtimeMethodHandle); } #ifdef FEATURE_READYTORUN - if (opts.IsReadyToRun()) + else if (opts.IsReadyToRun()) { if (!pCallInfo->exactContextNeedsRuntimeLookup) { - GenTreeCall* call = gtNewHelperCallNode(CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR, TYP_I_IMPL, thisPtr); - + call = gtNewHelperCallNode(CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR, TYP_I_IMPL, thisPtr); call->setEntryPoint(pCallInfo->codePointerLookup.constLookup); - - return call; } - // We need a runtime lookup. NativeAOT has a ReadyToRun helper for that too. - if (IsTargetAbi(CORINFO_NATIVEAOT_ABI)) + else if (IsTargetAbi(CORINFO_NATIVEAOT_ABI)) { GenTree* ctxTree = getRuntimeContextTree(pCallInfo->codePointerLookup.lookupKind.runtimeLookupKind); - return impReadyToRunHelperToTree(pResolvedToken, CORINFO_HELP_READYTORUN_GENERIC_HANDLE, TYP_I_IMPL, + call = impReadyToRunHelperToTree(pResolvedToken, CORINFO_HELP_READYTORUN_GENERIC_HANDLE, TYP_I_IMPL, &pCallInfo->codePointerLookup.lookupKind, ctxTree); } } #endif - // Get the exact descriptor for the static callsite - GenTree* exactTypeDesc = impParentClassTokenToHandle(pResolvedToken); - if (exactTypeDesc == nullptr) - { // compDonotInline() - return nullptr; - } + if (call == nullptr) + { + // Get the exact descriptor for the static callsite + GenTree* exactTypeDesc = impParentClassTokenToHandle(pResolvedToken); + if (exactTypeDesc == nullptr) + { + assert(compIsForInlining()); + return nullptr; + } - GenTree* exactMethodDesc = impTokenToHandle(pResolvedToken); - if (exactMethodDesc == nullptr) - { // compDonotInline() - return nullptr; + GenTree* exactMethodDesc = impTokenToHandle(pResolvedToken); + if (exactMethodDesc == nullptr) + { + assert(compIsForInlining()); + return nullptr; + } + + // Call helper function. This gets the target address of the final destination callsite. + // + call = gtNewHelperCallNode(CORINFO_HELP_VIRTUAL_FUNC_PTR, TYP_I_IMPL, thisPtr, exactTypeDesc, exactMethodDesc); } - // Call helper function. This gets the target address of the final destination callsite. + assert(call != nullptr); + + if (isInterface) + { + // Annotate helper so later on if helper result is unconsumed we know it is not sound + // to optimize the call into a null check. + // + call->gtCallMoreFlags = GTF_CALL_M_LDVIRTFTN_INTERFACE; + } - return gtNewHelperCallNode(CORINFO_HELP_VIRTUAL_FUNC_PTR, TYP_I_IMPL, thisPtr, exactTypeDesc, exactMethodDesc); + return call; } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index a34552aa1f197..e3e8da3394d8d 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -7802,7 +7802,7 @@ GenTree* Compiler::fgMorphCall(GenTreeCall* call) #endif } - if ((call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) == 0 && + if (((call->gtCallMoreFlags & (GTF_CALL_M_SPECIAL_INTRINSIC | GTF_CALL_M_LDVIRTFTN_INTERFACE)) == 0) && (call->gtCallMethHnd == eeFindHelper(CORINFO_HELP_VIRTUAL_FUNC_PTR) #ifdef FEATURE_READYTORUN || call->gtCallMethHnd == eeFindHelper(CORINFO_HELP_READYTORUN_VIRTUAL_FUNC_PTR) From 20078656cdfa5f4298cb1c97c8adf91eb81450eb Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 22 Feb 2023 16:43:17 -0800 Subject: [PATCH 4/4] fix flag setting --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 1d6ea573d678c..f7865da929a3c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3181,7 +3181,7 @@ GenTree* Compiler::impImportLdvirtftn(GenTree* thisPtr, // Annotate helper so later on if helper result is unconsumed we know it is not sound // to optimize the call into a null check. // - call->gtCallMoreFlags = GTF_CALL_M_LDVIRTFTN_INTERFACE; + call->gtCallMoreFlags |= GTF_CALL_M_LDVIRTFTN_INTERFACE; } return call;