From d814df327e592975cadb13434514108bcc9e41af Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 30 Apr 2021 20:01:30 -0700 Subject: [PATCH 1/8] JIT: more general value class devirtualization When devirtualization knows that the `this` object is a boxed value class, it now attempts to update the call to invoke the unboxed entry (when there is one). This extends an existing optimization that worked on calls where the box was local and only fed the call. We now handle calls that dispatch on boxed value classes more generally, even if their creation is not local or is local but in a form the existng opt could not handle. These new cases either come from failed local box removal opts (say multi-use boxes) or from guarded devirtualization. The "boxed" entry for value class methods is an un-inlineable VM stub. This transformation effectively inlinines the stub and unblocks inlining of the underlying method. --- src/coreclr/jit/importer.cpp | 273 ++++++++++++++------ src/coreclr/jit/indirectcalltransformer.cpp | 99 ++++--- src/coreclr/jit/inline.h | 2 + 3 files changed, 268 insertions(+), 106 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index a0b08aa3bd1db5..f6866bd63db2b3 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -19198,9 +19198,12 @@ void Compiler::impCheckCanInline(GenTreeCall* call, pInfo = new (pParam->pThis, CMK_Inlining) InlineCandidateInfo; // Null out bits we don't use when we're just inlining - pInfo->guardedClassHandle = nullptr; - pInfo->guardedMethodHandle = nullptr; - pInfo->stubAddr = nullptr; + pInfo->guardedClassHandle = nullptr; + pInfo->guardedMethodHandle = nullptr; + pInfo->guardedMethodUnboxedEntryHandle = nullptr; + pInfo->stubAddr = nullptr; + pInfo->likelihood = 0; + pInfo->requiresInstMethodTableArg = false; } pInfo->methInfo = methInfo; @@ -20338,8 +20341,15 @@ void Compiler::impMarkInlineCandidateHelper(GenTreeCall* call, if (call->IsGuardedDevirtualizationCandidate()) { - fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodHandle; - methAttr = info.compCompHnd->getMethodAttribs(fncHandle); + if (call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle != nullptr) + { + fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodUnboxedEntryHandle; + } + else + { + fncHandle = call->gtGuardedDevirtualizationCandidateInfo->guardedMethodHandle; + } + methAttr = info.compCompHnd->getMethodAttribs(fncHandle); } else { @@ -20636,16 +20646,20 @@ bool Compiler::IsMathIntrinsic(GenTree* 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. +// If devirtualization succeeds and the call's this object is a +// (boxed) value type, the jit will ask the EE for the unboxed entry +// point. If this exists, the jit will invoke the unboxed entry +// on the box payload. In addition if the boxing operation is +// visible to the jit and the call is the only consmer of the box, +// the jit will try analyze the box to see if the call can be instead +// instead made on a local copy. If that is doable, the call is +// updated to invoke the unboxed entry on the local copy and the +// boxing operation is removed. // // When guarded devirtualization is enabled, this method will mark // calls as guarded devirtualization candidates, if the type of `this` // is not exactly known, and there is a plausible guess for the type. - +// void Compiler::impDevirtualizeCall(GenTreeCall* call, CORINFO_METHOD_HANDLE* method, unsigned* methodFlags, @@ -20981,16 +20995,12 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, } #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()) + // If the 'this' object is a value class, see if we can rework the call to invoke the + // unboxed entry. This effectively inlines the normally un-inlineable wrapper stub + // and exposes the potentially inlinable unboxed entry method. + if (info.compCompHnd->isValueClass(derivedClass)) { - JITDUMP("Now have direct call to boxed entry point, looking for unboxed entry point\n"); - - if (isExplicitTailCall) - { - JITDUMP("Call is an explicit tail call, we cannot perform an unbox\n"); - return; - } + JITDUMP("Have a direct call to boxed entry point. Trying to optimize to call an unboxed entry point\n"); // Note for some shared methods the unboxed entry point requires an extra parameter. bool requiresInstMethodTableArg = false; @@ -20999,81 +21009,106 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, 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. + bool optimizedTheBox = false; + + // If the 'this' object is a local box, see if we can revise things + // to not require boxing. // - // 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) + if (thisObj->IsBoxedValue() && !isExplicitTailCall) { - // Perform a trial box removal and ask for the type handle tree. - JITDUMP("Unboxed entry needs method table arg...\n"); - GenTree* methodTableArg = gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); - - if (methodTableArg != 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) { - // If that worked, turn the box into a copy to a local var - JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); - GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + // Perform a trial box removal and ask for the type handle tree that fed the box. + // + JITDUMP("Unboxed entry needs method table arg...\n"); + GenTree* methodTableArg = gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); - if (localCopyThis != nullptr) + if (methodTableArg != nullptr) { - // Pass the local var as this and the type handle as a new arg - JITDUMP("Success! invoking unboxed entry point on local copy, and passing method table arg\n"); - call->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + // If that worked, turn the box into a copy to a local var + // + JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); + GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); - // Prepend for R2L arg passing or empty L2R passing - if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) - { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); - } - // Append for non-empty L2R - else + if (localCopyThis != nullptr) { - GenTreeCall::Use* beforeArg = call->gtCallArgs; - while (beforeArg->GetNext() != nullptr) + // Pass the local var as this and the type handle as a new arg + // + JITDUMP( + "Success! invoking unboxed entry point on local copy, and passing method table arg\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - beforeArg = beforeArg->GetNext(); + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) + { + beforeArg = beforeArg->GetNext(); + } - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); - } + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } - call->gtCallMethHnd = unboxedEntryMethod; - derivedMethod = unboxedEntryMethod; + call->gtCallMethHnd = unboxedEntryMethod; + derivedMethod = unboxedEntryMethod; - // Method attributes will differ because unboxed entry point is shared - const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); - JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, - unboxedMethodAttribs); - derivedMethodAttribs = unboxedMethodAttribs; + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethodAttribs = unboxedMethodAttribs; + optimizedTheBox = true; + } + else + { + JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); + } } else { - JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); + JITDUMP("Sorry, failed to undo the box -- can't find method table arg\n"); } } else { - JITDUMP("Sorry, failed to undo the box -- can't find method table arg\n"); + 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->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + derivedMethod = unboxedEntryMethod; + + optimizedTheBox = true; + } + else + { + JITDUMP("Sorry, failed to undo the box\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) + if (optimizedTheBox) { - JITDUMP("Success! invoking unboxed entry point on local copy\n"); - call->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - derivedMethod = unboxedEntryMethod; #if FEATURE_TAILCALL_OPT if (call->IsImplicitTailCall()) @@ -21087,9 +21122,74 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, } #endif // FEATURE_TAILCALL_OPT } + } + + if (!optimizedTheBox) + { + // If we get here, we have a boxed value class that either wasn't boxed + // locally, or was boxed locally but we were unable to remove the box for + // various reasons. + // + // We can still update the call to invoke the unboxed entry. + // + if (requiresInstMethodTableArg) + { + JITDUMP("revising call to invoke unboxed entry\n"); + + // Update the 'this' pointer to refer to the box payload + // + GenTree* const thisArg = call->gtCallThisArg->GetNode(); + GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); + + call->gtCallThisArg = gtNewCallArgs(boxPayload); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethod = unboxedEntryMethod; + derivedMethodAttribs = unboxedMethodAttribs; + + // Add the method table argument...note we know the value of the class handle + // (we could also fetch this via indir off of the 'this' arg) + // + JITDUMP("call to unboxed entry requires inst method table arg -- adding\n"); + GenTree* const methodTableArg = gtNewIconEmbClsHndNode(derivedClass); + + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) + { + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) + { + beforeArg = beforeArg->GetNext(); + } + + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } + } else { - JITDUMP("Sorry, failed to undo the box\n"); + JITDUMP("revising call to invoke unboxed entry\n"); + + GenTree* const thisArg = call->gtCallThisArg->GetNode(); + GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); + + call->gtCallThisArg = gtNewCallArgs(boxPayload); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + derivedMethod = unboxedEntryMethod; } } } @@ -21514,9 +21614,28 @@ void Compiler::addGuardedDevirtualizationCandidate(GenTreeCall* call, // GuardedDevirtualizationCandidateInfo* pInfo = new (this, CMK_Inlining) InlineCandidateInfo; - pInfo->guardedMethodHandle = methodHandle; - pInfo->guardedClassHandle = classHandle; - pInfo->likelihood = likelihood; + pInfo->guardedMethodHandle = methodHandle; + pInfo->guardedMethodUnboxedEntryHandle = nullptr; + pInfo->guardedClassHandle = classHandle; + pInfo->likelihood = likelihood; + pInfo->requiresInstMethodTableArg = false; + + // If the guarded class is a value class, look for an unboxed entry point. + // + if ((classAttr & CORINFO_FLG_VALUECLASS) != 0) + { + JITDUMP(" ... class is a value class, looking for unboxed entry\n"); + bool requiresInstMethodTableArg = false; + CORINFO_METHOD_HANDLE unboxedEntryMethodHandle = + info.compCompHnd->getUnboxedEntry(methodHandle, &requiresInstMethodTableArg); + + if (unboxedEntryMethodHandle != nullptr) + { + JITDUMP(" ... updating GDV candidate with unboxed entry info\n"); + pInfo->guardedMethodUnboxedEntryHandle = unboxedEntryMethodHandle; + pInfo->requiresInstMethodTableArg = requiresInstMethodTableArg; + } + } // Save off the stub address since it shares a union with the candidate info. // diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index f03482f1109de4..d8b4622ddaa572 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -517,6 +517,7 @@ class IndirectCallTransformer compiler->dspTreeID(origCall), currBlock->bbNum); // We currently need inline candidate info to guarded devirt. + // if (!origCall->IsInlineCandidate()) { JITDUMP("*** %s Bailing on [%06u] -- not an inline candidate\n", Name(), compiler->dspTreeID(origCall)); @@ -748,11 +749,25 @@ class IndirectCallTransformer // Then invoke impDevirtualizeCall to actually // transform the call for us. It should succeed.... as we have // now provided an exact typed this. - CORINFO_METHOD_HANDLE methodHnd = inlineInfo->methInfo.ftn; - unsigned methodFlags = inlineInfo->methAttr; - CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; - const bool isLateDevirtualization = true; - bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; + // + CORINFO_METHOD_HANDLE methodHnd = inlineInfo->methInfo.ftn; + unsigned methodFlags = inlineInfo->methAttr; + CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; + + // If we have a boxed value class the inline info might reflect the properties of the unboxed + // entry. For devirtualization we need these to reflect the boxed entry. + // + // The devirtualizer will update these back, if it can figure how to invoke the unboxed entry. + // + CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedEntryHandle; + if (unboxedMethodHnd != nullptr) + { + methodHnd = inlineInfo->guardedMethodHandle; + 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, &methodHnd, &methodFlags, &context, nullptr, isLateDevirtualization, explicitTailCall); @@ -761,38 +776,64 @@ class IndirectCallTransformer // up here. assert(!call->IsVirtual()); - // Re-establish this call as an inline candidate. - // - GenTree* oldRetExpr = inlineInfo->retExpr; - inlineInfo->clsHandle = compiler->info.compCompHnd->getMethodClass(methodHnd); - inlineInfo->exactContextHnd = context; - inlineInfo->preexistingSpillTemp = returnTemp; - call->gtInlineCandidateInfo = inlineInfo; - - // Add the call. - compiler->fgNewStmtAtEnd(thenBlock, call); - - // If there was a ret expr for this call, we need to create a new one - // and append it just after the call. - // - // Note the original GT_RET_EXPR is sitting at the join point of the - // guarded expansion and for non-void calls, and now refers to a temp local; - // we set all this up in FixupRetExpr(). - if (oldRetExpr != nullptr) + // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info + // we've set up is invalid. We won't be able to inline anyways... + if ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd)) { - GenTree* retExpr = compiler->gtNewInlineCandidateReturnExpr(call, call->TypeGet(), thenBlock->bbFlags); - inlineInfo->retExpr = retExpr; + // Demote this call to a non-inline candidate + // + 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->gtInlineCandidateInfo = nullptr; if (returnTemp != BAD_VAR_NUM) { - retExpr = compiler->gtNewTempAssign(returnTemp, retExpr); + GenTree* const assign = compiler->gtNewTempAssign(returnTemp, call); + compiler->fgNewStmtAtEnd(thenBlock, assign); } else { - // We should always have a return temp if we return results by value - assert(origCall->TypeGet() == TYP_VOID); + compiler->fgNewStmtAtEnd(thenBlock, call); + } + } + else + { + // Add the call. + // + compiler->fgNewStmtAtEnd(thenBlock, call); + + // Re-establish this call as an inline candidate. + // + GenTree* oldRetExpr = inlineInfo->retExpr; + inlineInfo->clsHandle = compiler->info.compCompHnd->getMethodClass(methodHnd); + inlineInfo->exactContextHnd = context; + inlineInfo->preexistingSpillTemp = returnTemp; + call->gtInlineCandidateInfo = inlineInfo; + + // If there was a ret expr for this call, we need to create a new one + // and append it just after the call. + // + // Note the original GT_RET_EXPR has been bashed to a temp. + // we set all this up in FixupRetExpr(). + if (oldRetExpr != nullptr) + { + GenTree* retExpr = + compiler->gtNewInlineCandidateReturnExpr(call, call->TypeGet(), thenBlock->bbFlags); + inlineInfo->retExpr = retExpr; + + if (returnTemp != BAD_VAR_NUM) + { + retExpr = compiler->gtNewTempAssign(returnTemp, retExpr); + } + else + { + // We should always have a return temp if we return results by value + assert(origCall->TypeGet() == TYP_VOID); + } + compiler->fgNewStmtAtEnd(thenBlock, retExpr); } - compiler->fgNewStmtAtEnd(thenBlock, retExpr); } } diff --git a/src/coreclr/jit/inline.h b/src/coreclr/jit/inline.h index 3ed9a5dc97cf5c..e1fb23e80c7916 100644 --- a/src/coreclr/jit/inline.h +++ b/src/coreclr/jit/inline.h @@ -529,7 +529,9 @@ struct GuardedDevirtualizationCandidateInfo : ClassProfileCandidateInfo { CORINFO_CLASS_HANDLE guardedClassHandle; CORINFO_METHOD_HANDLE guardedMethodHandle; + CORINFO_METHOD_HANDLE guardedMethodUnboxedEntryHandle; unsigned likelihood; + bool requiresInstMethodTableArg; }; // InlineCandidateInfo provides basic information about a particular From 359eb20b82e4518a698599dcde09b849e55c9b0e Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Mon, 3 May 2021 18:02:57 -0700 Subject: [PATCH 2/8] Crossgen2 wants to start from the base class method when devirtualizing. --- src/coreclr/jit/indirectcalltransformer.cpp | 38 +++++++-------------- 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/src/coreclr/jit/indirectcalltransformer.cpp b/src/coreclr/jit/indirectcalltransformer.cpp index d8b4622ddaa572..5b6b663e7e035d 100644 --- a/src/coreclr/jit/indirectcalltransformer.cpp +++ b/src/coreclr/jit/indirectcalltransformer.cpp @@ -746,38 +746,26 @@ class IndirectCallTransformer JITDUMP("Direct call [%06u] in block " FMT_BB "\n", compiler->dspTreeID(call), thenBlock->bbNum); - // Then invoke impDevirtualizeCall to actually - // transform the call for us. It should succeed.... as we have - // now provided an exact typed this. + // Then invoke impDevirtualizeCall to actually transform the call for us, + // given the original (base) method and the exact guarded class. It should succeed. // - CORINFO_METHOD_HANDLE methodHnd = inlineInfo->methInfo.ftn; - unsigned methodFlags = inlineInfo->methAttr; - CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; - - // If we have a boxed value class the inline info might reflect the properties of the unboxed - // entry. For devirtualization we need these to reflect the boxed entry. - // - // The devirtualizer will update these back, if it can figure how to invoke the unboxed entry. - // - CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedEntryHandle; - if (unboxedMethodHnd != nullptr) - { - methodHnd = inlineInfo->guardedMethodHandle; - methodFlags = compiler->info.compCompHnd->getMethodAttribs(methodHnd); - } - - const bool isLateDevirtualization = true; - const bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; + CORINFO_METHOD_HANDLE methodHnd = call->gtCallMethHnd; + unsigned methodFlags = compiler->info.compCompHnd->getMethodAttribs(methodHnd); + CORINFO_CONTEXT_HANDLE context = inlineInfo->exactContextHnd; + const bool isLateDevirtualization = true; + const bool explicitTailCall = (call->AsCall()->gtCallMoreFlags & GTF_CALL_M_EXPLICIT_TAILCALL) != 0; compiler->impDevirtualizeCall(call, &methodHnd, &methodFlags, &context, nullptr, isLateDevirtualization, explicitTailCall); - // Presumably devirt might fail? If so we should try and avoid - // making this a guarded devirt candidate instead of ending - // up here. + // We know this call can devirtualize or we would not have set up GDV here. + // So impDevirtualizeCall should succeed in devirtualizing. + // assert(!call->IsVirtual()); // If the devirtualizer was unable to transform the call to invoke the unboxed entry, the inline info - // we've set up is invalid. We won't be able to inline anyways... + // we set up may be invalid. We won't be able to inline anyways. So demote the call as an inline candidate. + // + CORINFO_METHOD_HANDLE unboxedMethodHnd = inlineInfo->guardedMethodUnboxedEntryHandle; if ((unboxedMethodHnd != nullptr) && (methodHnd != unboxedMethodHnd)) { // Demote this call to a non-inline candidate From 2f0cc5be0ecab00a4cec74346c1b0ca1e21da8d7 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Tue, 4 May 2021 11:26:06 -0700 Subject: [PATCH 3/8] defer calling the unboxed entry if we have a shared method table handle --- src/coreclr/jit/importer.cpp | 80 ++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 35 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index f6866bd63db2b3..35fdd7f2c56660 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21130,52 +21130,62 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // locally, or was boxed locally but we were unable to remove the box for // various reasons. // - // We can still update the call to invoke the unboxed entry. + // We may still be able to update the call to invoke the unboxed entry. // if (requiresInstMethodTableArg) { - JITDUMP("revising call to invoke unboxed entry\n"); - - // Update the 'this' pointer to refer to the box payload - // - GenTree* const thisArg = call->gtCallThisArg->GetNode(); - GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); - - call->gtCallThisArg = gtNewCallArgs(boxPayload); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - - // Method attributes will differ because unboxed entry point is shared - // - const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); - JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, - unboxedMethodAttribs); - derivedMethod = unboxedEntryMethod; - derivedMethodAttribs = unboxedMethodAttribs; - - // Add the method table argument...note we know the value of the class handle - // (we could also fetch this via indir off of the 'this' arg) - // - JITDUMP("call to unboxed entry requires inst method table arg -- adding\n"); - GenTree* const methodTableArg = gtNewIconEmbClsHndNode(derivedClass); + const DWORD derivedClassAttribs = info.compCompHnd->getClassAttribs(derivedClass); - // Prepend for R2L arg passing or empty L2R passing - // Append for non-empty L2R - // - if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) + if ((derivedClassAttribs & CORINFO_FLG_SHAREDINST) != 0) { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + JITDUMP("unboxed entry needs MT arg, but the handle we have is shared. Deferring update.\n"); } else { - GenTreeCall::Use* beforeArg = call->gtCallArgs; - while (beforeArg->GetNext() != nullptr) + // See if the method table we have is a viable argument to the unboxed + // entry point. It is, if it's not a shared MT. + // + GenTree* const thisArg = call->gtCallThisArg->GetNode(); + GenTree* const methodTableArg = gtNewIconEmbClsHndNode(derivedClass); + + JITDUMP("revising call to invoke unboxed entry with additional known class handle arg\n"); + + // Update the 'this' pointer to refer to the box payload + // + GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); + + call->gtCallThisArg = gtNewCallArgs(boxPayload); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethod = unboxedEntryMethod; + derivedMethodAttribs = unboxedMethodAttribs; + + // Add the method table argument... + // + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - beforeArg = beforeArg->GetNext(); + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) + { + beforeArg = beforeArg->GetNext(); + } - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } } } else From 5544f8474aba7201e3886d75212ff8890a49ac37 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 5 May 2021 08:55:04 -0700 Subject: [PATCH 4/8] update explicit tail call info --- src/coreclr/jit/importer.cpp | 50 ++++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 35fdd7f2c56660..2ebcbe77e6787b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21230,13 +21230,17 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, *pExactContextHandle = MAKE_CLASSCONTEXT(derivedClass); } +// To update info for explicit tail calls and ready to run, +// we also need to create a resolved token. + #ifdef FEATURE_READYTORUN_COMPILER - if (opts.IsReadyToRun()) + const bool needResolvedToken = isExplicitTailCall || opts.IsReadyToRun(); +#else + const bool needResolvedToken = isExplicitTailCall; +#endif + + if (needResolvedToken) { - // For R2R, getCallInfo triggers bookkeeping on the zap - // side so we need to call it here. - // - // First, cons up a suitable resolved token. CORINFO_RESOLVED_TOKEN derivedResolvedToken = {}; derivedResolvedToken.tokenScope = info.compCompHnd->getMethodModule(derivedMethod); @@ -21246,16 +21250,36 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, derivedResolvedToken.hClass = derivedClass; derivedResolvedToken.hMethod = derivedMethod; - // Look up the new call info. - CORINFO_CALL_INFO derivedCallInfo; - eeGetCallInfo(&derivedResolvedToken, nullptr, addVerifyFlag(CORINFO_CALLINFO_ALLOWINSTPARAM), &derivedCallInfo); + // If this call is an explicit tail call, update the tail call info. + // + if (isExplicitTailCall) + { + TailCallSiteInfo* const tailCallInfo = call->tailCallInfo; + assert(tailCallInfo != nullptr); + CORINFO_SIG_INFO sig; + info.compCompHnd->getMethodSig(derivedMethod, &sig); + tailCallInfo->SetCall(&sig, &derivedResolvedToken); + } - // Update the call. - call->gtCallMoreFlags &= ~GTF_CALL_M_VIRTSTUB_REL_INDIRECT; - call->gtCallMoreFlags &= ~GTF_CALL_M_R2R_REL_INDIRECT; - call->setEntryPoint(derivedCallInfo.codePointerLookup.constLookup); - } +#ifdef FEATURE_READYTORUN_COMPILER + if (opts.IsReadyToRun()) + { + // For R2R, getCallInfo triggers bookkeeping on the zap + // side so we need to call it here. + // + // Look up the new call info. + CORINFO_CALL_INFO derivedCallInfo; + eeGetCallInfo(&derivedResolvedToken, nullptr, addVerifyFlag(CORINFO_CALLINFO_ALLOWINSTPARAM), + &derivedCallInfo); + + // Update the call. + // + call->gtCallMoreFlags &= ~GTF_CALL_M_VIRTSTUB_REL_INDIRECT; + call->gtCallMoreFlags &= ~GTF_CALL_M_R2R_REL_INDIRECT; + call->setEntryPoint(derivedCallInfo.codePointerLookup.constLookup); + } #endif // FEATURE_READYTORUN_COMPILER + } } //------------------------------------------------------------------------ From e9474229ca9c8b99b0a5c62eac25ff329848dfd6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 5 May 2021 09:16:49 -0700 Subject: [PATCH 5/8] Revert "update explicit tail call info" This reverts commit 5544f8474aba7201e3886d75212ff8890a49ac37. --- src/coreclr/jit/importer.cpp | 50 ++++++++++-------------------------- 1 file changed, 13 insertions(+), 37 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 2ebcbe77e6787b..35fdd7f2c56660 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21230,17 +21230,13 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, *pExactContextHandle = MAKE_CLASSCONTEXT(derivedClass); } -// To update info for explicit tail calls and ready to run, -// we also need to create a resolved token. - #ifdef FEATURE_READYTORUN_COMPILER - const bool needResolvedToken = isExplicitTailCall || opts.IsReadyToRun(); -#else - const bool needResolvedToken = isExplicitTailCall; -#endif - - if (needResolvedToken) + if (opts.IsReadyToRun()) { + // For R2R, getCallInfo triggers bookkeeping on the zap + // side so we need to call it here. + // + // First, cons up a suitable resolved token. CORINFO_RESOLVED_TOKEN derivedResolvedToken = {}; derivedResolvedToken.tokenScope = info.compCompHnd->getMethodModule(derivedMethod); @@ -21250,36 +21246,16 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, derivedResolvedToken.hClass = derivedClass; derivedResolvedToken.hMethod = derivedMethod; - // If this call is an explicit tail call, update the tail call info. - // - if (isExplicitTailCall) - { - TailCallSiteInfo* const tailCallInfo = call->tailCallInfo; - assert(tailCallInfo != nullptr); - CORINFO_SIG_INFO sig; - info.compCompHnd->getMethodSig(derivedMethod, &sig); - tailCallInfo->SetCall(&sig, &derivedResolvedToken); - } + // Look up the new call info. + CORINFO_CALL_INFO derivedCallInfo; + eeGetCallInfo(&derivedResolvedToken, nullptr, addVerifyFlag(CORINFO_CALLINFO_ALLOWINSTPARAM), &derivedCallInfo); -#ifdef FEATURE_READYTORUN_COMPILER - if (opts.IsReadyToRun()) - { - // For R2R, getCallInfo triggers bookkeeping on the zap - // side so we need to call it here. - // - // Look up the new call info. - CORINFO_CALL_INFO derivedCallInfo; - eeGetCallInfo(&derivedResolvedToken, nullptr, addVerifyFlag(CORINFO_CALLINFO_ALLOWINSTPARAM), - &derivedCallInfo); - - // Update the call. - // - call->gtCallMoreFlags &= ~GTF_CALL_M_VIRTSTUB_REL_INDIRECT; - call->gtCallMoreFlags &= ~GTF_CALL_M_R2R_REL_INDIRECT; - call->setEntryPoint(derivedCallInfo.codePointerLookup.constLookup); - } -#endif // FEATURE_READYTORUN_COMPILER + // Update the call. + call->gtCallMoreFlags &= ~GTF_CALL_M_VIRTSTUB_REL_INDIRECT; + call->gtCallMoreFlags &= ~GTF_CALL_M_R2R_REL_INDIRECT; + call->setEntryPoint(derivedCallInfo.codePointerLookup.constLookup); } +#endif // FEATURE_READYTORUN_COMPILER } //------------------------------------------------------------------------ From d9f2afdd8277f094ff38acf55bd6bb70e45d8db6 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 5 May 2021 09:27:01 -0700 Subject: [PATCH 6/8] avoid changing explicit tail call target to unboxed entry --- src/coreclr/jit/importer.cpp | 153 +++++++++++++++++++---------------- 1 file changed, 83 insertions(+), 70 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 35fdd7f2c56660..6fe08f9b3f4083 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21009,105 +21009,118 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, if (unboxedEntryMethod != nullptr) { - bool optimizedTheBox = false; + bool didOptimize = false; + bool canOptimize = false; // If the 'this' object is a local box, see if we can revise things // to not require boxing. // - if (thisObj->IsBoxedValue() && !isExplicitTailCall) + if (thisObj->IsBoxedValue()) { - // 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) + if (isExplicitTailCall) { - // Perform a trial box removal and ask for the type handle tree that fed the box. + // We won't optimize away boxes that feed explicit tail calls, as ensuring + // we get the right tail call info is tricky (we'd need to pass an updated + // sig and resolved token back to some callers). // - JITDUMP("Unboxed entry needs method table arg...\n"); - GenTree* methodTableArg = gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); - - if (methodTableArg != nullptr) + canOptimize = false; + } + else + { + // 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) { - // If that worked, turn the box into a copy to a local var + // Perform a trial box removal and ask for the type handle tree that fed the box. // - JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); - GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + JITDUMP("Unboxed entry needs method table arg...\n"); + GenTree* methodTableArg = + gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); - if (localCopyThis != nullptr) + if (methodTableArg != nullptr) { - // Pass the local var as this and the type handle as a new arg + // If that worked, turn the box into a copy to a local var // - JITDUMP( - "Success! invoking unboxed entry point on local copy, and passing method table arg\n"); - call->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); + GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); - // Prepend for R2L arg passing or empty L2R passing - // Append for non-empty L2R - // - if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) - { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); - } - else + if (localCopyThis != nullptr) { - GenTreeCall::Use* beforeArg = call->gtCallArgs; - while (beforeArg->GetNext() != nullptr) + // Pass the local var as this and the type handle as a new arg + // + JITDUMP("Success! invoking unboxed entry point on local copy, and passing method table " + "arg\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - beforeArg = beforeArg->GetNext(); + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) + { + beforeArg = beforeArg->GetNext(); + } - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); - } + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } - call->gtCallMethHnd = unboxedEntryMethod; - derivedMethod = unboxedEntryMethod; + call->gtCallMethHnd = unboxedEntryMethod; + derivedMethod = unboxedEntryMethod; - // Method attributes will differ because unboxed entry point is shared - // - const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); - JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, - unboxedMethodAttribs); - derivedMethodAttribs = unboxedMethodAttribs; - optimizedTheBox = true; + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = + info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethodAttribs = unboxedMethodAttribs; + didOptimize = true; + } + else + { + JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); + } } else { - JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); + JITDUMP("Sorry, failed to undo the box -- can't find method table arg\n"); } } else { - JITDUMP("Sorry, failed to undo the box -- can't find method table arg\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->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - derivedMethod = unboxedEntryMethod; + JITDUMP("Found unboxed entry point, trying to simplify box to a local copy\n"); + GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); - optimizedTheBox = true; - } - else - { - JITDUMP("Sorry, failed to undo the box\n"); + if (localCopyThis != nullptr) + { + JITDUMP("Success! invoking unboxed entry point on local copy\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + derivedMethod = unboxedEntryMethod; + didOptimize = true; + } + else + { + JITDUMP("Sorry, failed to undo the box\n"); + } } } - if (optimizedTheBox) + if (didOptimize) { #if FEATURE_TAILCALL_OPT @@ -21124,7 +21137,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, } } - if (!optimizedTheBox) + if (canOptimize && !didOptimize) { // If we get here, we have a boxed value class that either wasn't boxed // locally, or was boxed locally but we were unable to remove the box for From 4d814ac4ee5cf2976c8871dc2203cfbaa63007a1 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 5 May 2021 09:37:55 -0700 Subject: [PATCH 7/8] Revert "avoid changing explicit tail call target to unboxed entry" This reverts commit d9f2afdd8277f094ff38acf55bd6bb70e45d8db6. --- src/coreclr/jit/importer.cpp | 153 ++++++++++++++++------------------- 1 file changed, 70 insertions(+), 83 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 6fe08f9b3f4083..35fdd7f2c56660 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -21009,118 +21009,105 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, if (unboxedEntryMethod != nullptr) { - bool didOptimize = false; - bool canOptimize = false; + bool optimizedTheBox = false; // If the 'this' object is a local box, see if we can revise things // to not require boxing. // - if (thisObj->IsBoxedValue()) + if (thisObj->IsBoxedValue() && !isExplicitTailCall) { - if (isExplicitTailCall) - { - // We won't optimize away boxes that feed explicit tail calls, as ensuring - // we get the right tail call info is tricky (we'd need to pass an updated - // sig and resolved token back to some callers). - // - canOptimize = false; - } - else + // 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) { - // 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. + // Perform a trial box removal and ask for the type handle tree that fed the box. // - // 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) + JITDUMP("Unboxed entry needs method table arg...\n"); + GenTree* methodTableArg = gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); + + if (methodTableArg != nullptr) { - // Perform a trial box removal and ask for the type handle tree that fed the box. + // If that worked, turn the box into a copy to a local var // - JITDUMP("Unboxed entry needs method table arg...\n"); - GenTree* methodTableArg = - gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); + JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); + GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); - if (methodTableArg != nullptr) + if (localCopyThis != nullptr) { - // If that worked, turn the box into a copy to a local var + // Pass the local var as this and the type handle as a new arg // - JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); - GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + JITDUMP( + "Success! invoking unboxed entry point on local copy, and passing method table arg\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - if (localCopyThis != nullptr) + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - // Pass the local var as this and the type handle as a new arg - // - JITDUMP("Success! invoking unboxed entry point on local copy, and passing method table " - "arg\n"); - call->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - - // Prepend for R2L arg passing or empty L2R passing - // Append for non-empty L2R - // - if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + beforeArg = beforeArg->GetNext(); } - else - { - GenTreeCall::Use* beforeArg = call->gtCallArgs; - while (beforeArg->GetNext() != nullptr) - { - beforeArg = beforeArg->GetNext(); - } - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); - } + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } - call->gtCallMethHnd = unboxedEntryMethod; - derivedMethod = unboxedEntryMethod; + call->gtCallMethHnd = unboxedEntryMethod; + derivedMethod = unboxedEntryMethod; - // Method attributes will differ because unboxed entry point is shared - // - const DWORD unboxedMethodAttribs = - info.compCompHnd->getMethodAttribs(unboxedEntryMethod); - JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, - unboxedMethodAttribs); - derivedMethodAttribs = unboxedMethodAttribs; - didOptimize = true; - } - else - { - JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); - } + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethodAttribs = unboxedMethodAttribs; + optimizedTheBox = true; } else { - JITDUMP("Sorry, failed to undo the box -- can't find method table arg\n"); + JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); } } else { - JITDUMP("Found unboxed entry point, trying to simplify box to a local copy\n"); - GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + JITDUMP("Sorry, failed to undo the box -- can't find method table arg\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->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - derivedMethod = unboxedEntryMethod; - didOptimize = true; - } - else - { - JITDUMP("Sorry, failed to undo the box\n"); - } + if (localCopyThis != nullptr) + { + JITDUMP("Success! invoking unboxed entry point on local copy\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + derivedMethod = unboxedEntryMethod; + + optimizedTheBox = true; + } + else + { + JITDUMP("Sorry, failed to undo the box\n"); } } - if (didOptimize) + if (optimizedTheBox) { #if FEATURE_TAILCALL_OPT @@ -21137,7 +21124,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, } } - if (canOptimize && !didOptimize) + if (!optimizedTheBox) { // If we get here, we have a boxed value class that either wasn't boxed // locally, or was boxed locally but we were unable to remove the box for From f8e3299e4d58fb6c2b30917330adabf36816258d Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Wed, 5 May 2021 09:43:33 -0700 Subject: [PATCH 8/8] block using unboxed entry more broadly --- src/coreclr/jit/importer.cpp | 334 ++++++++++++++++++----------------- 1 file changed, 174 insertions(+), 160 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 35fdd7f2c56660..af20430165fac1 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -20998,219 +20998,233 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call, // If the 'this' object is a value class, see if we can rework the call to invoke the // unboxed entry. This effectively inlines the normally un-inlineable wrapper stub // and exposes the potentially inlinable unboxed entry method. + // + // We won't optimize explicit tail calls, as ensuring we get the right tail call info + // is tricky (we'd need to pass an updated sig and resolved token back to some callers). + // if (info.compCompHnd->isValueClass(derivedClass)) { - JITDUMP("Have a direct call to boxed entry point. Trying to optimize to call an unboxed entry point\n"); - - // Note for some shared methods the unboxed entry point requires an extra parameter. - bool requiresInstMethodTableArg = false; - CORINFO_METHOD_HANDLE unboxedEntryMethod = - info.compCompHnd->getUnboxedEntry(derivedMethod, &requiresInstMethodTableArg); - - if (unboxedEntryMethod != nullptr) + if (isExplicitTailCall) { - bool optimizedTheBox = false; + JITDUMP("Have a direct explicit tail call to boxed entry point; can't optimize further\n"); + } + else + { + JITDUMP("Have a direct call to boxed entry point. Trying to optimize to call an unboxed entry point\n"); - // If the 'this' object is a local box, see if we can revise things - // to not require boxing. - // - if (thisObj->IsBoxedValue() && !isExplicitTailCall) + // Note for some shared methods the unboxed entry point requires an extra parameter. + 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. + bool optimizedTheBox = false; + + // If the 'this' object is a local box, see if we can revise things + // to not require boxing. // - // 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) + if (thisObj->IsBoxedValue() && !isExplicitTailCall) { - // Perform a trial box removal and ask for the type handle tree that fed the box. + // 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. // - JITDUMP("Unboxed entry needs method table arg...\n"); - GenTree* methodTableArg = gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); - - if (methodTableArg != nullptr) + // 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) { - // If that worked, turn the box into a copy to a local var + // Perform a trial box removal and ask for the type handle tree that fed the box. // - JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); - GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + JITDUMP("Unboxed entry needs method table arg...\n"); + GenTree* methodTableArg = + gtTryRemoveBoxUpstreamEffects(thisObj, BR_DONT_REMOVE_WANT_TYPE_HANDLE); - if (localCopyThis != nullptr) + if (methodTableArg != nullptr) { - // Pass the local var as this and the type handle as a new arg + // If that worked, turn the box into a copy to a local var // - JITDUMP( - "Success! invoking unboxed entry point on local copy, and passing method table arg\n"); - call->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + JITDUMP("Found suitable method table arg tree [%06u]\n", dspTreeID(methodTableArg)); + GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); - // Prepend for R2L arg passing or empty L2R passing - // Append for non-empty L2R - // - if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) - { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); - } - else + if (localCopyThis != nullptr) { - GenTreeCall::Use* beforeArg = call->gtCallArgs; - while (beforeArg->GetNext() != nullptr) + // Pass the local var as this and the type handle as a new arg + // + JITDUMP("Success! invoking unboxed entry point on local copy, and passing method table " + "arg\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - beforeArg = beforeArg->GetNext(); + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) + { + beforeArg = beforeArg->GetNext(); + } - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); - } + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } - call->gtCallMethHnd = unboxedEntryMethod; - derivedMethod = unboxedEntryMethod; + call->gtCallMethHnd = unboxedEntryMethod; + derivedMethod = unboxedEntryMethod; - // Method attributes will differ because unboxed entry point is shared - // - const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); - JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, - unboxedMethodAttribs); - derivedMethodAttribs = unboxedMethodAttribs; - optimizedTheBox = true; + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = + info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethodAttribs = unboxedMethodAttribs; + optimizedTheBox = true; + } + else + { + JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); + } } else { - JITDUMP("Sorry, failed to undo the box -- can't convert to local copy\n"); + JITDUMP("Sorry, failed to undo the box -- can't find method table arg\n"); } } else { - JITDUMP("Sorry, failed to undo the box -- can't find method table arg\n"); - } - } - else - { - JITDUMP("Found unboxed entry point, trying to simplify box to a local copy\n"); - GenTree* localCopyThis = gtTryRemoveBoxUpstreamEffects(thisObj, BR_MAKE_LOCAL_COPY); + 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->gtCallThisArg = gtNewCallArgs(localCopyThis); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - derivedMethod = unboxedEntryMethod; + if (localCopyThis != nullptr) + { + JITDUMP("Success! invoking unboxed entry point on local copy\n"); + call->gtCallThisArg = gtNewCallArgs(localCopyThis); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + derivedMethod = unboxedEntryMethod; - optimizedTheBox = true; - } - else - { - JITDUMP("Sorry, failed to undo the box\n"); + optimizedTheBox = true; + } + else + { + JITDUMP("Sorry, failed to undo the box\n"); + } } - } - if (optimizedTheBox) - { + if (optimizedTheBox) + { #if FEATURE_TAILCALL_OPT - if (call->IsImplicitTailCall()) - { - JITDUMP("Clearing the implicit tail call flag\n"); + if (call->IsImplicitTailCall()) + { + JITDUMP("Clearing the implicit tail call flag\n"); - // If set, we clear the implicit tail call flag - // as we just introduced a new address taken local variable - // - call->gtCallMoreFlags &= ~GTF_CALL_M_IMPLICIT_TAILCALL; - } + // If set, we clear the implicit tail call flag + // as we just introduced a new address taken local variable + // + call->gtCallMoreFlags &= ~GTF_CALL_M_IMPLICIT_TAILCALL; + } #endif // FEATURE_TAILCALL_OPT + } } - } - if (!optimizedTheBox) - { - // If we get here, we have a boxed value class that either wasn't boxed - // locally, or was boxed locally but we were unable to remove the box for - // various reasons. - // - // We may still be able to update the call to invoke the unboxed entry. - // - if (requiresInstMethodTableArg) + if (!optimizedTheBox) { - const DWORD derivedClassAttribs = info.compCompHnd->getClassAttribs(derivedClass); - - if ((derivedClassAttribs & CORINFO_FLG_SHAREDINST) != 0) - { - JITDUMP("unboxed entry needs MT arg, but the handle we have is shared. Deferring update.\n"); - } - else + // If we get here, we have a boxed value class that either wasn't boxed + // locally, or was boxed locally but we were unable to remove the box for + // various reasons. + // + // We may still be able to update the call to invoke the unboxed entry. + // + if (requiresInstMethodTableArg) { - // See if the method table we have is a viable argument to the unboxed - // entry point. It is, if it's not a shared MT. - // - GenTree* const thisArg = call->gtCallThisArg->GetNode(); - GenTree* const methodTableArg = gtNewIconEmbClsHndNode(derivedClass); - - JITDUMP("revising call to invoke unboxed entry with additional known class handle arg\n"); - - // Update the 'this' pointer to refer to the box payload - // - GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); + const DWORD derivedClassAttribs = info.compCompHnd->getClassAttribs(derivedClass); - call->gtCallThisArg = gtNewCallArgs(boxPayload); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - - // Method attributes will differ because unboxed entry point is shared - // - const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); - JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, - unboxedMethodAttribs); - derivedMethod = unboxedEntryMethod; - derivedMethodAttribs = unboxedMethodAttribs; - - // Add the method table argument... - // - // Prepend for R2L arg passing or empty L2R passing - // Append for non-empty L2R - // - if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) + if ((derivedClassAttribs & CORINFO_FLG_SHAREDINST) != 0) { - call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); + JITDUMP( + "unboxed entry needs MT arg, but the handle we have is shared. Deferring update.\n"); } else { - GenTreeCall::Use* beforeArg = call->gtCallArgs; - while (beforeArg->GetNext() != nullptr) + // See if the method table we have is a viable argument to the unboxed + // entry point. It is, if it's not a shared MT. + // + GenTree* const thisArg = call->gtCallThisArg->GetNode(); + GenTree* const methodTableArg = gtNewIconEmbClsHndNode(derivedClass); + + JITDUMP("revising call to invoke unboxed entry with additional known class handle arg\n"); + + // Update the 'this' pointer to refer to the box payload + // + GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); + + call->gtCallThisArg = gtNewCallArgs(boxPayload); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + + // Method attributes will differ because unboxed entry point is shared + // + const DWORD unboxedMethodAttribs = info.compCompHnd->getMethodAttribs(unboxedEntryMethod); + JITDUMP("Updating method attribs from 0x%08x to 0x%08x\n", derivedMethodAttribs, + unboxedMethodAttribs); + derivedMethod = unboxedEntryMethod; + derivedMethodAttribs = unboxedMethodAttribs; + + // Add the method table argument... + // + // Prepend for R2L arg passing or empty L2R passing + // Append for non-empty L2R + // + if ((Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) || (call->gtCallArgs == nullptr)) { - beforeArg = beforeArg->GetNext(); + call->gtCallArgs = gtPrependNewCallArg(methodTableArg, call->gtCallArgs); } + else + { + GenTreeCall::Use* beforeArg = call->gtCallArgs; + while (beforeArg->GetNext() != nullptr) + { + beforeArg = beforeArg->GetNext(); + } - beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + beforeArg->SetNext(gtNewCallArgs(methodTableArg)); + } } } - } - else - { - JITDUMP("revising call to invoke unboxed entry\n"); + else + { + JITDUMP("revising call to invoke unboxed entry\n"); - GenTree* const thisArg = call->gtCallThisArg->GetNode(); - GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); + GenTree* const thisArg = call->gtCallThisArg->GetNode(); + GenTree* const payloadOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTree* const boxPayload = gtNewOperNode(GT_ADD, TYP_BYREF, thisArg, payloadOffset); - call->gtCallThisArg = gtNewCallArgs(boxPayload); - call->gtCallMethHnd = unboxedEntryMethod; - call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; - derivedMethod = unboxedEntryMethod; + call->gtCallThisArg = gtNewCallArgs(boxPayload); + call->gtCallMethHnd = unboxedEntryMethod; + call->gtCallMoreFlags |= GTF_CALL_M_UNBOXED; + derivedMethod = unboxedEntryMethod; + } } } - } - 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"); + 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"); + } } }