From e17cc67d1bdaf6652fa247461e0c3701c9c6eebe Mon Sep 17 00:00:00 2001 From: petris Date: Sat, 22 Apr 2023 15:56:55 +0200 Subject: [PATCH 01/18] Transform indirect calls to direct ones in the importer Imports indirect calls as direct ones when the target method is known. Only handles addresses from ldftn as the VM has no way to verify pointers from static readonly fields and crashes on invalid ones. The helpers currently contain a small dead path that I'll soon use when extending the code to also handle delegates. Opening as a draft so that the code can be reviewed while I finish the tests. Fixes #44610 --- src/coreclr/jit/compiler.h | 10 + src/coreclr/jit/importer.cpp | 59 +++++ src/coreclr/jit/importercalls.cpp | 208 +++++++++++++++++- .../JIT/opt/Devirtualization/Indirect.cs | 164 ++++++++++++++ .../JIT/opt/Devirtualization/Indirect.csproj | 11 + 5 files changed, 442 insertions(+), 10 deletions(-) create mode 100644 src/tests/JIT/opt/Devirtualization/Indirect.cs create mode 100644 src/tests/JIT/opt/Devirtualization/Indirect.csproj diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4420a7662a36cc..f1eb5520938ba8 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3830,6 +3830,15 @@ class Compiler bool impCanPInvokeInlineCallSite(BasicBlock* block); void impCheckForPInvokeCall( GenTreeCall* call, CORINFO_METHOD_HANDLE methHnd, CORINFO_SIG_INFO* sig, unsigned mflags, BasicBlock* block); + + enum SigTransform + { + LeaveIntact = 0, + DeleteThis = 1 << 0, + ReplaceRefThis = 1 << 1, + }; + + bool impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, SigTransform transformation); GenTreeCall* impImportIndirectCall(CORINFO_SIG_INFO* sig, const DebugInfo& di = DebugInfo()); void impPopArgsForUnmanagedCall(GenTreeCall* call, CORINFO_SIG_INFO* sig); @@ -3858,6 +3867,7 @@ class Compiler GenTree* impInitClass(CORINFO_RESOLVED_TOKEN* pResolvedToken); + GenTree* impGetNodeFromLocal(GenTree* node); GenTree* impImportStaticReadOnlyField(CORINFO_FIELD_HANDLE field, CORINFO_CLASS_HANDLE ownerCls); GenTree* impImportCnsTreeFromBuffer(uint8_t* buffer, var_types valueType); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index fca9e62a5825e3..e275d641470df4 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3764,6 +3764,65 @@ GenTree* Compiler::impInitClass(CORINFO_RESOLVED_TOKEN* pResolvedToken) return node; } +//------------------------------------------------------------------------ +// impGetNodeFromLocal: Tries to return the node that's assigned +// to the provided local. +// +// Arguments: +// node - GT_LCL_VAR whose value is searched for +// +// Return Value: +// The tree representing the node assigned to the variable when possible, +// nullptr otherwise. +// +GenTree* Compiler::impGetNodeFromLocal(GenTree* node) +{ + assert(node != nullptr); + assert(node->OperIs(GT_LCL_VAR)); + + unsigned lclNum = node->AsLclVarCommon()->GetLclNum(); + + if (lvaTable[lclNum].lvSingleDef == 0) + { + return nullptr; + } + + auto findValue = [&](Statement* stmtList) -> GenTree* { + for (Statement* stmt : StatementList(stmtList)) + { + GenTree* root = stmt->GetRootNode(); + if (root->OperIs(GT_ASG) && root->AsOp()->gtOp1->OperIs(GT_LCL_VAR) && + root->AsOp()->gtOp1->AsLclVarCommon()->GetLclNum() == lclNum) + { + return root->AsOp()->gtOp2; + } + } + return nullptr; + }; + + GenTree* valueNode = findValue(impStmtList); + BasicBlock* bb = fgFirstBB; + while (valueNode == nullptr && bb != nullptr) + { + valueNode = findValue(bb->bbStmtList); + if (valueNode == nullptr && bb->NumSucc(this) == 1) + { + bb = bb->GetSucc(0, this); + } + else + { + bb = nullptr; + } + } + + if (valueNode != nullptr && valueNode->OperIs(GT_LCL_VAR)) + { + return impGetNodeFromLocal(valueNode); + } + + return valueNode; +} + //------------------------------------------------------------------------ // impImportStaticReadOnlyField: Tries to import 'static readonly' field // as a constant if the host type is statically initialized. diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index e570c99e7537e3..c8ca8a306fe916 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -104,13 +104,16 @@ var_types Compiler::impImportCall(OPCODE opcode, bool checkForSmallType = false; bool bIntrinsicImported = false; - CORINFO_SIG_INFO calliSig; + CORINFO_SIG_INFO originalSig; NewCallArg extraArg; - /*------------------------------------------------------------------------- - * First create the call node - */ + // run transformations when instrumenting to not pollute PGO data + bool optimizedOrInstrumented = opts.OptimizationEnabled() || opts.IsInstrumented(); + CORINFO_METHOD_HANDLE replacementMethod = nullptr; + GenTree* newThis = nullptr; + SigTransform sigTransformation = SigTransform::LeaveIntact; + // handle special import cases if (opcode == CEE_CALLI) { if (IsTargetAbi(CORINFO_NATIVEAOT_ABI)) @@ -125,25 +128,102 @@ var_types Compiler::impImportCall(OPCODE opcode, } /* Get the call site sig */ - eeGetSig(pResolvedToken->token, pResolvedToken->tokenScope, pResolvedToken->tokenContext, &calliSig); + eeGetSig(pResolvedToken->token, pResolvedToken->tokenScope, pResolvedToken->tokenContext, &originalSig); - callRetTyp = JITtype2varType(calliSig.retType); + if (!optimizedOrInstrumented) + { + // ignore + } + else if (originalSig.getCallConv() == CORINFO_CALLCONV_DEFAULT) + { + JITDUMP("\n\nimpImportCall trying to import calli as call\n"); + GenTree* fptr = impStackTop().val; + if (fptr->OperIs(GT_LCL_VAR)) + { + JITDUMP("impImportCall trying to import calli as call - trying to substitute LCL_VAR\n"); + GenTree* lclValue = impGetNodeFromLocal(fptr); + if (lclValue != nullptr) + { + fptr = lclValue; + } + } + if (fptr->OperIs(GT_FTN_ADDR)) + { + replacementMethod = fptr->AsFptrVal()->gtFptrMethod; + } + else + { + JITDUMP("impImportCall failed to import calli as call - address node not found\n"); + } + } + else + { + JITDUMP("impImportCall failed to import calli as call - call conv %u is not managed\n", + originalSig.getCallConv()); + } + } + + if (replacementMethod != nullptr) + { + JITDUMP("impImportCall trying to transform call - found target method %s\n", + eeGetMethodName(replacementMethod)); + CORINFO_SIG_INFO methodSig; + CORINFO_CLASS_HANDLE targetClass = info.compCompHnd->getMethodClass(replacementMethod); + info.compCompHnd->getMethodSig(replacementMethod, &methodSig, targetClass); - call = impImportIndirectCall(&calliSig, di); + unsigned replacementFlags = info.compCompHnd->getMethodAttribs(replacementMethod); + + if ((replacementFlags & CORINFO_FLG_PINVOKE) != 0) + { + JITDUMP("impImportCall aborting transformation - found PInvoke\n"); + } + else if (impCanSubstituteSig(&originalSig, &methodSig, sigTransformation)) + { + impPopStack(); + if (newThis != nullptr) + { + assert(sigTransformation == SigTransform::ReplaceRefThis); + CORINFO_CLASS_HANDLE thisCls = NO_CLASS_HANDLE; + info.compCompHnd->getArgType(&methodSig, methodSig.args, &thisCls); + impPushOnStack(newThis, typeInfo(TI_REF, thisCls)); + } + JITDUMP("impImportCall transforming call\n"); + pResolvedToken->hMethod = replacementMethod; + pResolvedToken->hClass = targetClass; + + callInfo->sig = methodSig; + callInfo->hMethod = replacementMethod; + callInfo->methodFlags = replacementFlags; + callInfo->classFlags = info.compCompHnd->getClassAttribs(targetClass); + + return impImportCall(CEE_CALL, pResolvedToken, nullptr, nullptr, + prefixFlags, callInfo, rawILOffset); + } + } + + /*------------------------------------------------------------------------- + * First create the call node + */ + + if (opcode == CEE_CALLI) + { + callRetTyp = JITtype2varType(originalSig.retType); + + call = impImportIndirectCall(&originalSig, di); // We don't know the target method, so we have to infer the flags, or // assume the worst-case. - mflags = (calliSig.callConv & CORINFO_CALLCONV_HASTHIS) ? 0 : CORINFO_FLG_STATIC; + mflags = (originalSig.callConv & CORINFO_CALLCONV_HASTHIS) ? 0 : CORINFO_FLG_STATIC; #ifdef DEBUG if (verbose) { - unsigned structSize = (callRetTyp == TYP_STRUCT) ? eeTryGetClassSize(calliSig.retTypeSigClass) : 0; + unsigned structSize = (callRetTyp == TYP_STRUCT) ? eeTryGetClassSize(originalSig.retTypeSigClass) : 0; printf("\nIn Compiler::impImportCall: opcode is %s, kind=%d, callRetType is %s, structSize is %u\n", opcodeNames[opcode], callInfo->kind, varTypeName(callRetTyp), structSize); } #endif - sig = &calliSig; + sig = &originalSig; } else // (opcode != CEE_CALLI) { @@ -1622,6 +1702,114 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN #endif // FEATURE_MULTIREG_RET } +//----------------------------------------------------------------------------------- +// impCanSubstituteSig: Checks whether it's safe to replace a call with another one. +// This DOES NOT check if the calls are actually compatible, it only checks if their trees are. +// Use ONLY when code will call the method with target sig anyway. +// +// Arguments: +// sourceSig - original call signature +// targetSig - new call signature +// transformation - transformations performed on the original signature +// +// Return Value: +// Whether it's safe to change the IR to call the target method +// +bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, SigTransform transformation) +{ + const SigTransform thisChangeMask = (SigTransform)(SigTransform::DeleteThis | SigTransform::ReplaceRefThis); + assert((transformation & thisChangeMask) != thisChangeMask); + + if (sourceSig->getCallConv() != targetSig->getCallConv()) + { + JITDUMP("impCanSubstituteSig returning false - call conv %u != %u\n", sourceSig->callConv, targetSig->callConv); + return false; + } + + unsigned sourceArgCount = sourceSig->numArgs; + if ((transformation & SigTransform::DeleteThis) != 0) + { + sourceArgCount--; + } + + if (sourceArgCount != targetSig->numArgs) + { + JITDUMP("impCanSubstituteSig returning false - args count %u != %u\n", sourceArgCount, targetSig->numArgs); + return false; + } + + if (sourceSig->retType != targetSig->retType) + { + JITDUMP("impCanSubstituteSig returning false - return type %u != %u\n", + (unsigned)sourceSig->retType, (unsigned)targetSig->retType); + return false; + } + + if (sourceSig->retType == CORINFO_TYPE_VALUECLASS || sourceSig->retType == CORINFO_TYPE_REFANY) + { + ClassLayout* layoutRetA = typGetObjLayout(sourceSig->retTypeClass); + ClassLayout* layoutRetB = typGetObjLayout(targetSig->retTypeClass); + + if (!ClassLayout::AreCompatible(layoutRetA, layoutRetB)) + { + JITDUMP("impCanSubstituteSig returning false - return type %u is incompatible with %u\n", + (unsigned)sourceSig->retType, (unsigned)targetSig->retType); + return false; + } + } + + CORINFO_ARG_LIST_HANDLE sourceArg = sourceSig->args; + CORINFO_ARG_LIST_HANDLE targetArg = targetSig->args; + + assert((transformation & (SigTransform::DeleteThis | SigTransform::ReplaceRefThis)) == 0 || + eeGetArgType(sourceArg, sourceSig) == TYP_REF); + + if ((transformation & SigTransform::DeleteThis) != 0) + { + sourceArg = info.compCompHnd->getArgNext(sourceArg); + } + + if ((transformation & SigTransform::ReplaceRefThis) != 0 && eeGetArgType(targetArg, targetSig) != TYP_REF) + { + JITDUMP("impCanSubstituteSig returning false - this is not TYP_REF\n"); + return false; + } + + for (unsigned i = 0; i < targetSig->numArgs; i++, + sourceArg = info.compCompHnd->getArgNext(sourceArg), + targetArg = info.compCompHnd->getArgNext(targetArg)) + { + var_types sourceType = eeGetArgType(sourceArg, sourceSig); + var_types targetType = eeGetArgType(targetArg, targetSig); + if (sourceType != targetType) + { + JITDUMP("impCanSubstituteSig returning false - parameter %u type %s != %s\n", + i, varTypeName(sourceType), varTypeName(targetType)); + return false; + } + + if (varTypeIsStruct(sourceType) && varTypeIsStruct(targetType)) + { + CORINFO_CLASS_HANDLE sourceClassHnd = NO_CLASS_HANDLE; + CORINFO_CLASS_HANDLE targetClassHnd = NO_CLASS_HANDLE; + info.compCompHnd->getArgType(sourceSig, sourceArg, &sourceClassHnd); + info.compCompHnd->getArgType(targetSig, targetArg, &targetClassHnd); + + ClassLayout* sourceLayout = typGetObjLayout(sourceClassHnd); + ClassLayout* targetLayout = typGetObjLayout(targetClassHnd); + + if (!ClassLayout::AreCompatible(sourceLayout, targetLayout)) + { + JITDUMP("impCanSubstituteSig returning false - parameter %u type %s is inconmpatible with %s\n", + i, varTypeName(sourceType), varTypeName(targetType)); + return false; + } + } + } + + return true; +} + GenTreeCall* Compiler::impImportIndirectCall(CORINFO_SIG_INFO* sig, const DebugInfo& di) { var_types callRetTyp = JITtype2varType(sig->retType); diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.cs b/src/tests/JIT/opt/Devirtualization/Indirect.cs new file mode 100644 index 00000000000000..59691bb0a80ec8 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/Indirect.cs @@ -0,0 +1,164 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; + +public static unsafe class Program +{ + public static int Main() + { + _ = Test.ExecuteCctor(); + Test.Run(); + return 100; + } +} + +public static unsafe class Test +{ + // valid ptrs + private static readonly delegate* Ptr = &A; + + // invalid ptrs + private static readonly delegate* PtrNull = (delegate*)0; + private static readonly delegate* PtrPlus1 = (delegate*)(((nuint)(delegate*)(&A)) + 1); + private static readonly delegate* PtrMinus1 = (delegate*)(((nuint)(delegate*)(&A)) - 1); + private static readonly delegate* PtrPlus16 = (delegate*)(((nuint)(delegate*)(&A)) + 16); + private static readonly delegate* PtrMinus16 = (delegate*)(((nuint)(delegate*)(&A)) - 16); + private static readonly delegate* PtrPlus32 = (delegate*)(((nuint)(delegate*)(&A)) + 32); + private static readonly delegate* PtrMinus32 = (delegate*)(((nuint)(delegate*)(&A)) - 32); + private static readonly delegate* PtrSmall = (delegate*)4096; + private static readonly delegate* PtrDeadBeef = (delegate*)0xDEADBEEFU; + + // valid but failing checks + private static readonly delegate* PtrWrongSig = (delegate*)(delegate*)&C; + + private static int A() => 1; + private static int B() => 2; + private static void C() { } + + [MethodImpl(MethodImplOptions.NoInlining)] + public static delegate* ExecuteCctor() => Ptr; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static void Run() + { + AssertThrowsNullReferenceException(() => ((delegate*)0)()); + AssertThrowsNullReferenceException(() => PtrNull()); + + AreSame(A(), Invoke(() => ((delegate*)&A)())); + AreSame(B(), Invoke(() => ((delegate*)&B)())); + + AreSame(A(), Invoke(() => Ptr())); + + static int CallPtr(delegate* ptr) => ptr(); + AreSame(A(), Invoke(() => CallPtr(&A))); + AreSame(B(), Invoke(() => CallPtr(&B))); + + static delegate* ReturnA() => &A; + AreSame(A(), Invoke(() => ReturnA()())); + static delegate* ReturnAStatic() => Ptr; + AreSame(A(), Invoke(() => ReturnAStatic()())); + + AreSame(A(), Invoke(() => + { + var ptr = (delegate*)&A; + _ = B(); + return ptr(); + })); + AreSame(A(), Invoke(() => + { + var ptr = (delegate*)&A; + _ = NoInline(B()); + return ptr(); + })); + + static int Branch(bool a) + { + delegate* ptr; + if (a) + ptr = &A; + else + ptr = &B; + return ptr(); + } + + AreSame(A(), Invoke(() => Branch(true))); + AreSame(B(), Invoke(() => Branch(false))); + AreSame(A(), Invoke(a => Branch(a), true)); + AreSame(B(), Invoke(a => Branch(a), false)); + + AreSame(A(), Invoke(a => a ? PtrNull() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrPlus1() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrMinus1() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrPlus16() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrMinus16() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrPlus32() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrMinus32() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrSmall() : Ptr(), false)); + AreSame(A(), Invoke(a => a ? PtrDeadBeef() : Ptr(), false)); + + AreSame(A(), Invoke(a => a ? PtrWrongSig() : Ptr(), false)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void Assign(delegate** ptrptr, delegate* ptr) => *ptrptr = ptr; + + AreSame(A(), Invoke(() => + { + delegate* ptr = &A; + Assign(&ptr, &B); + return ptr(); + })); + AreSame(A(), Invoke(() => + { + delegate* ptr; + Assign(&ptr, &B); + ptr = &A; + return ptr(); + })); + + static int ConditionalAddressAssign(bool a) + { + delegate* ptr = &A; + if (a) + Assign(&ptr, &B); + return ptr(); + } + + AreSame(A(), Invoke(() => ConditionalAddressAssign(false))); + AreSame(B(), Invoke(() => ConditionalAddressAssign(true))); + AreSame(A(), Invoke(a => ConditionalAddressAssign(a), false)); + AreSame(B(), Invoke(a => ConditionalAddressAssign(a), true)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static T Invoke(Func action) => action(); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static TRet Invoke(Func action, T value) => action(value); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static T NoInline(T value) => value; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void AreSame(T expected, T actual, [CallerLineNumber] int line = 0) + { + if (!EqualityComparer.Default.Equals(expected, actual)) + throw new InvalidOperationException($"Invalid value, expected {expected}, got {actual} at line {line}"); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void AssertThrowsNullReferenceException(Func a, [CallerLineNumber] int line = 0) + { + try + { + _ = a(); + } + catch (NullReferenceException) + { + return; + } + + throw new InvalidOperationException($"Expected NullReferenceException at line {line}"); + } +} diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.csproj b/src/tests/JIT/opt/Devirtualization/Indirect.csproj new file mode 100644 index 00000000000000..e905aaa56191e0 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/Indirect.csproj @@ -0,0 +1,11 @@ + + + Exe + 1 + True + true + + + + + From ee58f66eb7f9bb0e59031bee872c8e2905007665 Mon Sep 17 00:00:00 2001 From: petris Date: Sat, 22 Apr 2023 16:15:59 +0200 Subject: [PATCH 02/18] Format, add tests for UCO --- src/coreclr/jit/importer.cpp | 4 +- src/coreclr/jit/importercalls.cpp | 50 +++++++++---------- .../JIT/opt/Devirtualization/Indirect.cs | 11 ++++ 3 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e275d641470df4..c9a8dcca61aa9f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3800,8 +3800,8 @@ GenTree* Compiler::impGetNodeFromLocal(GenTree* node) return nullptr; }; - GenTree* valueNode = findValue(impStmtList); - BasicBlock* bb = fgFirstBB; + GenTree* valueNode = findValue(impStmtList); + BasicBlock* bb = fgFirstBB; while (valueNode == nullptr && bb != nullptr) { valueNode = findValue(bb->bbStmtList); diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index c8ca8a306fe916..7c6214b7523781 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -108,10 +108,10 @@ var_types Compiler::impImportCall(OPCODE opcode, NewCallArg extraArg; // run transformations when instrumenting to not pollute PGO data - bool optimizedOrInstrumented = opts.OptimizationEnabled() || opts.IsInstrumented(); - CORINFO_METHOD_HANDLE replacementMethod = nullptr; - GenTree* newThis = nullptr; - SigTransform sigTransformation = SigTransform::LeaveIntact; + bool optimizedOrInstrumented = opts.OptimizationEnabled() || opts.IsInstrumented(); + CORINFO_METHOD_HANDLE replacementMethod = nullptr; + GenTree* newThis = nullptr; + SigTransform sigTransformation = SigTransform::LeaveIntact; // handle special import cases if (opcode == CEE_CALLI) @@ -159,15 +159,15 @@ var_types Compiler::impImportCall(OPCODE opcode, else { JITDUMP("impImportCall failed to import calli as call - call conv %u is not managed\n", - originalSig.getCallConv()); + originalSig.getCallConv()); } } if (replacementMethod != nullptr) { JITDUMP("impImportCall trying to transform call - found target method %s\n", - eeGetMethodName(replacementMethod)); - CORINFO_SIG_INFO methodSig; + eeGetMethodName(replacementMethod)); + CORINFO_SIG_INFO methodSig; CORINFO_CLASS_HANDLE targetClass = info.compCompHnd->getMethodClass(replacementMethod); info.compCompHnd->getMethodSig(replacementMethod, &methodSig, targetClass); @@ -189,15 +189,14 @@ var_types Compiler::impImportCall(OPCODE opcode, } JITDUMP("impImportCall transforming call\n"); pResolvedToken->hMethod = replacementMethod; - pResolvedToken->hClass = targetClass; + pResolvedToken->hClass = targetClass; - callInfo->sig = methodSig; - callInfo->hMethod = replacementMethod; + callInfo->sig = methodSig; + callInfo->hMethod = replacementMethod; callInfo->methodFlags = replacementFlags; - callInfo->classFlags = info.compCompHnd->getClassAttribs(targetClass); + callInfo->classFlags = info.compCompHnd->getClassAttribs(targetClass); - return impImportCall(CEE_CALL, pResolvedToken, nullptr, nullptr, - prefixFlags, callInfo, rawILOffset); + return impImportCall(CEE_CALL, pResolvedToken, nullptr, nullptr, prefixFlags, callInfo, rawILOffset); } } @@ -1715,7 +1714,9 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN // Return Value: // Whether it's safe to change the IR to call the target method // -bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, SigTransform transformation) +bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, + CORINFO_SIG_INFO* targetSig, + SigTransform transformation) { const SigTransform thisChangeMask = (SigTransform)(SigTransform::DeleteThis | SigTransform::ReplaceRefThis); assert((transformation & thisChangeMask) != thisChangeMask); @@ -1740,8 +1741,8 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO if (sourceSig->retType != targetSig->retType) { - JITDUMP("impCanSubstituteSig returning false - return type %u != %u\n", - (unsigned)sourceSig->retType, (unsigned)targetSig->retType); + JITDUMP("impCanSubstituteSig returning false - return type %u != %u\n", (unsigned)sourceSig->retType, + (unsigned)targetSig->retType); return false; } @@ -1753,7 +1754,7 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO if (!ClassLayout::AreCompatible(layoutRetA, layoutRetB)) { JITDUMP("impCanSubstituteSig returning false - return type %u is incompatible with %u\n", - (unsigned)sourceSig->retType, (unsigned)targetSig->retType); + (unsigned)sourceSig->retType, (unsigned)targetSig->retType); return false; } } @@ -1762,7 +1763,7 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO CORINFO_ARG_LIST_HANDLE targetArg = targetSig->args; assert((transformation & (SigTransform::DeleteThis | SigTransform::ReplaceRefThis)) == 0 || - eeGetArgType(sourceArg, sourceSig) == TYP_REF); + eeGetArgType(sourceArg, sourceSig) == TYP_REF); if ((transformation & SigTransform::DeleteThis) != 0) { @@ -1775,16 +1776,15 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO return false; } - for (unsigned i = 0; i < targetSig->numArgs; i++, - sourceArg = info.compCompHnd->getArgNext(sourceArg), - targetArg = info.compCompHnd->getArgNext(targetArg)) + for (unsigned i = 0; i < targetSig->numArgs; + i++, sourceArg = info.compCompHnd->getArgNext(sourceArg), targetArg = info.compCompHnd->getArgNext(targetArg)) { var_types sourceType = eeGetArgType(sourceArg, sourceSig); var_types targetType = eeGetArgType(targetArg, targetSig); if (sourceType != targetType) { - JITDUMP("impCanSubstituteSig returning false - parameter %u type %s != %s\n", - i, varTypeName(sourceType), varTypeName(targetType)); + JITDUMP("impCanSubstituteSig returning false - parameter %u type %s != %s\n", i, varTypeName(sourceType), + varTypeName(targetType)); return false; } @@ -1800,8 +1800,8 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO if (!ClassLayout::AreCompatible(sourceLayout, targetLayout)) { - JITDUMP("impCanSubstituteSig returning false - parameter %u type %s is inconmpatible with %s\n", - i, varTypeName(sourceType), varTypeName(targetType)); + JITDUMP("impCanSubstituteSig returning false - parameter %u type %s is inconmpatible with %s\n", i, + varTypeName(sourceType), varTypeName(targetType)); return false; } } diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.cs b/src/tests/JIT/opt/Devirtualization/Indirect.cs index 59691bb0a80ec8..2f6922aef4fb67 100644 --- a/src/tests/JIT/opt/Devirtualization/Indirect.cs +++ b/src/tests/JIT/opt/Devirtualization/Indirect.cs @@ -37,6 +37,14 @@ public static unsafe class Test private static int B() => 2; private static void C() { } + private static int D() => 3; + [UnmanagedCallersOnly] + private static int UnmanagedDefault() => D(); + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] + private static int UnmanagedCdecl() => D(); + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })] + private static int UnmanagedStdcall() => D(); + [MethodImpl(MethodImplOptions.NoInlining)] public static delegate* ExecuteCctor() => Ptr; @@ -48,6 +56,9 @@ public static void Run() AreSame(A(), Invoke(() => ((delegate*)&A)())); AreSame(B(), Invoke(() => ((delegate*)&B)())); + AreSame(D(), Invoke(() => ((delegate* unmanaged)&UnmanagedDefault)())); + AreSame(D(), Invoke(() => ((delegate* unmanaged[Cdecl])&UnmanagedCdecl)())); + AreSame(D(), Invoke(() => ((delegate* unmanaged[Stdcall])&UnmanagedStdcall)())); AreSame(A(), Invoke(() => Ptr())); From 699cf4a094336e312e9dd175a65f3049adcccaf8 Mon Sep 17 00:00:00 2001 From: petris Date: Sat, 22 Apr 2023 16:22:05 +0200 Subject: [PATCH 03/18] Fix warning --- src/coreclr/jit/importercalls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 7c6214b7523781..77271b999c0421 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -104,7 +104,7 @@ var_types Compiler::impImportCall(OPCODE opcode, bool checkForSmallType = false; bool bIntrinsicImported = false; - CORINFO_SIG_INFO originalSig; + CORINFO_SIG_INFO originalSig = {}; NewCallArg extraArg; // run transformations when instrumenting to not pollute PGO data From da5831d910d8bbbb3aa23a00ed540b30551a4ab6 Mon Sep 17 00:00:00 2001 From: petris Date: Sat, 22 Apr 2023 19:57:01 +0200 Subject: [PATCH 04/18] Spill unmanaged pointers --- src/coreclr/jit/importercalls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 77271b999c0421..7d9a9735cfbf70 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -158,7 +158,7 @@ var_types Compiler::impImportCall(OPCODE opcode, } else { - JITDUMP("impImportCall failed to import calli as call - call conv %u is not managed\n", + JITDUMP("\n\nimpImportCall failed to import calli as call - call conv %u is not managed\n", originalSig.getCallConv()); } } From 3e9f88b41f403c786a37fdc4b92e8b0dc48f6e0f Mon Sep 17 00:00:00 2001 From: petris Date: Sat, 22 Apr 2023 23:34:37 +0200 Subject: [PATCH 05/18] Try to reduce diffs --- src/coreclr/jit/importer.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index c9a8dcca61aa9f..5b942b465174d5 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1873,16 +1873,16 @@ bool Compiler::impSpillStackEntry(unsigned level, { CORINFO_CLASS_HANDLE stkHnd = verCurrentState.esStack[level].seTypeInfo.GetClassHandle(); lvaSetClass(tnum, tree, stkHnd); - } - // If we're assigning a GT_RET_EXPR, note the temp over on the call, - // so the inliner can use it in case it needs a return spill temp. - if (tree->OperGet() == GT_RET_EXPR) - { - JITDUMP("\n*** see V%02u = GT_RET_EXPR, noting temp\n", tnum); - GenTree* call = tree->AsRetExpr()->gtInlineCandidate; - InlineCandidateInfo* ici = call->AsCall()->gtInlineCandidateInfo; - ici->preexistingSpillTemp = tnum; + // If we're assigning a GT_RET_EXPR, note the temp over on the call, + // so the inliner can use it in case it needs a return spill temp. + if (tree->OperGet() == GT_RET_EXPR) + { + JITDUMP("\n*** see V%02u = GT_RET_EXPR, noting temp\n", tnum); + GenTree* call = tree->AsRetExpr()->gtInlineCandidate; + InlineCandidateInfo* ici = call->AsCall()->gtInlineCandidateInfo; + ici->preexistingSpillTemp = tnum; + } } } From 5d626a1e5f85fa1f4e9290c9dd26b626415f4385 Mon Sep 17 00:00:00 2001 From: petris Date: Sun, 23 Apr 2023 03:15:17 +0200 Subject: [PATCH 06/18] Revert change --- src/coreclr/jit/importer.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5b942b465174d5..c9a8dcca61aa9f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1873,16 +1873,16 @@ bool Compiler::impSpillStackEntry(unsigned level, { CORINFO_CLASS_HANDLE stkHnd = verCurrentState.esStack[level].seTypeInfo.GetClassHandle(); lvaSetClass(tnum, tree, stkHnd); + } - // If we're assigning a GT_RET_EXPR, note the temp over on the call, - // so the inliner can use it in case it needs a return spill temp. - if (tree->OperGet() == GT_RET_EXPR) - { - JITDUMP("\n*** see V%02u = GT_RET_EXPR, noting temp\n", tnum); - GenTree* call = tree->AsRetExpr()->gtInlineCandidate; - InlineCandidateInfo* ici = call->AsCall()->gtInlineCandidateInfo; - ici->preexistingSpillTemp = tnum; - } + // If we're assigning a GT_RET_EXPR, note the temp over on the call, + // so the inliner can use it in case it needs a return spill temp. + if (tree->OperGet() == GT_RET_EXPR) + { + JITDUMP("\n*** see V%02u = GT_RET_EXPR, noting temp\n", tnum); + GenTree* call = tree->AsRetExpr()->gtInlineCandidate; + InlineCandidateInfo* ici = call->AsCall()->gtInlineCandidateInfo; + ici->preexistingSpillTemp = tnum; } } From 4903e3af86c55bde9c6498f4e86d1234904bfe47 Mon Sep 17 00:00:00 2001 From: petris Date: Sun, 23 Apr 2023 22:35:32 +0200 Subject: [PATCH 07/18] Fix assert --- src/coreclr/jit/importercalls.cpp | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 7d9a9735cfbf70..dd8ac0eea5c367 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1718,24 +1718,29 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, SigTransform transformation) { - const SigTransform thisChangeMask = (SigTransform)(SigTransform::DeleteThis | SigTransform::ReplaceRefThis); - assert((transformation & thisChangeMask) != thisChangeMask); - if (sourceSig->getCallConv() != targetSig->getCallConv()) { JITDUMP("impCanSubstituteSig returning false - call conv %u != %u\n", sourceSig->callConv, targetSig->callConv); return false; } - unsigned sourceArgCount = sourceSig->numArgs; + if (sourceSig->hasExplicitThis() || targetSig->hasExplicitThis()) + { + JITDUMP("impCanSubstituteSig returning false - explicit this\n"); + return false; + } + + unsigned sourceArgCount = sourceSig->totalILArgs(); if ((transformation & SigTransform::DeleteThis) != 0) { + assert((transformation & SigTransform::ReplaceRefThis) == 0); + assert(sourceSig->HasThis()); sourceArgCount--; } - if (sourceArgCount != targetSig->numArgs) + if (sourceArgCount != targetSig->totalILArgs()) { - JITDUMP("impCanSubstituteSig returning false - args count %u != %u\n", sourceArgCount, targetSig->numArgs); + JITDUMP("impCanSubstituteSig returning false - args count %u != %u\n", sourceArgCount, targetSig->totalILArgs()); return false; } @@ -1762,20 +1767,6 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_ARG_LIST_HANDLE sourceArg = sourceSig->args; CORINFO_ARG_LIST_HANDLE targetArg = targetSig->args; - assert((transformation & (SigTransform::DeleteThis | SigTransform::ReplaceRefThis)) == 0 || - eeGetArgType(sourceArg, sourceSig) == TYP_REF); - - if ((transformation & SigTransform::DeleteThis) != 0) - { - sourceArg = info.compCompHnd->getArgNext(sourceArg); - } - - if ((transformation & SigTransform::ReplaceRefThis) != 0 && eeGetArgType(targetArg, targetSig) != TYP_REF) - { - JITDUMP("impCanSubstituteSig returning false - this is not TYP_REF\n"); - return false; - } - for (unsigned i = 0; i < targetSig->numArgs; i++, sourceArg = info.compCompHnd->getArgNext(sourceArg), targetArg = info.compCompHnd->getArgNext(targetArg)) { From 7e9cdb43563c7792f40efb6cf532430d528ef6b3 Mon Sep 17 00:00:00 2001 From: petris Date: Sun, 23 Apr 2023 22:44:09 +0200 Subject: [PATCH 08/18] Fix typo --- src/coreclr/jit/importercalls.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index dd8ac0eea5c367..fc3f6612311441 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1734,7 +1734,7 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, if ((transformation & SigTransform::DeleteThis) != 0) { assert((transformation & SigTransform::ReplaceRefThis) == 0); - assert(sourceSig->HasThis()); + assert(sourceSig->hasThis()); sourceArgCount--; } From e081790470b253b3c9041f166ae4045370eab44f Mon Sep 17 00:00:00 2001 From: petris Date: Mon, 24 Apr 2023 23:16:08 +0200 Subject: [PATCH 09/18] Fix the this handling logic --- src/coreclr/jit/compiler.h | 9 +---- src/coreclr/jit/importercalls.cpp | 55 ++++++++++++++++++++++++------- 2 files changed, 45 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index f1eb5520938ba8..a1e152f7161d87 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3831,14 +3831,7 @@ class Compiler void impCheckForPInvokeCall( GenTreeCall* call, CORINFO_METHOD_HANDLE methHnd, CORINFO_SIG_INFO* sig, unsigned mflags, BasicBlock* block); - enum SigTransform - { - LeaveIntact = 0, - DeleteThis = 1 << 0, - ReplaceRefThis = 1 << 1, - }; - - bool impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, SigTransform transformation); + bool impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, var_types sourceThis, var_types targetThis); GenTreeCall* impImportIndirectCall(CORINFO_SIG_INFO* sig, const DebugInfo& di = DebugInfo()); void impPopArgsForUnmanagedCall(GenTreeCall* call, CORINFO_SIG_INFO* sig); diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index fc3f6612311441..713035c5feee9c 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -111,7 +111,8 @@ var_types Compiler::impImportCall(OPCODE opcode, bool optimizedOrInstrumented = opts.OptimizationEnabled() || opts.IsInstrumented(); CORINFO_METHOD_HANDLE replacementMethod = nullptr; GenTree* newThis = nullptr; - SigTransform sigTransformation = SigTransform::LeaveIntact; + var_types oldThis = TYP_UNDEF; + var_types newThis = TYP_UNDEF; // handle special import cases if (opcode == CEE_CALLI) @@ -177,12 +178,13 @@ var_types Compiler::impImportCall(OPCODE opcode, { JITDUMP("impImportCall aborting transformation - found PInvoke\n"); } - else if (impCanSubstituteSig(&originalSig, &methodSig, sigTransformation)) + else if (impCanSubstituteSig(&originalSig, &methodSig, oldThis, newThis)) { impPopStack(); if (newThis != nullptr) { - assert(sigTransformation == SigTransform::ReplaceRefThis); + assert(oldThis == TYP_REF); + assert(newThis == TYP_REF); CORINFO_CLASS_HANDLE thisCls = NO_CLASS_HANDLE; info.compCompHnd->getArgType(&methodSig, methodSig.args, &thisCls); impPushOnStack(newThis, typeInfo(TI_REF, thisCls)); @@ -1716,31 +1718,37 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN // bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, - SigTransform transformation) + var_types sourceThis, + var_types targetThis) { + assert(sourceSig->hasThisNonExplicit() || sourceThis == TYP_UNDEF); + assert(targetSig->hasThisNonExplicit() || targetThis == TYP_UNDEF || targetThis == TYP_VOID); + assert(!targetSig->hasThisNonExplicit() || targetThis != TYP_VOID); + assert(sourceThis != TYP_VOID); + if (sourceSig->getCallConv() != targetSig->getCallConv()) { JITDUMP("impCanSubstituteSig returning false - call conv %u != %u\n", sourceSig->callConv, targetSig->callConv); return false; } - if (sourceSig->hasExplicitThis() || targetSig->hasExplicitThis()) + if (sourceSig->hasThisNonExplicit() && sourceThis == TYP_UNDEF || targetSig->hasThisNonExplicit() && targetThis == TYP_UNDEF) { - JITDUMP("impCanSubstituteSig returning false - explicit this\n"); + JITDUMP("impCanSubstituteSig returning false - unknown this type\n"); return false; } unsigned sourceArgCount = sourceSig->totalILArgs(); - if ((transformation & SigTransform::DeleteThis) != 0) + if (targetThis == TYP_VOID) { - assert((transformation & SigTransform::ReplaceRefThis) == 0); - assert(sourceSig->hasThis()); + assert(sourceSig->hasThisNonExplicit() && sourceThis == TYP_REF); sourceArgCount--; } if (sourceArgCount != targetSig->totalILArgs()) { - JITDUMP("impCanSubstituteSig returning false - args count %u != %u\n", sourceArgCount, targetSig->totalILArgs()); + JITDUMP("impCanSubstituteSig returning false - args count %u != %u\n", sourceArgCount, + targetSig->totalILArgs()); return false; } @@ -1767,7 +1775,32 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_ARG_LIST_HANDLE sourceArg = sourceSig->args; CORINFO_ARG_LIST_HANDLE targetArg = targetSig->args; - for (unsigned i = 0; i < targetSig->numArgs; + unsigned numArgs = targetSig->totalILArgs(); + + if (sourceSig->hasThisNonExplicit() && targetThis != TYP_VOID || targetSig->hasThisNonExplicit()) + { + if (sourceThis == TYP_UNDEF) + { + sourceThis = eeGetArgType(sourceArg, sourceSig); + sourceArg = info.compCompHnd->getArgNext(sourceArg); + numArgs--; + } + else + { + targetThis = eeGetArgType(targetArg, targetSig); + targetArg = info.compCompHnd->getArgNext(targetArg); + } + assert(sourceThis == TYP_REF || sourceThis == TYP_BYREF || sourceThis == TYP_I_IMPL); + assert(targetThis == TYP_REF || targetThis == TYP_BYREF || targetThis == TYP_I_IMPL); + if (sourceThis != targetThis) + { + JITDUMP("impCanSubstituteSig returning false - this type %s != %s\n", i, varTypeName(sourceType), + varTypeName(targetType)); + return false; + } + } + + for (unsigned i = 0; i < numArgs; i++, sourceArg = info.compCompHnd->getArgNext(sourceArg), targetArg = info.compCompHnd->getArgNext(targetArg)) { var_types sourceType = eeGetArgType(sourceArg, sourceSig); From 9c6c2c2d31756cac25e615da40e89ec3e90e61eb Mon Sep 17 00:00:00 2001 From: petris Date: Mon, 24 Apr 2023 23:48:35 +0200 Subject: [PATCH 10/18] Add IL tests, fix build --- src/coreclr/jit/importercalls.cpp | 10 +- .../JIT/opt/Devirtualization/Indirect.cs | 17 +- .../JIT/opt/Devirtualization/Indirect.csproj | 5 +- .../JIT/opt/Devirtualization/IndirectIL.il | 270 ++++++++++++++++++ .../opt/Devirtualization/IndirectIL.ilproj | 11 + 5 files changed, 303 insertions(+), 10 deletions(-) create mode 100644 src/tests/JIT/opt/Devirtualization/IndirectIL.il create mode 100644 src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 713035c5feee9c..53c9c3690fe90b 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -112,7 +112,7 @@ var_types Compiler::impImportCall(OPCODE opcode, CORINFO_METHOD_HANDLE replacementMethod = nullptr; GenTree* newThis = nullptr; var_types oldThis = TYP_UNDEF; - var_types newThis = TYP_UNDEF; + var_types targetThis = TYP_UNDEF; // handle special import cases if (opcode == CEE_CALLI) @@ -178,13 +178,13 @@ var_types Compiler::impImportCall(OPCODE opcode, { JITDUMP("impImportCall aborting transformation - found PInvoke\n"); } - else if (impCanSubstituteSig(&originalSig, &methodSig, oldThis, newThis)) + else if (impCanSubstituteSig(&originalSig, &methodSig, oldThis, targetThis)) { impPopStack(); if (newThis != nullptr) { assert(oldThis == TYP_REF); - assert(newThis == TYP_REF); + assert(targetThis == TYP_REF); CORINFO_CLASS_HANDLE thisCls = NO_CLASS_HANDLE; info.compCompHnd->getArgType(&methodSig, methodSig.args, &thisCls); impPushOnStack(newThis, typeInfo(TI_REF, thisCls)); @@ -1794,8 +1794,8 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, assert(targetThis == TYP_REF || targetThis == TYP_BYREF || targetThis == TYP_I_IMPL); if (sourceThis != targetThis) { - JITDUMP("impCanSubstituteSig returning false - this type %s != %s\n", i, varTypeName(sourceType), - varTypeName(targetType)); + JITDUMP("impCanSubstituteSig returning false - this type %s != %s\n", varTypeName(sourceThis), + varTypeName(targetThis)); return false; } } diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.cs b/src/tests/JIT/opt/Devirtualization/Indirect.cs index 2f6922aef4fb67..1ea64c158e5893 100644 --- a/src/tests/JIT/opt/Devirtualization/Indirect.cs +++ b/src/tests/JIT/opt/Devirtualization/Indirect.cs @@ -136,10 +136,19 @@ static int ConditionalAddressAssign(bool a) return ptr(); } - AreSame(A(), Invoke(() => ConditionalAddressAssign(false))); - AreSame(B(), Invoke(() => ConditionalAddressAssign(true))); - AreSame(A(), Invoke(a => ConditionalAddressAssign(a), false)); - AreSame(B(), Invoke(a => ConditionalAddressAssign(a), true)); + AreSame(2, IndirectIL.StaticClass()); + AreSame(1, IndirectIL.InstanceClass()); + AreSame(1, IndirectIL.InstanceExplicitClass()); + AreSame(4, IndirectIL.StaticClassParam()); + AreSame(3, IndirectIL.InstanceClassParam()); + AreSame(3, IndirectIL.InstanceExplicitClassParam()); + + AreSame(2, IndirectIL.StaticStruct()); + AreSame(1, IndirectIL.InstanceStruct()); + AreSame(1, IndirectIL.InstanceExplicitStruct()); + AreSame(4, IndirectIL.StaticStructParam()); + AreSame(3, IndirectIL.InstanceStructParam()); + AreSame(3, IndirectIL.InstanceExplicitStructParam()); } [MethodImpl(MethodImplOptions.NoInlining)] diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.csproj b/src/tests/JIT/opt/Devirtualization/Indirect.csproj index e905aaa56191e0..551841b652015a 100644 --- a/src/tests/JIT/opt/Devirtualization/Indirect.csproj +++ b/src/tests/JIT/opt/Devirtualization/Indirect.csproj @@ -6,6 +6,9 @@ true - + + + + diff --git a/src/tests/JIT/opt/Devirtualization/IndirectIL.il b/src/tests/JIT/opt/Devirtualization/IndirectIL.il new file mode 100644 index 00000000000000..1da2be6da11b4e --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/IndirectIL.il @@ -0,0 +1,270 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern System.Runtime { .publickeytoken = (B0 3F 5F 7F 11 D5 0A 3A ) } + +.assembly IndirectIL { } + +.class public abstract auto ansi sealed beforefieldinit ILTests + extends [System.Runtime]System.Object +{ + .method public hidebysig static int32 StaticClass() cil managed noinlining aggressiveoptimization + { + // Code size 12 (0xc) + .maxstack 8 + IL_0000: ldftn int32 Cl::B() + IL_0006: calli int32() + IL_000b: ret + } // end of method ILTests::A + + .method public hidebysig static int32 InstanceClass() cil managed noinlining aggressiveoptimization + { + // Code size 17 (0x11) + .maxstack 2 + .locals init (class Cl V_0) + IL_0000: newobj instance void Cl::.ctor() + IL_0005: ldftn instance int32 Cl::A() + IL_000b: calli instance int32() + IL_0010: ret + } // end of method ILTests::B + + .method public hidebysig static int32 InstanceExplicitClass() cil managed noinlining aggressiveoptimization + { + // Code size 17 (0x11) + .maxstack 2 + .locals init (class Cl V_0) + IL_0000: newobj instance void Cl::.ctor() + IL_0005: ldftn instance int32 Cl::A() + IL_000b: calli explicit instance int32(class Cl) + IL_0010: ret + } // end of method ILTests::C + + .method public hidebysig static int32 StaticClassParam() cil managed noinlining aggressiveoptimization + { + // Code size 15 (0xf) + .maxstack 2 + .locals init (method int32 *(int32) V_0) + IL_0000: ldftn int32 Cl::D(int32) + IL_0006: stloc.0 + IL_0007: ldc.i4.0 + IL_0008: ldloc.0 + IL_0009: calli int32(int32) + IL_000e: ret + } // end of method ILTests::D + + .method public hidebysig static int32 InstanceClassParam() cil managed noinlining aggressiveoptimization + { + // Code size 22 (0x16) + .maxstack 3 + .locals init (class Cl V_0, + method int32 *(int32) V_1) + IL_0000: newobj instance void Cl::.ctor() + IL_0005: stloc.0 + IL_0006: ldftn instance int32 Cl::C(int32) + IL_000c: stloc.1 + IL_000d: ldloc.0 + IL_000e: ldc.i4.0 + IL_000f: ldloc.1 + IL_0010: calli instance int32(int32) + IL_0015: ret + } // end of method ILTests::E + + .method public hidebysig static int32 InstanceExplicitClassParam() cil managed noinlining aggressiveoptimization + { + // Code size 22 (0x16) + .maxstack 3 + .locals init (class Cl V_0, + method int32 *(int32) V_1) + IL_0000: newobj instance void Cl::.ctor() + IL_0005: stloc.0 + IL_0006: ldftn instance int32 Cl::C(int32) + IL_000c: stloc.1 + IL_000d: ldloc.0 + IL_000e: ldc.i4.0 + IL_000f: ldloc.1 + IL_0010: calli explicit instance int32(class Cl,int32) + IL_0015: ret + } // end of method ILTests::F + + .method public hidebysig static int32 StaticStruct() cil managed noinlining aggressiveoptimization + { + // Code size 12 (0xc) + .maxstack 8 + IL_0000: ldftn int32 St::B() + IL_0006: calli int32() + IL_000b: ret + } // end of method ILTests::G + + .method public hidebysig static int32 InstanceStruct() cil managed noinlining aggressiveoptimization + { + // Code size 22 (0x16) + .maxstack 2 + .locals init (valuetype St V_0) + IL_0000: ldloca.s V_0 + IL_0002: initobj St + IL_0008: ldloca.s V_0 + IL_000a: ldftn instance int32 St::A() + IL_0010: calli instance int32() + IL_0015: ret + } // end of method ILTests::H + + .method public hidebysig static int32 InstanceExplicitStruct() cil managed noinlining aggressiveoptimization + { + // Code size 22 (0x16) + .maxstack 2 + .locals init (valuetype St V_0) + IL_0000: ldloca.s V_0 + IL_0002: initobj St + IL_0008: ldloca.s V_0 + IL_000a: ldftn instance int32 St::A() + IL_0010: calli explicit instance int32(valuetype St&) + IL_0015: ret + } // end of method ILTests::I + + .method public hidebysig static int32 StaticStructParam() cil managed noinlining aggressiveoptimization + { + // Code size 15 (0xf) + .maxstack 2 + .locals init (method int32 *(int32) V_0) + IL_0000: ldftn int32 St::D(int32) + IL_0006: stloc.0 + IL_0007: ldc.i4.0 + IL_0008: ldloc.0 + IL_0009: calli int32(int32) + IL_000e: ret + } // end of method ILTests::J + + .method public hidebysig static int32 InstanceStructParam() cil managed noinlining aggressiveoptimization + { + // Code size 25 (0x19) + .maxstack 3 + .locals init (valuetype St V_0, + method int32 *(int32) V_1) + IL_0000: ldloca.s V_0 + IL_0002: initobj St + IL_0008: ldftn instance int32 St::C(int32) + IL_000e: stloc.1 + IL_000f: ldloca.s V_0 + IL_0011: ldc.i4.0 + IL_0012: ldloc.1 + IL_0013: calli instance int32(int32) + IL_0018: ret + } // end of method ILTests::K + + .method public hidebysig static int32 InstanceExplicitStructParam() cil managed noinlining aggressiveoptimization + { + // Code size 25 (0x19) + .maxstack 3 + .locals init (valuetype St V_0, + method int32 *(int32) V_1) + IL_0000: ldloca.s V_0 + IL_0002: initobj St + IL_0008: ldftn instance int32 St::C(int32) + IL_000e: stloc.1 + IL_000f: ldloca.s V_0 + IL_0011: ldc.i4.0 + IL_0012: ldloc.1 + IL_0013: calli explicit instance int32(valuetype St&,int32) + IL_0018: ret + } // end of method ILTests::L + +} // end of class ILTests + +.class public auto ansi sealed beforefieldinit Cl + extends [System.Runtime]System.Object +{ + .method public hidebysig instance int32 + A() cil managed aggressiveinlining aggressiveoptimization + { + // Code size 2 (0x2) + .maxstack 8 + IL_0000: ldc.i4.1 + IL_0001: ret + } // end of method Cl::A + + .method public hidebysig static int32 B() cil managed aggressiveinlining aggressiveoptimization + { + // Code size 2 (0x2) + .maxstack 8 + IL_0000: ldc.i4.2 + IL_0001: ret + } // end of method Cl::B + + .method public hidebysig instance int32 + C(int32 a) cil managed aggressiveinlining aggressiveoptimization + { + // Code size 4 (0x4) + .maxstack 8 + IL_0000: ldc.i4.3 + IL_0001: ldarg.1 + IL_0002: add + IL_0003: ret + } // end of method Cl::C + + .method public hidebysig static int32 D(int32 a) cil managed aggressiveinlining aggressiveoptimization + { + // Code size 4 (0x4) + .maxstack 8 + IL_0000: ldc.i4.4 + IL_0001: ldarg.0 + IL_0002: add + IL_0003: ret + } // end of method Cl::D + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 7 (0x7) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: call instance void [System.Runtime]System.Object::.ctor() + IL_0006: ret + } // end of method Cl::.ctor + +} // end of class Cl + +.class public sequential ansi sealed beforefieldinit St + extends [System.Runtime]System.ValueType +{ + .pack 0 + .size 1 + .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsReadOnlyAttribute::.ctor() = ( 01 00 00 00 ) + .method public hidebysig instance int32 + A() cil managed aggressiveinlining aggressiveoptimization + { + // Code size 2 (0x2) + .maxstack 8 + IL_0000: ldc.i4.1 + IL_0001: ret + } // end of method St::A + + .method public hidebysig static int32 B() cil managed aggressiveinlining aggressiveoptimization + { + // Code size 2 (0x2) + .maxstack 8 + IL_0000: ldc.i4.2 + IL_0001: ret + } // end of method St::B + + .method public hidebysig instance int32 + C(int32 a) cil managed aggressiveinlining aggressiveoptimization + { + // Code size 4 (0x4) + .maxstack 8 + IL_0000: ldc.i4.3 + IL_0001: ldarg.1 + IL_0002: add + IL_0003: ret + } // end of method St::C + + .method public hidebysig static int32 D(int32 a) cil managed aggressiveinlining aggressiveoptimization + { + // Code size 4 (0x4) + .maxstack 8 + IL_0000: ldc.i4.4 + IL_0001: ldarg.0 + IL_0002: add + IL_0003: ret + } // end of method St::D + +} // end of class St diff --git a/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj b/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj new file mode 100644 index 00000000000000..d404d7884c8f29 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj @@ -0,0 +1,11 @@ + + + Library + true + BuildOnly + false + + + + + From 548e88a8f3d08cb68090169d9691a0e3b1a0f08e Mon Sep 17 00:00:00 2001 From: petris Date: Mon, 24 Apr 2023 23:54:24 +0200 Subject: [PATCH 11/18] Fix warning --- src/coreclr/jit/importercalls.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 53c9c3690fe90b..42c59658599f65 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -1732,7 +1732,7 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, return false; } - if (sourceSig->hasThisNonExplicit() && sourceThis == TYP_UNDEF || targetSig->hasThisNonExplicit() && targetThis == TYP_UNDEF) + if ((sourceSig->hasThisNonExplicit() && sourceThis == TYP_UNDEF) || (targetSig->hasThisNonExplicit() && targetThis == TYP_UNDEF)) { JITDUMP("impCanSubstituteSig returning false - unknown this type\n"); return false; @@ -1777,7 +1777,7 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, unsigned numArgs = targetSig->totalILArgs(); - if (sourceSig->hasThisNonExplicit() && targetThis != TYP_VOID || targetSig->hasThisNonExplicit()) + if ((sourceSig->hasThisNonExplicit() && targetThis != TYP_VOID) || targetSig->hasThisNonExplicit()) { if (sourceThis == TYP_UNDEF) { From ea96e2279433f5b860480a8e293350b27648fdd7 Mon Sep 17 00:00:00 2001 From: petris Date: Tue, 25 Apr 2023 01:51:33 +0200 Subject: [PATCH 12/18] Fetch target type --- src/coreclr/jit/importercalls.cpp | 5 +++++ src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 42c59658599f65..246a3e354ef864 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -172,6 +172,11 @@ var_types Compiler::impImportCall(OPCODE opcode, CORINFO_CLASS_HANDLE targetClass = info.compCompHnd->getMethodClass(replacementMethod); info.compCompHnd->getMethodSig(replacementMethod, &methodSig, targetClass); + if (methodSig.hasThisNonExplicit() && targetThis == TYP_UNDEF) + { + targetThis = eeIsValueClass(targetClass) ? TYP_BYREF : TYP_REF; + } + unsigned replacementFlags = info.compCompHnd->getMethodAttribs(replacementMethod); if ((replacementFlags & CORINFO_FLG_PINVOKE) != 0) diff --git a/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj b/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj index d404d7884c8f29..05fbf28a4e307c 100644 --- a/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj +++ b/src/tests/JIT/opt/Devirtualization/IndirectIL.ilproj @@ -1,4 +1,4 @@ - + Library true From 3e1f208d85a16234f528fc1dad922673e392e941 Mon Sep 17 00:00:00 2001 From: petris Date: Tue, 25 Apr 2023 20:51:53 +0200 Subject: [PATCH 13/18] Reformat, add tests --- src/coreclr/jit/importercalls.cpp | 27 +++--- .../JIT/opt/Devirtualization/Indirect.cs | 88 +++++++++++++++++++ 2 files changed, 102 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 246a3e354ef864..fc3d53020d73e0 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -111,8 +111,8 @@ var_types Compiler::impImportCall(OPCODE opcode, bool optimizedOrInstrumented = opts.OptimizationEnabled() || opts.IsInstrumented(); CORINFO_METHOD_HANDLE replacementMethod = nullptr; GenTree* newThis = nullptr; - var_types oldThis = TYP_UNDEF; - var_types targetThis = TYP_UNDEF; + var_types oldThis = TYP_UNDEF; + var_types targetThis = TYP_UNDEF; // handle special import cases if (opcode == CEE_CALLI) @@ -172,7 +172,7 @@ var_types Compiler::impImportCall(OPCODE opcode, CORINFO_CLASS_HANDLE targetClass = info.compCompHnd->getMethodClass(replacementMethod); info.compCompHnd->getMethodSig(replacementMethod, &methodSig, targetClass); - if (methodSig.hasThisNonExplicit() && targetThis == TYP_UNDEF) + if (methodSig.hasImplicitThis() && targetThis == TYP_UNDEF) { targetThis = eeIsValueClass(targetClass) ? TYP_BYREF : TYP_REF; } @@ -1723,12 +1723,12 @@ GenTree* Compiler::impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HAN // bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, CORINFO_SIG_INFO* targetSig, - var_types sourceThis, - var_types targetThis) + var_types sourceThis, + var_types targetThis) { - assert(sourceSig->hasThisNonExplicit() || sourceThis == TYP_UNDEF); - assert(targetSig->hasThisNonExplicit() || targetThis == TYP_UNDEF || targetThis == TYP_VOID); - assert(!targetSig->hasThisNonExplicit() || targetThis != TYP_VOID); + assert(sourceSig->hasImplicitThis() || sourceThis == TYP_UNDEF); + assert(targetSig->hasImplicitThis() || targetThis == TYP_UNDEF || targetThis == TYP_VOID); + assert(!targetSig->hasImplicitThis() || targetThis != TYP_VOID); assert(sourceThis != TYP_VOID); if (sourceSig->getCallConv() != targetSig->getCallConv()) @@ -1737,7 +1737,8 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, return false; } - if ((sourceSig->hasThisNonExplicit() && sourceThis == TYP_UNDEF) || (targetSig->hasThisNonExplicit() && targetThis == TYP_UNDEF)) + if ((sourceSig->hasImplicitThis() && sourceThis == TYP_UNDEF) || + (targetSig->hasImplicitThis() && targetThis == TYP_UNDEF)) { JITDUMP("impCanSubstituteSig returning false - unknown this type\n"); return false; @@ -1746,7 +1747,7 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, unsigned sourceArgCount = sourceSig->totalILArgs(); if (targetThis == TYP_VOID) { - assert(sourceSig->hasThisNonExplicit() && sourceThis == TYP_REF); + assert(sourceSig->hasImplicitThis() && sourceThis == TYP_REF); sourceArgCount--; } @@ -1782,18 +1783,18 @@ bool Compiler::impCanSubstituteSig(CORINFO_SIG_INFO* sourceSig, unsigned numArgs = targetSig->totalILArgs(); - if ((sourceSig->hasThisNonExplicit() && targetThis != TYP_VOID) || targetSig->hasThisNonExplicit()) + if ((sourceSig->hasImplicitThis() && targetThis != TYP_VOID) || targetSig->hasImplicitThis()) { if (sourceThis == TYP_UNDEF) { sourceThis = eeGetArgType(sourceArg, sourceSig); - sourceArg = info.compCompHnd->getArgNext(sourceArg); + sourceArg = info.compCompHnd->getArgNext(sourceArg); numArgs--; } else { targetThis = eeGetArgType(targetArg, targetSig); - targetArg = info.compCompHnd->getArgNext(targetArg); + targetArg = info.compCompHnd->getArgNext(targetArg); } assert(sourceThis == TYP_REF || sourceThis == TYP_BYREF || sourceThis == TYP_I_IMPL); assert(targetThis == TYP_REF || targetThis == TYP_BYREF || targetThis == TYP_I_IMPL); diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.cs b/src/tests/JIT/opt/Devirtualization/Indirect.cs index 1ea64c158e5893..fd5cdcdeb52ad6 100644 --- a/src/tests/JIT/opt/Devirtualization/Indirect.cs +++ b/src/tests/JIT/opt/Devirtualization/Indirect.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; +using System.Collections.Generic; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -35,6 +37,8 @@ public static unsafe class Test private static int A() => 1; private static int B() => 2; + private static int A(int a, int b) => a + b + 1; + private static int B(int a, int b) => a + b + 2; private static void C() { } private static int D() => 3; @@ -45,6 +49,14 @@ private static void C() { } [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })] private static int UnmanagedStdcall() => D(); + private static int D(int a, int b) => a + b + 3; + [UnmanagedCallersOnly] + private static int UnmanagedDefault(int a, int b) => D(a, b); + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] + private static int UnmanagedCdecl(int a, int b) => D(a, b); + [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })] + private static int UnmanagedStdcall(int a, int b) => D(a, b); + [MethodImpl(MethodImplOptions.NoInlining)] public static delegate* ExecuteCctor() => Ptr; @@ -60,6 +72,12 @@ public static void Run() AreSame(D(), Invoke(() => ((delegate* unmanaged[Cdecl])&UnmanagedCdecl)())); AreSame(D(), Invoke(() => ((delegate* unmanaged[Stdcall])&UnmanagedStdcall)())); + AreSame(A(8, 9), Invoke(() => ((delegate*)&A)(8, 9))); + AreSame(B(8, 9), Invoke(() => ((delegate*)&B)(8, 9))); + AreSame(D(8, 9), Invoke(() => ((delegate* unmanaged)&UnmanagedDefault)(8, 9))); + AreSame(D(8, 9), Invoke(() => ((delegate* unmanaged[Cdecl])&UnmanagedCdecl)(8, 9))); + AreSame(D(8, 9), Invoke(() => ((delegate* unmanaged[Stdcall])&UnmanagedStdcall)(8, 9))); + AreSame(A(), Invoke(() => Ptr())); static int CallPtr(delegate* ptr) => ptr(); @@ -71,6 +89,13 @@ public static void Run() static delegate* ReturnAStatic() => Ptr; AreSame(A(), Invoke(() => ReturnAStatic()())); + static int CallPtrParam(delegate* ptr, int a, int b) => ptr(a, b); + AreSame(A(8, 9), Invoke(() => CallPtrParam(&A, 8, 9))); + AreSame(B(8, 9), Invoke(() => CallPtrParam(&B, 8, 9))); + + static delegate* ReturnAParam() => &A; + AreSame(A(8, 9), Invoke(() => ReturnAParam()(8, 9))); + AreSame(A(), Invoke(() => { var ptr = (delegate*)&A; @@ -84,6 +109,19 @@ public static void Run() return ptr(); })); + AreSame(A(8, 9), Invoke(() => + { + var ptr = (delegate*)&A; + _ = B(8, 9); + return ptr(8, 9); + })); + AreSame(A(8, 9), Invoke(() => + { + var ptr = (delegate*)&A; + _ = NoInline(B(8, 9)); + return ptr(8, 9); + })); + static int Branch(bool a) { delegate* ptr; @@ -99,6 +137,21 @@ static int Branch(bool a) AreSame(A(), Invoke(a => Branch(a), true)); AreSame(B(), Invoke(a => Branch(a), false)); + static int BranchParam(bool a) + { + delegate* ptr; + if (a) + ptr = &A; + else + ptr = &B; + return ptr(8, 9); + } + + AreSame(A(8, 9), Invoke(() => BranchParam(true))); + AreSame(B(8, 9), Invoke(() => BranchParam(false))); + AreSame(A(8, 9), Invoke(a => BranchParam(a), true)); + AreSame(B(8, 9), Invoke(a => BranchParam(a), false)); + AreSame(A(), Invoke(a => a ? PtrNull() : Ptr(), false)); AreSame(A(), Invoke(a => a ? PtrPlus1() : Ptr(), false)); AreSame(A(), Invoke(a => a ? PtrMinus1() : Ptr(), false)); @@ -136,6 +189,41 @@ static int ConditionalAddressAssign(bool a) return ptr(); } + AreSame(A(), Invoke(() => ConditionalAddressAssign(false))); + AreSame(B(), Invoke(() => ConditionalAddressAssign(true))); + AreSame(A(), Invoke(a => ConditionalAddressAssign(a), false)); + AreSame(B(), Invoke(a => ConditionalAddressAssign(a), true)); + + [MethodImpl(MethodImplOptions.NoInlining)] + static void AssignParam(delegate** ptrptr, delegate* ptr) => *ptrptr = ptr; + + AreSame(A(8, 9), Invoke(() => + { + delegate* ptr = &A; + AssignParam(&ptr, &B); + return ptr(8, 9); + })); + AreSame(A(8, 9), Invoke(() => + { + delegate* ptr; + AssignParam(&ptr, &B); + ptr = &A; + return ptr(8, 9); + })); + + static int ConditionalAddressAssignParam(bool a, int b, int c) + { + delegate* ptr = &A; + if (a) + AssignParam(&ptr, &B); + return ptr(b, c); + } + + AreSame(A(), Invoke(() => ConditionalAddressAssignParam(false, 8, 9))); + AreSame(B(), Invoke(() => ConditionalAddressAssignParam(true, 8, 9))); + AreSame(A(), Invoke(a => ConditionalAddressAssignParam(a, 8, 9), false)); + AreSame(B(), Invoke(a => ConditionalAddressAssignParam(a, 8, 9), true)); + AreSame(2, IndirectIL.StaticClass()); AreSame(1, IndirectIL.InstanceClass()); AreSame(1, IndirectIL.InstanceExplicitClass()); From 050ec5b85344ad9efe1cf0d7357c43a31da7e970 Mon Sep 17 00:00:00 2001 From: petris Date: Mon, 1 May 2023 22:06:26 +0200 Subject: [PATCH 14/18] Add pinvoke and no assign tests --- .../JIT/opt/Devirtualization/CMakeLists.txt | 6 ++ .../JIT/opt/Devirtualization/Indirect.cs | 50 +++++++++++++++ .../JIT/opt/Devirtualization/Indirect.csproj | 3 + .../JIT/opt/Devirtualization/IndirectIL.il | 64 +++++++++++++++---- .../opt/Devirtualization/IndirectNative.cpp | 21 ++++++ 5 files changed, 132 insertions(+), 12 deletions(-) create mode 100644 src/tests/JIT/opt/Devirtualization/CMakeLists.txt create mode 100644 src/tests/JIT/opt/Devirtualization/IndirectNative.cpp diff --git a/src/tests/JIT/opt/Devirtualization/CMakeLists.txt b/src/tests/JIT/opt/Devirtualization/CMakeLists.txt new file mode 100644 index 00000000000000..edc6877c52b099 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/CMakeLists.txt @@ -0,0 +1,6 @@ +project (IndirectNative) +include ("${CLR_INTEROP_TEST_ROOT}/Interop.cmake") +set(SOURCES IndirectNative.cpp) +add_library (IndirectNative SHARED ${SOURCES}) +# add the install targets +install (TARGETS IndirectNative DESTINATION bin) diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.cs b/src/tests/JIT/opt/Devirtualization/Indirect.cs index fd5cdcdeb52ad6..6637dc68a3d332 100644 --- a/src/tests/JIT/opt/Devirtualization/Indirect.cs +++ b/src/tests/JIT/opt/Devirtualization/Indirect.cs @@ -40,6 +40,7 @@ public static unsafe class Test private static int A(int a, int b) => a + b + 1; private static int B(int a, int b) => a + b + 2; private static void C() { } + private static void C(int a, int b) { } private static int D() => 3; [UnmanagedCallersOnly] @@ -57,6 +58,13 @@ private static void C() { } [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })] private static int UnmanagedStdcall(int a, int b) => D(a, b); + [DllImport("IndirectNative", EntryPoint = "E")] + private static extern int E(); + [DllImport("IndirectNative", EntryPoint = "EParam")] + private static extern int E(int a, int b); + [DllImport("IndirectNative", EntryPoint = "EPtrs")] + private static extern int E(ref int a, ref int b); + [MethodImpl(MethodImplOptions.NoInlining)] public static delegate* ExecuteCctor() => Ptr; @@ -68,16 +76,36 @@ public static void Run() AreSame(A(), Invoke(() => ((delegate*)&A)())); AreSame(B(), Invoke(() => ((delegate*)&B)())); + AreSame(A(), Invoke(() => { + ((delegate*)&C)(); + return A(); + })); + AreSame(D(), Invoke(() => ((delegate* unmanaged)&UnmanagedDefault)())); AreSame(D(), Invoke(() => ((delegate* unmanaged[Cdecl])&UnmanagedCdecl)())); AreSame(D(), Invoke(() => ((delegate* unmanaged[Stdcall])&UnmanagedStdcall)())); + AreSame(E(), Invoke(() => ((delegate*)&E)())); + AreSame(A(8, 9), Invoke(() => ((delegate*)&A)(8, 9))); AreSame(B(8, 9), Invoke(() => ((delegate*)&B)(8, 9))); + AreSame(A(8, 9), Invoke(() => { + ((delegate*)&C)(8, 9); + return A(8, 9); + })); + AreSame(D(8, 9), Invoke(() => ((delegate* unmanaged)&UnmanagedDefault)(8, 9))); AreSame(D(8, 9), Invoke(() => ((delegate* unmanaged[Cdecl])&UnmanagedCdecl)(8, 9))); AreSame(D(8, 9), Invoke(() => ((delegate* unmanaged[Stdcall])&UnmanagedStdcall)(8, 9))); + AreSame(E(8, 9), Invoke(() => ((delegate*)&E)(8, 9))); + + AreSame(E(8, 9), Invoke(() => { + int a = 8; + int b = 9; + return ((delegate*)&E)(ref a, ref b); + })); + AreSame(A(), Invoke(() => Ptr())); static int CallPtr(delegate* ptr) => ptr(); @@ -237,6 +265,13 @@ static int ConditionalAddressAssignParam(bool a, int b, int c) AreSame(4, IndirectIL.StaticStructParam()); AreSame(3, IndirectIL.InstanceStructParam()); AreSame(3, IndirectIL.InstanceExplicitStructParam()); + + AreSame(2, Invoke(() => IndirectIL.BranchAssign(true))); + AreSame(2, Invoke((a) => IndirectIL.BranchAssign(a), true)); + AssertThrowsNullReferenceException(() => IndirectIL.BranchAssign(false)); + AssertThrowsNullReferenceException((a) => IndirectIL.BranchAssign(a), false); + + AssertThrowsNullReferenceException(() => IndirectIL.NoAssign()); } [MethodImpl(MethodImplOptions.NoInlining)] @@ -269,4 +304,19 @@ private static void AssertThrowsNullReferenceException(Func a, [CallerLine throw new InvalidOperationException($"Expected NullReferenceException at line {line}"); } + + [MethodImpl(MethodImplOptions.NoInlining)] + private static void AssertThrowsNullReferenceException(Func a, TArg arg, [CallerLineNumber] int line = 0) + { + try + { + _ = a(arg); + } + catch (NullReferenceException) + { + return; + } + + throw new InvalidOperationException($"Expected NullReferenceException at line {line}"); + } } diff --git a/src/tests/JIT/opt/Devirtualization/Indirect.csproj b/src/tests/JIT/opt/Devirtualization/Indirect.csproj index 551841b652015a..45d9d2a00b87d0 100644 --- a/src/tests/JIT/opt/Devirtualization/Indirect.csproj +++ b/src/tests/JIT/opt/Devirtualization/Indirect.csproj @@ -11,4 +11,7 @@ + + + diff --git a/src/tests/JIT/opt/Devirtualization/IndirectIL.il b/src/tests/JIT/opt/Devirtualization/IndirectIL.il index 1da2be6da11b4e..e7fa3f010eb42a 100644 --- a/src/tests/JIT/opt/Devirtualization/IndirectIL.il +++ b/src/tests/JIT/opt/Devirtualization/IndirectIL.il @@ -15,7 +15,7 @@ IL_0000: ldftn int32 Cl::B() IL_0006: calli int32() IL_000b: ret - } // end of method ILTests::A + } // end of method ILTests::StaticClass .method public hidebysig static int32 InstanceClass() cil managed noinlining aggressiveoptimization { @@ -26,7 +26,7 @@ IL_0005: ldftn instance int32 Cl::A() IL_000b: calli instance int32() IL_0010: ret - } // end of method ILTests::B + } // end of method ILTests::InstanceClass .method public hidebysig static int32 InstanceExplicitClass() cil managed noinlining aggressiveoptimization { @@ -37,7 +37,7 @@ IL_0005: ldftn instance int32 Cl::A() IL_000b: calli explicit instance int32(class Cl) IL_0010: ret - } // end of method ILTests::C + } // end of method ILTests::InstanceExplicitClass .method public hidebysig static int32 StaticClassParam() cil managed noinlining aggressiveoptimization { @@ -50,7 +50,7 @@ IL_0008: ldloc.0 IL_0009: calli int32(int32) IL_000e: ret - } // end of method ILTests::D + } // end of method ILTests::StaticClassParam .method public hidebysig static int32 InstanceClassParam() cil managed noinlining aggressiveoptimization { @@ -67,7 +67,7 @@ IL_000f: ldloc.1 IL_0010: calli instance int32(int32) IL_0015: ret - } // end of method ILTests::E + } // end of method ILTests::InstanceClassParam .method public hidebysig static int32 InstanceExplicitClassParam() cil managed noinlining aggressiveoptimization { @@ -84,7 +84,7 @@ IL_000f: ldloc.1 IL_0010: calli explicit instance int32(class Cl,int32) IL_0015: ret - } // end of method ILTests::F + } // end of method ILTests::InstanceExplicitClassParam .method public hidebysig static int32 StaticStruct() cil managed noinlining aggressiveoptimization { @@ -93,7 +93,7 @@ IL_0000: ldftn int32 St::B() IL_0006: calli int32() IL_000b: ret - } // end of method ILTests::G + } // end of method ILTests::StaticStruct .method public hidebysig static int32 InstanceStruct() cil managed noinlining aggressiveoptimization { @@ -106,7 +106,7 @@ IL_000a: ldftn instance int32 St::A() IL_0010: calli instance int32() IL_0015: ret - } // end of method ILTests::H + } // end of method ILTests::InstanceStruct .method public hidebysig static int32 InstanceExplicitStruct() cil managed noinlining aggressiveoptimization { @@ -119,7 +119,7 @@ IL_000a: ldftn instance int32 St::A() IL_0010: calli explicit instance int32(valuetype St&) IL_0015: ret - } // end of method ILTests::I + } // end of method ILTests::InstanceExplicitStruct .method public hidebysig static int32 StaticStructParam() cil managed noinlining aggressiveoptimization { @@ -132,7 +132,7 @@ IL_0008: ldloc.0 IL_0009: calli int32(int32) IL_000e: ret - } // end of method ILTests::J + } // end of method ILTests::StaticStructParam .method public hidebysig static int32 InstanceStructParam() cil managed noinlining aggressiveoptimization { @@ -149,7 +149,7 @@ IL_0012: ldloc.1 IL_0013: calli instance int32(int32) IL_0018: ret - } // end of method ILTests::K + } // end of method ILTests::InstanceStructParam .method public hidebysig static int32 InstanceExplicitStructParam() cil managed noinlining aggressiveoptimization { @@ -166,8 +166,48 @@ IL_0012: ldloc.1 IL_0013: calli explicit instance int32(valuetype St&,int32) IL_0018: ret - } // end of method ILTests::L + } // end of method ILTests::InstanceExplicitStructParam + .method public hidebysig static + int32 BranchAssign ( + bool b + ) cil managed + { + // Method begins at RVA 0x2050 + // Code size 24 (0x18) + .maxstack 1 + .locals init ( + [0] method int32 *() + ) + + IL_0000: ldarg.0 + IL_0001: brfalse.s IL_0011 + + IL_0003: ldftn int32 Cl::B() + IL_0009: stloc.0 + IL_000a: ldloc.0 + IL_000b: calli int32() + IL_0010: ret + + IL_0011: ldloc.0 + IL_0012: calli int32() + IL_0017: ret + } // end of method ILTests::BranchAssign + + .method public hidebysig static + int32 NoAssign () cil managed + { + // Method begins at RVA 0x2050 + // Code size 7 (0x7) + .maxstack 1 + .locals init ( + [0] method int32 *() + ) + + IL_0000: ldloc.0 + IL_0001: calli int32() + IL_0006: ret + } // end of method ILTests::NoAssign } // end of class ILTests .class public auto ansi sealed beforefieldinit Cl diff --git a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp new file mode 100644 index 00000000000000..0f979dab726930 --- /dev/null +++ b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp @@ -0,0 +1,21 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +#include +#include +#include + +extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE E() +{ + return 4; +} + +extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE EParams(int32_t a, int32_t b) +{ + return a + b + 4; +} + +extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE EPtrs(int32_t* a, int32_t* b) +{ + return *a + *b + 4; +} From c06cdf0dea8744c5c459f53d928f282226834cb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 1 May 2023 22:33:35 +0200 Subject: [PATCH 15/18] Update CMakeLists.txt --- src/tests/JIT/opt/Devirtualization/CMakeLists.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tests/JIT/opt/Devirtualization/CMakeLists.txt b/src/tests/JIT/opt/Devirtualization/CMakeLists.txt index edc6877c52b099..02f46f6c126c5a 100644 --- a/src/tests/JIT/opt/Devirtualization/CMakeLists.txt +++ b/src/tests/JIT/opt/Devirtualization/CMakeLists.txt @@ -1,5 +1,4 @@ project (IndirectNative) -include ("${CLR_INTEROP_TEST_ROOT}/Interop.cmake") set(SOURCES IndirectNative.cpp) add_library (IndirectNative SHARED ${SOURCES}) # add the install targets From b9fa699e62989f7d15d84789a7ddeb0b1f0c7a0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 1 May 2023 23:17:11 +0200 Subject: [PATCH 16/18] Update IndirectNative.cpp --- src/tests/JIT/opt/Devirtualization/IndirectNative.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp index 0f979dab726930..bd8f914dda358e 100644 --- a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp +++ b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp @@ -1,7 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -#include +#include #include #include From ce759c9f663f551ec87272aa8eb85a6e1acaf6c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Mon, 1 May 2023 23:54:21 +0200 Subject: [PATCH 17/18] Update IndirectNative.cpp --- src/tests/JIT/opt/Devirtualization/IndirectNative.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp index bd8f914dda358e..1fb778feffd849 100644 --- a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp +++ b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. #include -#include #include extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE E() From a19d9b1656009aaa0cf29ea718f0aba2d27fa96a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Petryka?= <35800402+MichalPetryka@users.noreply.github.com> Date: Tue, 2 May 2023 02:49:46 +0200 Subject: [PATCH 18/18] Update IndirectNative.cpp --- .../JIT/opt/Devirtualization/IndirectNative.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp index 1fb778feffd849..bfa7ed3a5439fc 100644 --- a/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp +++ b/src/tests/JIT/opt/Devirtualization/IndirectNative.cpp @@ -2,19 +2,28 @@ // The .NET Foundation licenses this file to you under the MIT license. #include -#include -extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE E() +#ifdef TARGET_WINDOWS +#define DLL_EXPORT extern "C" __declspec(dllexport) +#else +#define DLL_EXPORT extern "C" __attribute((visibility("default"))) +#endif + +#ifndef TARGET_WINDOWS +#define __stdcall +#endif + +DLL_EXPORT int32_t __stdcall E() { return 4; } -extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE EParams(int32_t a, int32_t b) +DLL_EXPORT int32_t __stdcall EParams(int32_t a, int32_t b) { return a + b + 4; } -extern "C" DLL_EXPORT int32_t STDMETHODCALLTYPE EPtrs(int32_t* a, int32_t* b) +DLL_EXPORT int32_t __stdcall EPtrs(int32_t* a, int32_t* b) { return *a + *b + 4; }