From 99e6b6b461579fe5feab3b8675b57dd3bd22535a Mon Sep 17 00:00:00 2001 From: Jan Kotas Date: Sat, 22 Aug 2020 00:29:14 -0700 Subject: [PATCH] Fix tailcall regression with compiled F# This change skips instantiating stubs for direct tailcalls and instead passes the inst argument directly to the target method. Fixes #40864 --- .../RuntimeHelpers.CoreCLR.cs | 12 +- src/coreclr/src/jit/compiler.h | 1 - src/coreclr/src/jit/morph.cpp | 89 ++++------- src/coreclr/src/vm/corelib.h | 8 - src/coreclr/src/vm/ecalllist.h | 1 - src/coreclr/src/vm/gcenv.ee.cpp | 20 ++- src/coreclr/src/vm/jitinterface.cpp | 2 +- src/coreclr/src/vm/tailcallhelp.cpp | 145 ++++++++++-------- src/coreclr/src/vm/tailcallhelp.h | 5 +- src/coreclr/src/vm/threads.cpp | 44 ++---- src/coreclr/src/vm/threads.h | 25 ++- 11 files changed, 160 insertions(+), 192 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs index 5c9a38e2a6f70..6b6e4639bfec2 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs @@ -286,9 +286,6 @@ public static IntPtr AllocateTypeAssociatedMemory(Type type, int size) [MethodImpl(MethodImplOptions.InternalCall)] private static extern IntPtr AllocTailCallArgBuffer(int size, IntPtr gcDesc); - [MethodImpl(MethodImplOptions.InternalCall)] - private static extern void FreeTailCallArgBuffer(); - [MethodImpl(MethodImplOptions.InternalCall)] private static unsafe extern TailCallTls* GetTailCallInfo(IntPtr retAddrSlot, IntPtr* retAddr); @@ -323,6 +320,12 @@ private static unsafe void DispatchTailCalls( finally { tls->Frame = prevFrame; + + // If the arg buffer is reporting inst argument, it is safe to abandon it now + if (tls->ArgBuffer != IntPtr.Zero && *(int*)tls->ArgBuffer == 1 /* TAILCALLARGBUFFER_INSTARG_ONLY */) + { + *(int*)tls->ArgBuffer = 2 /* TAILCALLARGBUFFER_ABANDONED */; + } } } @@ -481,9 +484,6 @@ internal unsafe struct TailCallTls { public PortableTailCallFrame* Frame; public IntPtr ArgBuffer; - private IntPtr _argBufferSize; - private IntPtr _argBufferGCDesc; - private fixed byte _argBufferInline[64]; } } diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index e6788ddfe5679..e94dc8ad4c9a2 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -5579,7 +5579,6 @@ class Compiler GenTree* fgCreateCallDispatcherAndGetResult(GenTreeCall* origCall, CORINFO_METHOD_HANDLE callTargetStubHnd, CORINFO_METHOD_HANDLE dispatcherHnd); - GenTree* getMethodPointerTree(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_CALL_INFO* pCallInfo); GenTree* getLookupTree(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_LOOKUP* pLookup, unsigned handleFlags, diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index b173993d6bb55..dc5bef62f9c44 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -7686,6 +7686,8 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL assert(!call->IsImplicitTailCall()); assert(!fgCanFastTailCall(call, nullptr)); + bool virtualCall = call->IsVirtual(); + // If VSD then get rid of arg to VSD since we turn this into a direct call. // The extra arg will be the first arg so this needs to be done before we // handle the retbuf below. @@ -7720,41 +7722,39 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL // where we pass instantiating stub. if ((help.flags & CORINFO_TAILCALL_STORE_TARGET) != 0) { - // If asked to store target and we have a type arg we will store - // instantiating stub, so in that case we should not pass the type arg. - if (call->tailCallInfo->GetSig()->hasTypeArg()) + JITDUMP("Adding target since VM requested it\n"); + GenTree* target; + if (!virtualCall) { - JITDUMP("Removing type arg"); - - assert(call->gtCallArgs != nullptr); - if (Target::g_tgtArgOrder == Target::ARG_ORDER_R2L) + if (call->gtCallType == CT_INDIRECT) { - // Generic context is first arg - call->gtCallArgs = call->gtCallArgs->GetNext(); + noway_assert(call->gtCallAddr != nullptr); + target = call->gtCallAddr; } else { - // Generic context is last arg - GenTreeCall::Use** lastArgSlot = &call->gtCallArgs; - while ((*lastArgSlot)->GetNext() != nullptr) + CORINFO_CONST_LOOKUP addrInfo; + info.compCompHnd->getFunctionEntryPoint(call->gtCallMethHnd, &addrInfo); + + CORINFO_GENERIC_HANDLE handle = nullptr; + void* pIndirection = nullptr; + assert(addrInfo.accessType != IAT_PPVALUE && addrInfo.accessType != IAT_RELPVALUE); + + if (addrInfo.accessType == IAT_VALUE) { - lastArgSlot = &(*lastArgSlot)->NextRef(); + handle = addrInfo.handle; } - - *lastArgSlot = nullptr; + else if (addrInfo.accessType == IAT_PVALUE) + { + pIndirection = addrInfo.addr; + } + target = gtNewIconEmbHndNode(handle, pIndirection, GTF_ICON_FTN_ADDR, call->gtCallMethHnd); } - call->fgArgInfo = nullptr; - } - - JITDUMP("Adding target since VM requested it\n"); - GenTree* target; - if (call->tailCallInfo->IsCalli()) - { - noway_assert(call->gtCallType == CT_INDIRECT && call->gtCallAddr != nullptr); - target = call->gtCallAddr; } else { + assert(!call->tailCallInfo->GetSig()->hasTypeArg()); + CORINFO_CALL_INFO callInfo; unsigned flags = CORINFO_CALLINFO_LDFTN; if (call->tailCallInfo->IsCallvirt()) @@ -7764,19 +7764,10 @@ GenTree* Compiler::fgMorphTailCallViaHelpers(GenTreeCall* call, CORINFO_TAILCALL eeGetCallInfo(call->tailCallInfo->GetToken(), nullptr, (CORINFO_CALLINFO_FLAGS)flags, &callInfo); - if (!call->tailCallInfo->IsCallvirt() || - ((callInfo.methodFlags & (CORINFO_FLG_FINAL | CORINFO_FLG_STATIC)) != 0) || - ((callInfo.methodFlags & CORINFO_FLG_VIRTUAL) == 0)) - { - target = getMethodPointerTree(call->tailCallInfo->GetToken(), &callInfo); - } - else - { - assert(call->gtCallThisArg != nullptr); - // TODO: Proper cloning of the this pointer. - target = getVirtMethodPointerTree(gtCloneExpr(call->gtCallThisArg->GetNode()), - call->tailCallInfo->GetToken(), &callInfo); - } + assert(call->gtCallThisArg != nullptr); + // TODO: Proper cloning of the this pointer. + target = getVirtMethodPointerTree(gtCloneExpr(call->gtCallThisArg->GetNode()), + call->tailCallInfo->GetToken(), &callInfo); } // Insert target as last arg @@ -8030,30 +8021,6 @@ GenTree* Compiler::fgCreateCallDispatcherAndGetResult(GenTreeCall* orig return finalTree; } -//------------------------------------------------------------------------ -// getMethodPointerTree: get a method pointer tree -// -// Arguments: -// pResolvedToken - resolved token of the call -// pCallInfo - the call info of the call -// -// Return Value: -// A node representing the method pointer -// -GenTree* Compiler::getMethodPointerTree(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_CALL_INFO* pCallInfo) -{ - switch (pCallInfo->kind) - { - case CORINFO_CALL: - return new (this, GT_FTN_ADDR) GenTreeFptrVal(TYP_I_IMPL, pCallInfo->hMethod); - case CORINFO_CALL_CODE_POINTER: - return getLookupTree(pResolvedToken, &pCallInfo->codePointerLookup, GTF_ICON_FTN_ADDR, pCallInfo->hMethod); - default: - noway_assert(!"unknown call kind"); - return nullptr; - } -} - //------------------------------------------------------------------------ // getLookupTree: get a lookup tree // diff --git a/src/coreclr/src/vm/corelib.h b/src/coreclr/src/vm/corelib.h index 5b08cc4695c6e..044147c8eb2a6 100644 --- a/src/coreclr/src/vm/corelib.h +++ b/src/coreclr/src/vm/corelib.h @@ -708,7 +708,6 @@ DEFINE_METHOD(RUNTIME_HELPERS, ENUM_EQUALS, EnumEquals, NoSig) DEFINE_METHOD(RUNTIME_HELPERS, ENUM_COMPARE_TO, EnumCompareTo, NoSig) DEFINE_METHOD(RUNTIME_HELPERS, ALLOC_TAILCALL_ARG_BUFFER, AllocTailCallArgBuffer, SM_Int_IntPtr_RetIntPtr) DEFINE_METHOD(RUNTIME_HELPERS, GET_TAILCALL_INFO, GetTailCallInfo, NoSig) -DEFINE_METHOD(RUNTIME_HELPERS, FREE_TAILCALL_ARG_BUFFER, FreeTailCallArgBuffer, SM_RetVoid) DEFINE_METHOD(RUNTIME_HELPERS, DISPATCH_TAILCALLS, DispatchTailCalls, NoSig) DEFINE_CLASS(UNSAFE, InternalCompilerServices, Unsafe) @@ -760,9 +759,6 @@ DEFINE_FIELD(PORTABLE_TAIL_CALL_FRAME, NEXT_CALL, NextCall) DEFINE_CLASS(TAIL_CALL_TLS, CompilerServices, TailCallTls) DEFINE_FIELD(TAIL_CALL_TLS, FRAME, Frame) DEFINE_FIELD(TAIL_CALL_TLS, ARG_BUFFER, ArgBuffer) -DEFINE_FIELD(TAIL_CALL_TLS, ARG_BUFFER_SIZE, _argBufferSize) -DEFINE_FIELD(TAIL_CALL_TLS, ARG_BUFFER_GC_DESC, _argBufferGCDesc) -DEFINE_FIELD(TAIL_CALL_TLS, ARG_BUFFER_INLINE, _argBufferInline) DEFINE_CLASS_U(CompilerServices, PortableTailCallFrame, PortableTailCallFrame) DEFINE_FIELD_U(Prev, PortableTailCallFrame, Prev) @@ -772,10 +768,6 @@ DEFINE_FIELD_U(NextCall, PortableTailCallFrame, NextCall) DEFINE_CLASS_U(CompilerServices, TailCallTls, TailCallTls) DEFINE_FIELD_U(Frame, TailCallTls, m_frame) DEFINE_FIELD_U(ArgBuffer, TailCallTls, m_argBuffer) -DEFINE_FIELD_U(_argBufferSize, TailCallTls, m_argBufferSize) -DEFINE_FIELD_U(_argBufferGCDesc, TailCallTls, m_argBufferGCDesc) -DEFINE_FIELD_U(_argBufferInline, TailCallTls, m_argBufferInline) - DEFINE_CLASS(RUNTIME_WRAPPED_EXCEPTION, CompilerServices, RuntimeWrappedException) DEFINE_METHOD(RUNTIME_WRAPPED_EXCEPTION, OBJ_CTOR, .ctor, IM_Obj_RetVoid) diff --git a/src/coreclr/src/vm/ecalllist.h b/src/coreclr/src/vm/ecalllist.h index f1827fa3bd97a..2c2dd518c09bf 100644 --- a/src/coreclr/src/vm/ecalllist.h +++ b/src/coreclr/src/vm/ecalllist.h @@ -881,7 +881,6 @@ FCFuncStart(gRuntimeHelpers) FCFuncElement("GetUninitializedObjectInternal", ReflectionSerialization::GetUninitializedObject) QCFuncElement("AllocateTypeAssociatedMemoryInternal", RuntimeTypeHandle::AllocateTypeAssociatedMemory) FCFuncElement("AllocTailCallArgBuffer", TailCallHelp::AllocTailCallArgBuffer) - FCFuncElement("FreeTailCallArgBuffer", TailCallHelp::FreeTailCallArgBuffer) FCFuncElement("GetTailCallInfo", TailCallHelp::GetTailCallInfo) FCFuncElement("GetILBytesJitted", GetJittedBytes) FCFuncElement("GetMethodsJittedCount", GetJittedMethodsCount) diff --git a/src/coreclr/src/vm/gcenv.ee.cpp b/src/coreclr/src/vm/gcenv.ee.cpp index 54bde52c750b4..6eaf17683cc28 100644 --- a/src/coreclr/src/vm/gcenv.ee.cpp +++ b/src/coreclr/src/vm/gcenv.ee.cpp @@ -157,27 +157,33 @@ static void ScanStackRoots(Thread * pThread, promote_func* fn, ScanContext* sc) static void ScanTailCallArgBufferRoots(Thread* pThread, promote_func* fn, ScanContext* sc) { - void* gcDesc; - char* argBuffer = pThread->GetTailCallTls()->GetArgBuffer(&gcDesc); - if (gcDesc == NULL) + TailCallArgBuffer* argBuffer = pThread->GetTailCallTls()->GetArgBuffer(); + if (argBuffer == NULL || argBuffer->GCDesc == NULL) return; - GCRefMapDecoder decoder(static_cast(gcDesc)); + if (argBuffer->State == TAILCALLARGBUFFER_ABANDONED) + return; + + bool instArgOnly = argBuffer->State == TAILCALLARGBUFFER_INSTARG_ONLY; + + GCRefMapDecoder decoder(static_cast(argBuffer->GCDesc)); while (!decoder.AtEnd()) { int pos = decoder.CurrentPos(); int token = decoder.ReadToken(); - PTR_TADDR ppObj = dac_cast(argBuffer + pos * sizeof(TADDR)); + PTR_TADDR ppObj = dac_cast(((BYTE*)argBuffer->Args) + pos * sizeof(TADDR)); switch (token) { case GCREFMAP_SKIP: break; case GCREFMAP_REF: - fn(dac_cast(ppObj), sc, CHECK_APP_DOMAIN); + if (!instArgOnly) + fn(dac_cast(ppObj), sc, CHECK_APP_DOMAIN); break; case GCREFMAP_INTERIOR: - PromoteCarefully(fn, dac_cast(ppObj), sc, GC_CALL_INTERIOR); + if (!instArgOnly) + PromoteCarefully(fn, dac_cast(ppObj), sc, GC_CALL_INTERIOR); break; case GCREFMAP_METHOD_PARAM: if (sc->promotion) diff --git a/src/coreclr/src/vm/jitinterface.cpp b/src/coreclr/src/vm/jitinterface.cpp index dbf4ec793bf0b..37bede3e2dc0d 100644 --- a/src/coreclr/src/vm/jitinterface.cpp +++ b/src/coreclr/src/vm/jitinterface.cpp @@ -13952,7 +13952,7 @@ bool CEEInfo::getTailCallHelpersInternal(CORINFO_RESOLVED_TOKEN* callToken, TailCallHelp::CreateTailCallHelperStubs( m_pMethodBeingCompiled, pTargetMD, - msig, isCallvirt, isThisArgByRef, + msig, isCallvirt, isThisArgByRef, sig->hasTypeArg(), &pStoreArgsMD, &needsTarget, &pCallTargetMD); diff --git a/src/coreclr/src/vm/tailcallhelp.cpp b/src/coreclr/src/vm/tailcallhelp.cpp index 0db1330b7b3c0..10a9c501cf2f0 100644 --- a/src/coreclr/src/vm/tailcallhelp.cpp +++ b/src/coreclr/src/vm/tailcallhelp.cpp @@ -23,7 +23,7 @@ FCIMPL2(void*, TailCallHelp::AllocTailCallArgBuffer, INT32 size, void* gcDesc) _ASSERTE(size >= 0); - void* result = GetThread()->GetTailCallTls()->AllocArgBuffer(static_cast(size), gcDesc); + void* result = GetThread()->GetTailCallTls()->AllocArgBuffer(size, gcDesc); if (result == NULL) FCThrow(kOutOfMemoryException); @@ -32,14 +32,6 @@ FCIMPL2(void*, TailCallHelp::AllocTailCallArgBuffer, INT32 size, void* gcDesc) } FCIMPLEND -FCIMPL0(void, TailCallHelp::FreeTailCallArgBuffer) -{ - FCALL_CONTRACT; - - GetThread()->GetTailCallTls()->FreeArgBuffer(); -} -FCIMPLEND - FCIMPL2(void*, TailCallHelp::GetTailCallInfo, void** retAddrSlot, void** retAddr) { FCALL_CONTRACT; @@ -67,12 +59,14 @@ struct ArgBufferValue struct ArgBufferLayout { bool HasTargetAddress; + bool HasInstArg; unsigned int TargetAddressOffset; InlineSArray Values; unsigned int Size; ArgBufferLayout() : HasTargetAddress(false) + , HasInstArg(false) , TargetAddressOffset(0) , Size(0) { @@ -133,7 +127,7 @@ MethodDesc* TailCallHelp::GetTailCallDispatcherMD() void TailCallHelp::CreateTailCallHelperStubs( MethodDesc* pCallerMD, MethodDesc* pCalleeMD, - MetaSig& callSiteSig, bool virt, bool thisArgByRef, + MetaSig& callSiteSig, bool virt, bool thisArgByRef, bool hasInstArg, MethodDesc** storeArgsStub, bool* storeArgsNeedsTarget, MethodDesc** callTargetStub) { @@ -154,7 +148,7 @@ void TailCallHelp::CreateTailCallHelperStubs( TypeHandle retTyHnd = NormalizeSigType(callSiteSig.GetRetTypeHandleThrowing()); TailCallInfo info(pCallerMD, pCalleeMD, pLoaderAllocator, &callSiteSig, virt, retTyHnd); - LayOutArgBuffer(callSiteSig, pCalleeMD, *storeArgsNeedsTarget, thisArgByRef, &info.ArgBufLayout); + LayOutArgBuffer(callSiteSig, pCalleeMD, *storeArgsNeedsTarget, thisArgByRef, hasInstArg, &info.ArgBufLayout); info.HasGCDescriptor = GenerateGCDescriptor(pCalleeMD, info.ArgBufLayout, &info.GCRefMapBuilder); *storeArgsStub = CreateStoreArgsStub(info); @@ -163,9 +157,9 @@ void TailCallHelp::CreateTailCallHelperStubs( void TailCallHelp::LayOutArgBuffer( MetaSig& callSiteSig, MethodDesc* calleeMD, - bool storeTarget, bool thisArgByRef, ArgBufferLayout* layout) + bool storeTarget, bool thisArgByRef, bool hasInstArg, ArgBufferLayout* layout) { - unsigned int offs = 0; + unsigned int offs = offsetof(TailCallArgBuffer, Args); auto addValue = [&](TypeHandle th) { @@ -194,6 +188,15 @@ void TailCallHelp::LayOutArgBuffer( addValue(thisHnd); } + layout->HasInstArg = hasInstArg; + +#ifndef TARGET_X86 + if (hasInstArg) + { + addValue(TypeHandle(CoreLibBinder::GetElementType(ELEMENT_TYPE_I))); + } +#endif + callSiteSig.Reset(); CorElementType ty; while ((ty = callSiteSig.NextArg()) != ELEMENT_TYPE_END) @@ -203,6 +206,13 @@ void TailCallHelp::LayOutArgBuffer( addValue(tyHnd); } +#ifdef TARGET_X86 + if (hasInstArg) + { + addValue(TypeHandle(CoreLibBinder::GetElementType(ELEMENT_TYPE_I))); + } +#endif + if (storeTarget) { offs = AlignUp(offs, TARGET_POINTER_SIZE); @@ -247,35 +257,49 @@ TypeHandle TailCallHelp::NormalizeSigType(TypeHandle tyHnd) bool TailCallHelp::GenerateGCDescriptor( MethodDesc* pTargetMD, const ArgBufferLayout& layout, GCRefMapBuilder* builder) { - auto writeGCType = [&](unsigned int offset, CorInfoGCType type) + auto writeGCType = [&](unsigned int argPos, CorInfoGCType type) { - _ASSERTE(offset % TARGET_POINTER_SIZE == 0); switch (type) { - case TYPE_GC_REF: builder->WriteToken(offset / TARGET_POINTER_SIZE, GCREFMAP_REF); break; - case TYPE_GC_BYREF: builder->WriteToken(offset / TARGET_POINTER_SIZE, GCREFMAP_INTERIOR); break; + case TYPE_GC_REF: builder->WriteToken(argPos, GCREFMAP_REF); break; + case TYPE_GC_BYREF: builder->WriteToken(argPos, GCREFMAP_INTERIOR); break; case TYPE_GC_NONE: break; default: UNREACHABLE_MSG("Invalid type"); break; } }; + bool reportInstArg = layout.HasInstArg; + CQuickBytes gcPtrs; for (COUNT_T i = 0; i < layout.Values.GetCount(); i++) { const ArgBufferValue& val = layout.Values[i]; + unsigned int argPos = (val.Offset - offsetof(TailCallArgBuffer, Args)) / TARGET_POINTER_SIZE; + TypeHandle tyHnd = val.TyHnd; if (tyHnd.IsValueType()) { if (!tyHnd.GetMethodTable()->ContainsPointers()) + { +#ifndef TARGET_X86 + // The generic instantiation arg is right after this pointer + if (reportInstArg) + { + _ASSERTE(i == 0 || i == 1); + builder->WriteToken(argPos, pTargetMD->RequiresInstMethodDescArg() ? GCREFMAP_METHOD_PARAM : GCREFMAP_TYPE_PARAM); + reportInstArg = false; + } +#endif continue; + } unsigned int numSlots = (tyHnd.GetSize() + TARGET_POINTER_SIZE - 1) / TARGET_POINTER_SIZE; BYTE* ptr = static_cast(gcPtrs.AllocThrows(numSlots)); CEEInfo::getClassGClayoutStatic(tyHnd, ptr); for (unsigned int i = 0; i < numSlots; i++) { - writeGCType(val.Offset + i * TARGET_POINTER_SIZE, (CorInfoGCType)ptr[i]); + writeGCType(argPos + i , (CorInfoGCType)ptr[i]); } continue; @@ -284,9 +308,19 @@ bool TailCallHelp::GenerateGCDescriptor( CorElementType ety = tyHnd.GetSignatureCorElementType(); CorInfoGCType gc = CorTypeInfo::GetGCType(ety); - writeGCType(val.Offset, gc); + writeGCType(argPos, gc); } +#ifdef TARGET_X86 + // The generic instantiation arg is last + if (reportInstArg) + { + const ArgBufferValue& val = layout.Values[layout.Values.GetCount() - 1]; + unsigned int argPos = (val.Offset - offsetof(TailCallArgBuffer, Args)) / TARGET_POINTER_SIZE; + builder->WriteToken(argPos, pTargetMD->RequiresInstMethodDescArg() ? GCREFMAP_METHOD_PARAM : GCREFMAP_TYPE_PARAM); + } +#endif + builder->Flush(); return builder->GetBlobLength() > 0; @@ -331,29 +365,24 @@ MethodDesc* TailCallHelp::CreateStoreArgsStub(TailCallInfo& info) auto emitOffs = [&](UINT offs) { pCode->EmitLDLOC(bufferLcl); - if (offs != 0) - { - pCode->EmitLDC(offs); - pCode->EmitADD(); - } + pCode->EmitLDC(offs); + pCode->EmitADD(); }; - unsigned int argIndex = 0; - for (COUNT_T i = 0; i < info.ArgBufLayout.Values.GetCount(); i++) { const ArgBufferValue& arg = info.ArgBufLayout.Values[i]; CorElementType ty = arg.TyHnd.GetSignatureCorElementType(); emitOffs(arg.Offset); - pCode->EmitLDARG(argIndex++); + pCode->EmitLDARG(i); EmitStoreTyHnd(pCode, arg.TyHnd); } if (info.ArgBufLayout.HasTargetAddress) { emitOffs(info.ArgBufLayout.TargetAddressOffset); - pCode->EmitLDARG(argIndex++); + pCode->EmitLDARG(info.ArgBufLayout.Values.GetCount()); pCode->EmitSTIND_I(); } @@ -447,42 +476,31 @@ MethodDesc* TailCallHelp::CreateCallTargetStub(const TailCallInfo& info) auto emitOffs = [&](UINT offs) { pCode->EmitLDARG(ARG_ARG_BUFFER); - if (offs != 0) - { - pCode->EmitLDC(offs); - pCode->EmitADD(); - } + pCode->EmitLDC(offs); + pCode->EmitADD(); }; - StackSArray argLocals; + // *pTailCallAwareRetAddr = NextCallReturnAddress(); + pCode->EmitLDARG(ARG_PTR_TAILCALL_AWARE_RET_ADDR); + pCode->EmitCALL(METHOD__STUBHELPERS__NEXT_CALL_RETURN_ADDRESS, 0, 1); + pCode->EmitSTIND_I(); + for (COUNT_T i = 0; i < info.ArgBufLayout.Values.GetCount(); i++) { const ArgBufferValue& arg = info.ArgBufLayout.Values[i]; - DWORD argLcl = pCode->NewLocal(LocalDesc(arg.TyHnd)); - argLocals.Append(argLcl); // arg = args->Arg_i emitOffs(arg.Offset); EmitLoadTyHnd(pCode, arg.TyHnd); - pCode->EmitSTLOC(argLcl); - } - - DWORD targetAddrLcl; - if (info.ArgBufLayout.HasTargetAddress) - { - targetAddrLcl = pCode->NewLocal(ELEMENT_TYPE_I); - - emitOffs(info.ArgBufLayout.TargetAddressOffset); - pCode->EmitLDIND_I(); - pCode->EmitSTLOC(targetAddrLcl); } - // RuntimeHelpers.FreeTailCallArgBuffer(); - pCode->EmitCALL(METHOD__RUNTIME_HELPERS__FREE_TAILCALL_ARG_BUFFER, 0, 0); - - // *pTailCallAwareRetAddr = NextCallReturnAddress(); - pCode->EmitLDARG(ARG_PTR_TAILCALL_AWARE_RET_ADDR); - pCode->EmitCALL(METHOD__STUBHELPERS__NEXT_CALL_RETURN_ADDRESS, 0, 1); + // All arguments are loaded on the stack, it is safe to disable the GC reporting of ArgBuffer now. + // This is optimization to avoid extending argument lifetime unnecessarily. + // We still need to report the inst argument of shared generic code to prevent it from being unloaded. The inst + // argument is just a regular IntPtr on the stack. It is safe to stop reporting it only after the target method + // takes over. + pCode->EmitLDARG(ARG_ARG_BUFFER); + pCode->EmitLDC(info.ArgBufLayout.HasInstArg ? TAILCALLARGBUFFER_INSTARG_ONLY : TAILCALLARGBUFFER_ABANDONED); pCode->EmitSTIND_I(); int numRetVals = info.CallSiteSig->IsReturnTypeVoid() ? 0 : 1; @@ -495,23 +513,18 @@ MethodDesc* TailCallHelp::CreateCallTargetStub(const TailCallInfo& info) // the proper MethodRef. _ASSERTE(!info.CallSiteSig->IsVarArg()); - for (COUNT_T i = 0; i < argLocals.GetCount(); i++) - { - pCode->EmitLDLOC(argLocals[i]); - } - if (info.CallSiteIsVirtual) { pCode->EmitCALLVIRT( pCode->GetToken(info.Callee), - static_cast(argLocals.GetCount()), + static_cast(info.ArgBufLayout.Values.GetCount()), numRetVals); } else { pCode->EmitCALL( pCode->GetToken(info.Callee), - static_cast(argLocals.GetCount()), + static_cast(info.ArgBufLayout.Values.GetCount()), numRetVals); } } @@ -538,7 +551,7 @@ MethodDesc* TailCallHelp::CreateCallTargetStub(const TailCallInfo& info) COUNT_T firstSigArg = info.CallSiteSig->HasThis() ? 1 : 0; - for (COUNT_T i = firstSigArg; i < argLocals.GetCount(); i++) + for (COUNT_T i = firstSigArg; i < info.ArgBufLayout.Values.GetCount(); i++) { const ArgBufferValue& val = info.ArgBufLayout.Values[i]; AppendTypeHandle(calliSig, val.TyHnd); @@ -547,16 +560,12 @@ MethodDesc* TailCallHelp::CreateCallTargetStub(const TailCallInfo& info) DWORD cbCalliSig; PCCOR_SIGNATURE pCalliSig = (PCCOR_SIGNATURE)calliSig.GetSignature(&cbCalliSig); - for (COUNT_T i = 0; i < argLocals.GetCount(); i++) - { - pCode->EmitLDLOC(argLocals[i]); - } - - pCode->EmitLDLOC(targetAddrLcl); + emitOffs(info.ArgBufLayout.TargetAddressOffset); + pCode->EmitLDIND_I(); pCode->EmitCALLI( pCode->GetSigToken(pCalliSig, cbCalliSig), - static_cast(argLocals.GetCount()), + static_cast(info.ArgBufLayout.Values.GetCount()), numRetVals); } diff --git a/src/coreclr/src/vm/tailcallhelp.h b/src/coreclr/src/vm/tailcallhelp.h index 327dc3d2e1dc0..32883076a6920 100644 --- a/src/coreclr/src/vm/tailcallhelp.h +++ b/src/coreclr/src/vm/tailcallhelp.h @@ -14,12 +14,11 @@ class TailCallHelp { public: static FCDECL2(void*, AllocTailCallArgBuffer, INT32, void*); - static FCDECL0(void, FreeTailCallArgBuffer); static FCDECL2(void*, GetTailCallInfo, void**, void**); static void CreateTailCallHelperStubs( MethodDesc* pCallerMD, MethodDesc* pCalleeMD, - MetaSig& callSiteSig, bool virt, bool thisArgByRef, + MetaSig& callSiteSig, bool virt, bool thisArgByRef, bool hasInstArg, MethodDesc** storeArgsStub, bool* storeArgsNeedsTarget, MethodDesc** callTargetStub); @@ -29,7 +28,7 @@ class TailCallHelp static void LayOutArgBuffer( MetaSig& callSiteSig, MethodDesc* calleeMD, - bool storeTarget, bool thisArgByRef, ArgBufferLayout* layout); + bool storeTarget, bool thisArgByRef, bool hasInstArg, ArgBufferLayout* layout); static TypeHandle NormalizeSigType(TypeHandle tyHnd); static bool GenerateGCDescriptor(MethodDesc* pTargetMD, const ArgBufferLayout& values, GCRefMapBuilder* builder); diff --git a/src/coreclr/src/vm/threads.cpp b/src/coreclr/src/vm/threads.cpp index 9dae20e9d487a..bc09aaf9aaf63 100644 --- a/src/coreclr/src/vm/threads.cpp +++ b/src/coreclr/src/vm/threads.cpp @@ -61,12 +61,10 @@ TailCallTls::TailCallTls() // so casting away const is ok here. : m_frame(const_cast(&g_sentinelTailCallFrame)) , m_argBuffer(NULL) - , m_argBufferSize(0) - , m_argBufferGCDesc(NULL) { } -void* TailCallTls::AllocArgBuffer(size_t size, void* gcDesc) +TailCallArgBuffer* TailCallTls::AllocArgBuffer(int size, void* gcDesc) { CONTRACTL { @@ -75,42 +73,30 @@ void* TailCallTls::AllocArgBuffer(size_t size, void* gcDesc) } CONTRACTL_END - _ASSERTE(m_argBuffer == NULL); + _ASSERTE(size >= (int)offsetof(TailCallArgBuffer, Args)); - if (size > sizeof(m_argBufferInline)) + if (m_argBuffer != NULL && m_argBuffer->Size < size) { - m_argBuffer = new (nothrow) char[size]; - if (m_argBuffer == NULL) - return NULL; + FreeArgBuffer(); } - else - m_argBuffer = m_argBufferInline; - if (gcDesc != NULL) + if (m_argBuffer == NULL) { - memset(m_argBuffer, 0, size); - m_argBufferGCDesc = gcDesc; + m_argBuffer = (TailCallArgBuffer*)new (nothrow) BYTE[size]; + if (m_argBuffer == NULL) + return NULL; + m_argBuffer->Size = size; } - m_argBufferSize = size; - - return m_argBuffer; -} + m_argBuffer->State = TAILCALLARGBUFFER_ACTIVE; -void TailCallTls::FreeArgBuffer() -{ - CONTRACTL + m_argBuffer->GCDesc = gcDesc; + if (gcDesc != NULL) { - NOTHROW; - GC_NOTRIGGER; + memset(m_argBuffer->Args, 0, size - offsetof(TailCallArgBuffer, Args)); } - CONTRACTL_END - - if (m_argBufferSize > sizeof(m_argBufferInline)) - delete[] m_argBuffer; - m_argBufferGCDesc = NULL; - m_argBuffer = NULL; + return m_argBuffer; } #if defined (_DEBUG_IMPL) || defined(_PREFAST_) @@ -2656,6 +2642,8 @@ Thread::~Thread() delete m_pIBCInfo; } + m_tailCallTls.FreeArgBuffer(); + #ifdef FEATURE_EVENT_TRACE // Destruct the thread local type cache for allocation sampling if(m_pAllLoggedTypes) { diff --git a/src/coreclr/src/vm/threads.h b/src/coreclr/src/vm/threads.h index 03aea2ea03cee..36bf5b3014082 100644 --- a/src/coreclr/src/vm/threads.h +++ b/src/coreclr/src/vm/threads.h @@ -239,6 +239,19 @@ struct ThreadLocalBlock #endif }; +// TailCallArgBuffer states +#define TAILCALLARGBUFFER_ACTIVE 0 +#define TAILCALLARGBUFFER_INSTARG_ONLY 1 +#define TAILCALLARGBUFFER_ABANDONED 2 + +struct TailCallArgBuffer +{ + int State; + int Size; + void* GCDesc; + BYTE Args[1]; +}; + #ifdef CROSSGEN_COMPILE #include "asmconstants.h" @@ -980,18 +993,14 @@ class TailCallTls friend class CoreLibBinder; PortableTailCallFrame* m_frame; - char* m_argBuffer; - size_t m_argBufferSize; - void* m_argBufferGCDesc; - char m_argBufferInline[64]; + TailCallArgBuffer* m_argBuffer; public: TailCallTls(); - void* AllocArgBuffer(size_t size, void* gcDesc); - void FreeArgBuffer(); - char* GetArgBuffer(void** gcDesc) + TailCallArgBuffer* AllocArgBuffer(int size, void* gcDesc); + void FreeArgBuffer() { delete[] (BYTE*)m_argBuffer; m_argBuffer = NULL; } + TailCallArgBuffer* GetArgBuffer() { - *gcDesc = m_argBufferGCDesc; return m_argBuffer; } const PortableTailCallFrame* GetFrame() { return m_frame; }