From 50bc23755386098a24a36f93c68176ea4bb3cd29 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 23 Oct 2017 17:46:41 -0700 Subject: [PATCH 1/2] JIT: optimize calls on boxed objects For calls with boxed this objects, invoke the unboxed entry point if available and modify the box to copy the original value or struct to a local. Pass the address of this local as the first argument to the unboxed entry point. Defer for cases where the unboxed entry point requires an extra type parameter. This may enable subsequent inlining of the method, and if the method has other boxed parameters, may enable undoing those boxes as well. The jit is not yet as proficient at eliminating the struct copies, but they are present with or without this change, and some may even be required when the methods mutate the fields of `this`. Closes #14213. --- .../superpmi-shared/icorjitinfoimpl.h | 5 + .../superpmi/superpmi-shared/lwmlist.h | 1 + .../superpmi-shared/methodcontext.cpp | 49 +++++++ .../superpmi/superpmi-shared/methodcontext.h | 10 +- .../superpmi-shim-collector/icorjitinfo.cpp | 15 +++ .../superpmi-shim-counter/icorjitinfo.cpp | 7 + .../superpmi-shim-simple/icorjitinfo.cpp | 6 + src/ToolBox/superpmi/superpmi/icorjitinfo.cpp | 9 ++ src/inc/corinfo.h | 16 ++- src/jit/compiler.h | 3 +- src/jit/gentree.cpp | 93 ++++++++++--- src/jit/importer.cpp | 127 +++++++++++++----- src/vm/jitinterface.cpp | 39 ++++++ src/vm/jitinterface.h | 5 + src/zap/zapinfo.cpp | 10 +- src/zap/zapinfo.h | 10 +- 16 files changed, 344 insertions(+), 61 deletions(-) diff --git a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h index 64a9619e9fea..5c6dc4fc0476 100644 --- a/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h +++ b/src/ToolBox/superpmi/superpmi-shared/icorjitinfoimpl.h @@ -121,6 +121,11 @@ CORINFO_METHOD_HANDLE resolveVirtualMethod(CORINFO_METHOD_HANDLE virtualMethod, CORINFO_CLASS_HANDLE implementingClass, CORINFO_CONTEXT_HANDLE ownerType); +// Get the unboxed entry point for a method, if possible. +CORINFO_METHOD_HANDLE getUnboxedEntry( + CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg /* OUT */); + // Given T, return the type of the default EqualityComparer. // Returns null if the type can't be determined exactly. CORINFO_CLASS_HANDLE getDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE elemType); diff --git a/src/ToolBox/superpmi/superpmi-shared/lwmlist.h b/src/ToolBox/superpmi/superpmi-shared/lwmlist.h index b2f69af782c4..b0cb7ad3c8ee 100644 --- a/src/ToolBox/superpmi/superpmi-shared/lwmlist.h +++ b/src/ToolBox/superpmi/superpmi-shared/lwmlist.h @@ -124,6 +124,7 @@ LWM(GetThreadTLSIndex, DWORD, DLD) LWM(GetTokenTypeAsHandle, GetTokenTypeAsHandleValue, DWORDLONG) LWM(GetTypeForBox, DWORDLONG, DWORDLONG) LWM(GetTypeForPrimitiveValueClass, DWORDLONG, DWORD) +LWM(GetUnboxedEntry, DWORDLONG, DLD); LWM(GetUnBoxHelper, DWORDLONG, DWORD) LWM(GetUnmanagedCallConv, DWORDLONG, DWORD) LWM(GetVarArgsHandle, GetVarArgsHandleValue, DLDL) diff --git a/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp index 0ee17c9104c4..3326a4993789 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.cpp @@ -3018,6 +3018,55 @@ CORINFO_METHOD_HANDLE MethodContext::repResolveVirtualMethod(CORINFO_METHOD_HAND return (CORINFO_METHOD_HANDLE)result; } +void MethodContext::recGetUnboxedEntry(CORINFO_METHOD_HANDLE ftn, + bool *requiresInstMethodTableArg, + CORINFO_METHOD_HANDLE result) +{ + if (GetUnboxedEntry == nullptr) + { + GetUnboxedEntry = new LightWeightMap(); + } + + DWORDLONG key = (DWORDLONG) ftn; + DLD value; + value.A = (DWORDLONG) result; + if (requiresInstMethodTableArg != nullptr) + { + value.B = (DWORD) *requiresInstMethodTableArg ? 1 : 0; + } + else + { + value.B = 0; + } + GetUnboxedEntry->Add(key, value); + DEBUG_REC(dmpGetUnboxedEntry(key, value)); +} + +void MethodContext::dmpGetUnboxedEntry(DWORDLONG key, DLD value) +{ + printf("GetUnboxedEntry ftn-%016llX, result-%016llX, requires-inst-%u", + key, value.A, value.B); +} + +CORINFO_METHOD_HANDLE MethodContext::repGetUnboxedEntry(CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg) +{ + DWORDLONG key = (DWORDLONG)ftn; + + AssertCodeMsg(GetUnboxedEntry != nullptr, EXCEPTIONCODE_MC, "No GetUnboxedEntry map for %016llX", key); + AssertCodeMsg(GetUnboxedEntry->GetIndex(key) != -1, EXCEPTIONCODE_MC, "Didn't find %016llX", key); + DLD result = GetUnboxedEntry->Get(key); + + DEBUG_REP(dmpGetUnboxedEntry(key, result)); + + if (requiresInstMethodTableArg != nullptr) + { + *requiresInstMethodTableArg = (result.B == 1); + } + + return (CORINFO_METHOD_HANDLE)(result.A); +} + void MethodContext::recGetDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE cls, CORINFO_CLASS_HANDLE result) { if (GetDefaultEqualityComparerClass == nullptr) diff --git a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h index ce62aa188d6a..450831d11418 100644 --- a/src/ToolBox/superpmi/superpmi-shared/methodcontext.h +++ b/src/ToolBox/superpmi/superpmi-shared/methodcontext.h @@ -877,6 +877,13 @@ class MethodContext CORINFO_CLASS_HANDLE implClass, CORINFO_CONTEXT_HANDLE ownerType); + void recGetUnboxedEntry(CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg, + CORINFO_METHOD_HANDLE result); + void dmpGetUnboxedEntry(DWORDLONG key, DLD value); + CORINFO_METHOD_HANDLE repGetUnboxedEntry(CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg); + void recGetDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE cls, CORINFO_CLASS_HANDLE result); void dmpGetDefaultEqualityComparerClass(DWORDLONG key, DWORDLONG value); CORINFO_CLASS_HANDLE repGetDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE cls); @@ -1269,7 +1276,7 @@ class MethodContext }; // ********************* Please keep this up-to-date to ease adding more *************** -// Highest packet number: 164 +// Highest packet number: 165 // ************************************************************************************* enum mcPackets { @@ -1384,6 +1391,7 @@ enum mcPackets Packet_GetTokenTypeAsHandle = 89, Packet_GetTypeForBox = 90, Packet_GetTypeForPrimitiveValueClass = 91, + Packet_GetUnboxedEntry = 165, // Added 10/26/17 Packet_GetUnBoxHelper = 92, Packet_GetReadyToRunHelper = 150, // Added 10/10/2014 Packet_GetReadyToRunDelegateCtorHelper = 157, // Added 3/30/2016 diff --git a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp index 1c7c0cbd9f8e..3477baca770f 100644 --- a/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-collector/icorjitinfo.cpp @@ -242,6 +242,21 @@ CORINFO_METHOD_HANDLE interceptor_ICJI::resolveVirtualMethod(CORINFO_METHOD_HAND return result; } +// Get the unboxed entry point for a method, if possible. +CORINFO_METHOD_HANDLE interceptor_ICJI::getUnboxedEntry(CORINFO_METHOD_HANDLE ftn, bool* requiresInstMethodTableArg) +{ + mc->cr->AddCall("getUnboxedEntry"); + bool localRequiresInstMethodTableArg = false; + CORINFO_METHOD_HANDLE result = + original_ICorJitInfo->getUnboxedEntry(ftn, &localRequiresInstMethodTableArg); + mc->recGetUnboxedEntry(ftn, &localRequiresInstMethodTableArg, result); + if (requiresInstMethodTableArg != nullptr) + { + *requiresInstMethodTableArg = localRequiresInstMethodTableArg; + } + return result; +} + // Given T, return the type of the default EqualityComparer. // Returns null if the type can't be determined exactly. CORINFO_CLASS_HANDLE interceptor_ICJI::getDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE cls) diff --git a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp index 1e7ee24d509f..a9e5761c3477 100644 --- a/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-counter/icorjitinfo.cpp @@ -163,6 +163,13 @@ CORINFO_METHOD_HANDLE interceptor_ICJI::resolveVirtualMethod(CORINFO_METHOD_HAND return original_ICorJitInfo->resolveVirtualMethod(virtualMethod, implementingClass, ownerType); } +// Get the unboxed entry point for a method, if possible. +CORINFO_METHOD_HANDLE interceptor_ICJI::getUnboxedEntry(CORINFO_METHOD_HANDLE ftn, bool* requiresInstMethodTableArg) +{ + mcs->AddCall("getUnboxedEntry"); + return original_ICorJitInfo->getUnboxedEntry(ftn, requiresInstMethodTableArg); +} + // Given T, return the type of the default EqualityComparer. // Returns null if the type can't be determined exactly. CORINFO_CLASS_HANDLE interceptor_ICJI::getDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE cls) diff --git a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp index bf3d6483d36a..d7ca029fae84 100644 --- a/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi-shim-simple/icorjitinfo.cpp @@ -141,6 +141,12 @@ void interceptor_ICJI::getMethodVTableOffset(CORINFO_METHOD_HANDLE method, original_ICorJitInfo->getMethodVTableOffset(method, offsetOfIndirection, offsetAfterIndirection, isRelative); } +// Get the unboxed entry point for a method, if possible. +CORINFO_METHOD_HANDLE interceptor_ICJI::getUnboxedEntry(CORINFO_METHOD_HANDLE ftn, bool* requiresInstMethodTableArg) +{ + return original_ICorJitInfo->getUnboxedEntry(ftn, requiresInstMethodTableArg); +} + // Find the virtual method in implementingClass that overrides virtualMethod. // Return null if devirtualization is not possible. CORINFO_METHOD_HANDLE interceptor_ICJI::resolveVirtualMethod(CORINFO_METHOD_HANDLE virtualMethod, diff --git a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp index c53c77942ac6..e8f86cedbd55 100644 --- a/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp +++ b/src/ToolBox/superpmi/superpmi/icorjitinfo.cpp @@ -185,6 +185,15 @@ CORINFO_METHOD_HANDLE MyICJI::resolveVirtualMethod(CORINFO_METHOD_HANDLE virtua return result; } +// Get the unboxed entry point for a method, if possible. +CORINFO_METHOD_HANDLE MyICJI::getUnboxedEntry(CORINFO_METHOD_HANDLE ftn, bool* requiresInstMethodTableArg) +{ + jitInstance->mc->cr->AddCall("getUnboxedEntry"); + CORINFO_METHOD_HANDLE result = + jitInstance->mc->repGetUnboxedEntry(ftn, requiresInstMethodTableArg); + return result; +} + // Given T, return the type of the default EqualityComparer. // Returns null if the type can't be determined exactly. CORINFO_CLASS_HANDLE MyICJI::getDefaultEqualityComparerClass(CORINFO_CLASS_HANDLE cls) diff --git a/src/inc/corinfo.h b/src/inc/corinfo.h index ccd44a61ab28..cd48999cd03e 100644 --- a/src/inc/corinfo.h +++ b/src/inc/corinfo.h @@ -213,11 +213,11 @@ TODO: Talk about initializing strutures before use #define SELECTANY extern __declspec(selectany) #endif -SELECTANY const GUID JITEEVersionIdentifier = { /* 860df660-2919-440a-ad6d-865f014e5360 */ - 0x860df660, - 0x2919, - 0x440a, - { 0xad, 0x6d, 0x86, 0x5f, 0x01, 0x4e, 0x53, 0x60 } +SELECTANY const GUID JITEEVersionIdentifier = { /* b6af83a1-ca48-46c6-b7b0-5d7d6a79a5c5 */ + 0xb6af83a1, + 0xca48, + 0x46c6, + {0xb7, 0xb0, 0x5d, 0x7d, 0x6a, 0x79, 0xa5, 0xc5} }; ////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -2094,6 +2094,12 @@ class ICorStaticInfo CORINFO_CONTEXT_HANDLE ownerType = NULL /* IN */ ) = 0; + // Get the unboxed entry point for a method, if possible. + virtual CORINFO_METHOD_HANDLE getUnboxedEntry( + CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg = NULL /* OUT */ + ) = 0; + // Given T, return the type of the default EqualityComparer. // Returns null if the type can't be determined exactly. virtual CORINFO_CLASS_HANDLE getDefaultEqualityComparerClass( diff --git a/src/jit/compiler.h b/src/jit/compiler.h index 98d4e0b20ebc..369e8b7f5741 100644 --- a/src/jit/compiler.h +++ b/src/jit/compiler.h @@ -2283,7 +2283,8 @@ class Compiler BR_REMOVE_AND_NARROW, // remove effects, minimize remaining work, return possibly narrowed source tree BR_REMOVE_AND_NARROW_WANT_TYPE_HANDLE, // remove effects and minimize remaining work, return type handle tree BR_REMOVE_BUT_NOT_NARROW, // remove effects, return original source tree - BR_DONT_REMOVE // just check if removal is possible + BR_DONT_REMOVE, // just check if removal is possible + BR_MAKE_LOCAL_COPY // revise box to copy to temp local and return local's address }; GenTree* gtTryRemoveBoxUpstreamEffects(GenTree* tree, BoxRemovalOptions options = BR_REMOVE_AND_NARROW); diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp index 65d35d7da097..0031efe83f07 100644 --- a/src/jit/gentree.cpp +++ b/src/jit/gentree.cpp @@ -13150,27 +13150,21 @@ GenTreePtr Compiler::gtFoldExprSpecial(GenTreePtr tree) GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions options) { - JITDUMP("gtTryRemoveBoxUpstreamEffects called for [%06u]\n", dspTreeID(op)); assert(op->IsBoxedValue()); // grab related parts for the optimization - GenTree* asgStmt = op->gtBox.gtAsgStmtWhenInlinedBoxValue; + GenTreeBox* box = op->AsBox(); + GenTree* asgStmt = box->gtAsgStmtWhenInlinedBoxValue; + GenTree* copyStmt = box->gtCopyStmtWhenInlinedBoxValue; + assert(asgStmt->gtOper == GT_STMT); - GenTree* copyStmt = op->gtBox.gtCopyStmtWhenInlinedBoxValue; assert(copyStmt->gtOper == GT_STMT); -#ifdef DEBUG - if (verbose) - { - printf("\n%s to remove side effects of BOX (valuetype)\n", - options == BR_DONT_REMOVE ? "Checking if it is possible" : "Attempting"); - gtDispTree(op); - printf("\nWith assign\n"); - gtDispTree(asgStmt); - printf("\nAnd copy\n"); - gtDispTree(copyStmt); - } -#endif + JITDUMP("gtTryRemoveBoxUpstreamEffects: %s to %s of BOX (valuetype)" + " [%06u] (assign/newobj [%06u] copy [%06u])\n", + (options == BR_DONT_REMOVE) ? "checking if it is possible" : "attempting", + (options == BR_MAKE_LOCAL_COPY) ? "make local unboxed version" : "remove side effects", dspTreeID(op), + dspTreeID(asgStmt), dspTreeID(copyStmt)); // If we don't recognize the form of the assign, bail. GenTree* asg = asgStmt->gtStmt.gtStmtExpr; @@ -13205,7 +13199,7 @@ GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions if (newobjArgs == nullptr) { assert(newobjCall->IsHelperCall(this, CORINFO_HELP_READYTORUN_NEW)); - JITDUMP("bailing; newobj via R2R helper\n"); + JITDUMP(" bailing; newobj via R2R helper\n"); return nullptr; } @@ -13241,6 +13235,73 @@ GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions return nullptr; } + // Handle case where we are optimizing the box into a local copy + if (options == BR_MAKE_LOCAL_COPY) + { + // Drill into the box to get at the box temp local and the box type + GenTree* boxTemp = box->BoxOp(); + assert(boxTemp->IsLocal()); + const unsigned boxTempLcl = boxTemp->AsLclVar()->GetLclNum(); + assert(lvaTable[boxTempLcl].lvType == TYP_REF); + CORINFO_CLASS_HANDLE boxClass = lvaTable[boxTempLcl].lvClassHnd; + assert(boxClass != nullptr); + + // Verify that the copyDst has the expected shape + // (blk|obj|ind (add (boxTempLcl, ptr-size))) + // + // The shape here is constrained to the patterns we produce + // over in impImportAndPushBox for the inlined box case. + GenTree* copyDst = copy->gtOp.gtOp1; + + if (!copyDst->OperIs(GT_BLK, GT_IND, GT_OBJ)) + { + JITDUMP("Unexpected copy dest operator %s\n", GenTree::OpName(copyDst->gtOper)); + return nullptr; + } + + GenTree* copyDstAddr = copyDst->gtOp.gtOp1; + if (copyDstAddr->OperGet() != GT_ADD) + { + JITDUMP("Unexpected copy dest address tree\n"); + return nullptr; + } + + GenTree* copyDstAddrOp1 = copyDstAddr->gtOp.gtOp1; + if ((copyDstAddrOp1->OperGet() != GT_LCL_VAR) || (copyDstAddrOp1->gtLclVarCommon.gtLclNum != boxTempLcl)) + { + JITDUMP("Unexpected copy dest address 1st addend\n"); + return nullptr; + } + + GenTree* copyDstAddrOp2 = copyDstAddr->gtOp.gtOp2; + if (!copyDstAddrOp2->IsIntegralConst(TARGET_POINTER_SIZE)) + { + JITDUMP("Unexpected copy dest address 2nd addend\n"); + return nullptr; + } + + // Screening checks have all passed. Do the transformation. + // + // Retype the box temp to be a struct + JITDUMP("Retyping box temp V%02u to struct %s\n", boxTempLcl, eeGetClassName(boxClass)); + lvaTable[boxTempLcl].lvType = TYP_UNDEF; + const bool isUnsafeValueClass = false; + lvaSetStruct(boxTempLcl, boxClass, isUnsafeValueClass); + + // Remove the newobj and assigment to box temp + JITDUMP("Bashing NEWOBJ [%06u] to NOP\n", dspTreeID(asg)); + asg->gtBashToNOP(); + + // Update the copy from the value to be boxed to the box temp + GenTree* newDst = gtNewOperNode(GT_ADDR, TYP_BYREF, gtNewLclvNode(boxTempLcl, TYP_STRUCT)); + copyDst->gtOp.gtOp1 = newDst; + + // Return the address of the now-struct typed box temp + GenTree* retValue = gtNewOperNode(GT_ADDR, TYP_BYREF, gtNewLclvNode(boxTempLcl, TYP_STRUCT)); + + return retValue; + } + // If the copy is a struct copy, make sure we know how to isolate // any source side effects. GenTree* copySrc = copy->gtOp.gtOp2; diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index 55c27bb487f1..bb02ca809f85 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -12529,9 +12529,10 @@ void Compiler::impImportBlockCode(BasicBlock* block) // If the expression to dup is simple, just clone it. // Otherwise spill it to a temp, and reload the temp // twice. - StackEntry se = impPopStack(); - tiRetVal = se.seTypeInfo; - op1 = se.val; + StackEntry se = impPopStack(); + GenTree* tree = se.val; + tiRetVal = se.seTypeInfo; + op1 = tree; if (!opts.compDbgCode && !op1->IsIntegralConst(0) && !op1->IsFPZero() && !op1->IsLocal()) { @@ -12540,10 +12541,10 @@ void Compiler::impImportBlockCode(BasicBlock* block) var_types type = genActualType(lvaTable[tmpNum].TypeGet()); op1 = gtNewLclvNode(tmpNum, type); - // Propagate type info to the temp + // Propagate type info to the temp from the stack and the original tree if (type == TYP_REF) { - lvaSetClass(tmpNum, op1, tiRetVal.GetClassHandle()); + lvaSetClass(tmpNum, tree, tiRetVal.GetClassHandle()); } } @@ -19073,6 +19074,12 @@ bool Compiler::IsMathIntrinsic(GenTreePtr tree) // code after inlining, if the return value of the inlined call is // the 'this obj' of a subsequent virtual call. // +// If devirtualization succeeds and the call's this object is the +// result of a box, the jit will ask the EE for the unboxed entry +// point. If this exists, the jit will see if it can rework the box +// to instead make a local copy. If that is doable, the call is +// updated to invoke the unboxed entry on the local copy. +// void Compiler::impDevirtualizeCall(GenTreeCall* call, CORINFO_METHOD_HANDLE* method, unsigned* methodFlags, @@ -19353,12 +19360,95 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // stubs) call->gtInlineCandidateInfo = nullptr; +#if defined(DEBUG) + if (verbose) + { + printf("... after devirt...\n"); + gtDispTree(call); + } + + if (doPrint) + { + printf("Devirtualized %s call to %s:%s; now direct call to %s:%s [%s]\n", callKind, baseClassName, + baseMethodName, derivedClassName, derivedMethodName, note); + } +#endif // defined(DEBUG) + + // If the 'this' object is a box, see if we can find the unboxed entry point for the call. + if (thisObj->IsBoxedValue()) + { + JITDUMP("Now have direct call to boxed entry point, looking for unboxed entry point\n"); + + // Note for some shared methods the unboxed entry point requires an extra parameter. + // We defer optimizing if so. + bool requiresInstMethodTableArg = false; + CORINFO_METHOD_HANDLE unboxedEntryMethod = + info.compCompHnd->getUnboxedEntry(derivedMethod, &requiresInstMethodTableArg); + + if (unboxedEntryMethod != nullptr) + { + // Since the call is the only consumer of the box, we know the box can't escape + // since it is being passed an interior pointer. + // + // So, revise the box to simply create a local copy, use the address of that copy + // as the this pointer, and update the entry point to the unboxed entry. + // + // Ideally, we then inline the boxed method and and if it turns out not to modify + // the copy, we can undo the copy too. + if (requiresInstMethodTableArg) + { + // We can likely handle this case by grabbing the argument passed to + // the newobj in the box. But defer for now. + JITDUMP("Found unboxed entry point, but it needs method table arg, deferring\n"); + } + else + { + JITDUMP("Found unboxed entry point, trying to simplify box to a local copy\n"); + GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + + if (localCopyThis != nullptr) + { + JITDUMP("Success! invoking unboxed entry point on local copy\n"); + call->gtCallObjp = localCopyThis; + call->gtCallMethHnd = unboxedEntryMethod; + derivedMethod = unboxedEntryMethod; + } + else + { + JITDUMP("Sorry, failed to undo the box\n"); + } + } + } + else + { + // Many of the low-level methods on value classes won't have unboxed entries, + // as they need access to the type of the object. + // + // Note this may be a cue for us to stack allocate the boxed object, since + // we probably know that these objects don't escape. + JITDUMP("Sorry, failed to find unboxed entry point\n"); + } + } + // Fetch the class that introduced the derived method. // // Note this may not equal objClass, if there is a // final method that objClass inherits. CORINFO_CLASS_HANDLE derivedClass = info.compCompHnd->getMethodClass(derivedMethod); + // Need to update call info too. This is fragile + // but hopefully the derived method conforms to + // the base in most other ways. + *method = derivedMethod; + *methodFlags = derivedMethodAttribs; + *contextHandle = MAKE_METHODCONTEXT(derivedMethod); + + // Update context handle. + if ((exactContextHandle != nullptr) && (*exactContextHandle != nullptr)) + { + *exactContextHandle = MAKE_METHODCONTEXT(derivedMethod); + } + #ifdef FEATURE_READYTORUN_COMPILER if (opts.IsReadyToRun()) { @@ -19385,33 +19475,6 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, call->setEntryPoint(derivedCallInfo.codePointerLookup.constLookup); } #endif // FEATURE_READYTORUN_COMPILER - - // Need to update call info too. This is fragile - // but hopefully the derived method conforms to - // the base in most other ways. - *method = derivedMethod; - *methodFlags = derivedMethodAttribs; - *contextHandle = MAKE_METHODCONTEXT(derivedMethod); - - // Update context handle. - if ((exactContextHandle != nullptr) && (*exactContextHandle != nullptr)) - { - *exactContextHandle = MAKE_METHODCONTEXT(derivedMethod); - } - -#if defined(DEBUG) - if (verbose) - { - printf("... after devirt...\n"); - gtDispTree(call); - } - - if (doPrint) - { - printf("Devirtualized %s call to %s:%s; now direct call to %s:%s [%s]\n", callKind, baseClassName, - baseMethodName, derivedClassName, derivedMethodName, note); - } -#endif // defined(DEBUG) } //------------------------------------------------------------------------ diff --git a/src/vm/jitinterface.cpp b/src/vm/jitinterface.cpp index 7c9fa17dac58..3c5f95a71cd1 100644 --- a/src/vm/jitinterface.cpp +++ b/src/vm/jitinterface.cpp @@ -8816,6 +8816,45 @@ CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethod(CORINFO_METHOD_HANDLE method return result; } +/*********************************************************************/ +CORINFO_METHOD_HANDLE CEEInfo::getUnboxedEntry( + CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg) +{ + CONTRACTL { + SO_TOLERANT; + THROWS; + GC_TRIGGERS; + MODE_PREEMPTIVE; + } CONTRACTL_END; + + CORINFO_METHOD_HANDLE result = NULL; + + JIT_TO_EE_TRANSITION(); + + MethodDesc* pMD = GetMethod(ftn); + bool requiresInstMTArg = false; + + if (pMD->IsUnboxingStub()) + { + MethodTable* pMT = pMD->GetMethodTable(); + MethodDesc* pUnboxedMD = pMT->GetUnboxedEntryPointMD(pMD); + + result = (CORINFO_METHOD_HANDLE)pUnboxedMD; + requiresInstMTArg = !!pUnboxedMD->RequiresInstMethodTableArg(); + } + + if (requiresInstMethodTableArg != NULL) + { + *requiresInstMethodTableArg = requiresInstMTArg; + } + + EE_TO_JIT_TRANSITION(); + + return result; +} + +/*********************************************************************/ void CEEInfo::expandRawHandleIntrinsic( CORINFO_RESOLVED_TOKEN * pResolvedToken, CORINFO_GENERICHANDLE_RESULT * pResult) diff --git a/src/vm/jitinterface.h b/src/vm/jitinterface.h index aa140b00a01a..deeb814f8275 100644 --- a/src/vm/jitinterface.h +++ b/src/vm/jitinterface.h @@ -765,6 +765,11 @@ class CEEInfo : public ICorJitInfo CORINFO_CONTEXT_HANDLE ownerType ); + CORINFO_METHOD_HANDLE getUnboxedEntry( + CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg + ); + CORINFO_CLASS_HANDLE getDefaultEqualityComparerClass( CORINFO_CLASS_HANDLE elemType ); diff --git a/src/zap/zapinfo.cpp b/src/zap/zapinfo.cpp index 4007f678ae3a..f0e9d9e8c4e8 100644 --- a/src/zap/zapinfo.cpp +++ b/src/zap/zapinfo.cpp @@ -3708,12 +3708,18 @@ void ZapInfo::getMethodVTableOffset(CORINFO_METHOD_HANDLE method, CORINFO_METHOD_HANDLE ZapInfo::resolveVirtualMethod( CORINFO_METHOD_HANDLE virtualMethod, CORINFO_CLASS_HANDLE implementingClass, - CORINFO_CONTEXT_HANDLE ownerType - ) + CORINFO_CONTEXT_HANDLE ownerType) { return m_pEEJitInfo->resolveVirtualMethod(virtualMethod, implementingClass, ownerType); } +CORINFO_METHOD_HANDLE ZapInfo::getUnboxedEntry( + CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg) +{ + return m_pEEJitInfo->getUnboxedEntry(ftn, requiresInstMethodTableArg); +} + CORINFO_CLASS_HANDLE ZapInfo::getDefaultEqualityComparerClass( CORINFO_CLASS_HANDLE elemType) { diff --git a/src/zap/zapinfo.h b/src/zap/zapinfo.h index f401cbaf9dc2..1c73d4a0f591 100644 --- a/src/zap/zapinfo.h +++ b/src/zap/zapinfo.h @@ -675,12 +675,14 @@ class ZapInfo CORINFO_METHOD_HANDLE resolveVirtualMethod( CORINFO_METHOD_HANDLE virtualMethod, CORINFO_CLASS_HANDLE implementingClass, - CORINFO_CONTEXT_HANDLE ownerType - ); + CORINFO_CONTEXT_HANDLE ownerType); + + CORINFO_METHOD_HANDLE getUnboxedEntry( + CORINFO_METHOD_HANDLE ftn, + bool* requiresInstMethodTableArg); CORINFO_CLASS_HANDLE getDefaultEqualityComparerClass( - CORINFO_CLASS_HANDLE elemType - ); + CORINFO_CLASS_HANDLE elemType); void expandRawHandleIntrinsic( CORINFO_RESOLVED_TOKEN * pResolvedToken, From 1471c9ccb520328a60ac3414d2a7b0fa90d1905d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 24 Oct 2017 14:39:29 -0700 Subject: [PATCH 2/2] Update code in AwaitUnsafeOnCompleted to use interface matching Given that the jit can now avoid boxing on some interface calls to value types, generalize the patterns introduced in AwaitUnsafeOnCompleted in #14718 by using interfaces instead of checking for specific types. Also move the catch-all processing back in line as the jit can now fold the interface tests early and not pull in the EH portion of the method unless it is needed. --- .../ConfiguredValueTaskAwaitable.cs | 5 +- .../CompilerServices/ValueTaskAwaiter.cs | 5 +- .../CompilerServices/AsyncMethodBuilder.cs | 65 +++++-------------- .../Runtime/CompilerServices/TaskAwaiter.cs | 16 +++++ 4 files changed, 39 insertions(+), 52 deletions(-) diff --git a/src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs b/src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs index 07191181326e..e8aa69e54e3c 100644 --- a/src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs +++ b/src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs @@ -35,7 +35,7 @@ internal ConfiguredValueTaskAwaitable(ValueTask value, bool continueOnC /// Provides an awaiter for a . [StructLayout(LayoutKind.Auto)] - public struct ConfiguredValueTaskAwaiter : ICriticalNotifyCompletion + public struct ConfiguredValueTaskAwaiter : ICriticalNotifyCompletion, IConfiguredValueTaskAwaiter { /// The value being awaited. private ValueTask _value; // Methods are called on this; avoid making it readonly so as to avoid unnecessary copies @@ -71,6 +71,9 @@ internal ConfiguredValueTaskAwaiter(ValueTask value, bool continueOnCap /// Gets the task underlying . internal Task AsTask() => _value.AsTask(); + + /// Gets the task underlying . + (Task, bool) IConfiguredValueTaskAwaiter.GetTask() => (_value.AsTask(), _continueOnCapturedContext); } } } diff --git a/src/mscorlib/shared/System/Runtime/CompilerServices/ValueTaskAwaiter.cs b/src/mscorlib/shared/System/Runtime/CompilerServices/ValueTaskAwaiter.cs index 203039a4a302..094ca7426f05 100644 --- a/src/mscorlib/shared/System/Runtime/CompilerServices/ValueTaskAwaiter.cs +++ b/src/mscorlib/shared/System/Runtime/CompilerServices/ValueTaskAwaiter.cs @@ -9,7 +9,7 @@ namespace System.Runtime.CompilerServices { /// Provides an awaiter for a . - public struct ValueTaskAwaiter : ICriticalNotifyCompletion + public struct ValueTaskAwaiter : ICriticalNotifyCompletion, IValueTaskAwaiter { /// The value being awaited. private ValueTask _value; // Methods are called on this; avoid making it readonly so as to avoid unnecessary copies @@ -38,5 +38,8 @@ public struct ValueTaskAwaiter : ICriticalNotifyCompletion /// Gets the task underlying . internal Task AsTask() => _value.AsTask(); + + /// Gets the task underlying . + Task IValueTaskAwaiter.GetTask() => _value.AsTask(); } } diff --git a/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs b/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs index 6f80529694e2..85eb17c5babe 100644 --- a/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs +++ b/src/mscorlib/src/System/Runtime/CompilerServices/AsyncMethodBuilder.cs @@ -393,7 +393,7 @@ public void SetStateMachine(IAsyncStateMachine stateMachine) { IAsyncStateMachineBox box = GetStateMachineBox(ref stateMachine); - // TThe null tests here ensure that the jit can optimize away the interface + // The null tests here ensure that the jit can optimize away the interface // tests when TAwaiter is is a ref type. if ((null != (object)default(TAwaiter)) && (awaiter is ITaskAwaiter)) { @@ -405,62 +405,27 @@ public void SetStateMachine(IAsyncStateMachine stateMachine) ref ConfiguredTaskAwaitable.ConfiguredTaskAwaiter ta = ref Unsafe.As(ref awaiter); TaskAwaiter.UnsafeOnCompletedInternal(ta.m_task, box, ta.m_continueOnCapturedContext); } - - // Handle common {Configured}ValueTaskAwaiter types. Unfortunately these need to be special-cased - // individually, as we don't have good way to extract the task from a ValueTaskAwaiter when we don't - // know what the T is; we could make ValueTaskAwaiter implement an IValueTaskAwaiter interface, but - // calling a GetTask method on that would end up boxing the awaiter. This hard-coded list here is - // somewhat arbitrary and is based on types currently in use with ValueTask in coreclr/corefx. - else if (typeof(TAwaiter) == typeof(ValueTaskAwaiter)) - { - var vta = (ValueTaskAwaiter)(object)awaiter; - TaskAwaiter.UnsafeOnCompletedInternal(vta.AsTask(), box, continueOnCapturedContext: true); - } - else if (typeof(TAwaiter) == typeof(ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter)) - { - var vta = (ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter)(object)awaiter; - TaskAwaiter.UnsafeOnCompletedInternal(vta.AsTask(), box, vta._continueOnCapturedContext); - } - else if (typeof(TAwaiter) == typeof(ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter)) - { - var vta = (ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter)(object)awaiter; - TaskAwaiter.UnsafeOnCompletedInternal(vta.AsTask(), box, vta._continueOnCapturedContext); - } - else if (typeof(TAwaiter) == typeof(ConfiguredValueTaskAwaitable>.ConfiguredValueTaskAwaiter)) + else if ((null != (object)default(TAwaiter)) && (awaiter is IValueTaskAwaiter)) { - var vta = (ConfiguredValueTaskAwaitable>.ConfiguredValueTaskAwaiter)(object)awaiter; - TaskAwaiter.UnsafeOnCompletedInternal(vta.AsTask(), box, vta._continueOnCapturedContext); + Task t = ((IValueTaskAwaiter)awaiter).GetTask(); + TaskAwaiter.UnsafeOnCompletedInternal(t, box, continueOnCapturedContext: true); } - else if (typeof(TAwaiter) == typeof(ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter)) + else if ((null != (object)default(TAwaiter)) && (awaiter is IConfiguredValueTaskAwaiter)) { - var vta = (ConfiguredValueTaskAwaitable.ConfiguredValueTaskAwaiter)(object)awaiter; - TaskAwaiter.UnsafeOnCompletedInternal(vta.AsTask(), box, vta._continueOnCapturedContext); + (Task task, bool continueOnCapturedContext) t = ((IConfiguredValueTaskAwaiter)awaiter).GetTask(); + TaskAwaiter.UnsafeOnCompletedInternal(t.task, box, t.continueOnCapturedContext); } - // The awaiter isn't specially known. Fall back to doing a normal await. else { - // TODO https://github.com/dotnet/coreclr/issues/14177: - // Move the code back into this method once the JIT is able to - // elide it successfully when one of the previous branches is hit. - AwaitArbitraryAwaiterUnsafeOnCompleted(ref awaiter, box); - } - } - - /// Schedules the specified state machine to be pushed forward when the specified awaiter completes. - /// Specifies the type of the awaiter. - /// The awaiter. - /// The state machine box. - private static void AwaitArbitraryAwaiterUnsafeOnCompleted(ref TAwaiter awaiter, IAsyncStateMachineBox box) - where TAwaiter : ICriticalNotifyCompletion - { - try - { - awaiter.UnsafeOnCompleted(box.MoveNextAction); - } - catch (Exception e) - { - AsyncMethodBuilderCore.ThrowAsync(e, targetContext: null); + try + { + awaiter.UnsafeOnCompleted(box.MoveNextAction); + } + catch (Exception e) + { + AsyncMethodBuilderCore.ThrowAsync(e, targetContext: null); + } } } diff --git a/src/mscorlib/src/System/Runtime/CompilerServices/TaskAwaiter.cs b/src/mscorlib/src/System/Runtime/CompilerServices/TaskAwaiter.cs index 01b803b69759..5b2ac2822232 100644 --- a/src/mscorlib/src/System/Runtime/CompilerServices/TaskAwaiter.cs +++ b/src/mscorlib/src/System/Runtime/CompilerServices/TaskAwaiter.cs @@ -391,6 +391,22 @@ internal interface ITaskAwaiter { } /// internal interface IConfiguredTaskAwaiter { } + /// + /// Internal interface used to enable extract the Task from arbitrary ValueTask awaiters. + /// > + internal interface IValueTaskAwaiter + { + Task GetTask(); + } + + /// + /// Internal interface used to enable extract the Task from arbitrary configured ValueTask awaiters. + /// + internal interface IConfiguredValueTaskAwaiter + { + (Task task, bool continueOnCapturedContext) GetTask(); + } + /// Provides an awaitable object that allows for configured awaits on . /// This type is intended for compiler use only. public struct ConfiguredTaskAwaitable