From 175b7e85a7a3a64b4735f3fca626eb2caf61315e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 May 2023 14:33:09 +0200 Subject: [PATCH 1/5] Remove AO from a couple of SpanHelpers methods --- .../System.Private.CoreLib/src/System/SpanHelpers.Byte.cs | 1 - .../System.Private.CoreLib/src/System/SpanHelpers.Char.cs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs index c3d7cd2cd5071..a50c49bd6a1a0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs @@ -342,7 +342,6 @@ private static void ThrowMustBeNullTerminatedString() // IndexOfNullByte processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. // This behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. - [MethodImpl(MethodImplOptions.AggressiveOptimization)] internal static unsafe int IndexOfNullByte(byte* searchSpace) { const int Length = int.MaxValue; diff --git a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs index f48659545b9d8..0f19bc197e58f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Char.cs @@ -422,7 +422,6 @@ public static unsafe int SequenceCompareTo(ref char first, int firstLength, ref // IndexOfNullCharacter processes memory in aligned chunks, and thus it won't crash even if it accesses memory beyond the null terminator. // This behavior is an implementation detail of the runtime and callers outside System.Private.CoreLib must not depend on it. - [MethodImpl(MethodImplOptions.AggressiveOptimization)] public static unsafe int IndexOfNullCharacter(char* searchSpace) { const char value = '\0'; From 8baf567cd955dfebf58443a20fb0606456ebfc35 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 9 May 2023 02:36:57 +0200 Subject: [PATCH 2/5] Address feedback --- src/coreclr/jit/importercalls.cpp | 32 +++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ce7d2f286db33..62c8a878cd24d 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -3987,6 +3987,38 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, { assert((sig->sigInst.methInstCount == 1) || (sig->sigInst.methInstCount == 2)); + if (sig->sigInst.methInstCount == 1) + { + const CORINFO_CLASS_HANDLE inst = sig->sigInst.methInst[0]; + assert(inst != nullptr); + + if ((info.compCompHnd->getClassAttribs(inst) & CORINFO_FLG_SHAREDINST) == 0) + { + GenTree* op = impPopStack().val; + assert(op->TypeIs(TYP_REF)); + + JITDUMP("Expanding Unsafe.As<%s>(...)\n", eeGetClassName(inst)); + + // In order to change the class handle of the object we need to spill it to a temp + // and update class info for that temp. + unsigned localNum = lvaGrabTemp(true DEBUGARG("updating class info")); + impAssignTempGen(localNum, op); + + bool isExact, isNonNull; + CORINFO_CLASS_HANDLE oldClass = gtGetClassHandle(op, &isExact, &isNonNull); + if ((oldClass != NO_CLASS_HANDLE) && info.compCompHnd->isMoreSpecificType(inst, oldClass)) + { + JITDUMP("Unsafe.As: Keep using old '%s' type as it is more specific\n", + eeGetClassName(oldClass)); + return op; + } + + // NOTE: we still can't say for sure that it is the exact type of the argument + lvaSetClass(localNum, inst, /*isExact*/ false); + return gtNewLclvNode(localNum, TYP_REF); + } + } + // ldarg.0 // ret From 47d61db86e42d6e4f91f9451d03ff8dd8c818667 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 9 May 2023 13:11:46 +0200 Subject: [PATCH 3/5] fix bug --- src/coreclr/jit/importercalls.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 62c8a878cd24d..89f7d40128c6a 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -3999,11 +3999,6 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, JITDUMP("Expanding Unsafe.As<%s>(...)\n", eeGetClassName(inst)); - // In order to change the class handle of the object we need to spill it to a temp - // and update class info for that temp. - unsigned localNum = lvaGrabTemp(true DEBUGARG("updating class info")); - impAssignTempGen(localNum, op); - bool isExact, isNonNull; CORINFO_CLASS_HANDLE oldClass = gtGetClassHandle(op, &isExact, &isNonNull); if ((oldClass != NO_CLASS_HANDLE) && info.compCompHnd->isMoreSpecificType(inst, oldClass)) @@ -4013,6 +4008,11 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, return op; } + // In order to change the class handle of the object we need to spill it to a temp + // and update class info for that temp. + unsigned localNum = lvaGrabTemp(true DEBUGARG("updating class info")); + impAssignTempGen(localNum, op); + // NOTE: we still can't say for sure that it is the exact type of the argument lvaSetClass(localNum, inst, /*isExact*/ false); return gtNewLclvNode(localNum, TYP_REF); From 1682cdd6449f1d1955175f9fd205235ca2c90c0f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 9 May 2023 14:47:54 +0200 Subject: [PATCH 4/5] Address feedback --- src/coreclr/jit/compiler.h | 5 ++- src/coreclr/jit/importercalls.cpp | 61 +++++++++++++++---------------- 2 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 91d4350e0afda..eb0c56f7a3441 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3945,7 +3945,7 @@ class Compiler CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO* sig, unsigned methodFlags, - int memberRef, + CORINFO_RESOLVED_TOKEN* pResolvedToken, bool readonlyCall, bool tailCall, CORINFO_RESOLVED_TOKEN* pContstrainedResolvedToken, @@ -3967,7 +3967,8 @@ class Compiler GenTree* impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, CORINFO_CLASS_HANDLE clsHnd, CORINFO_METHOD_HANDLE method, - CORINFO_SIG_INFO* sig); + CORINFO_SIG_INFO* sig, + CORINFO_RESOLVED_TOKEN* pResolvedToken); GenTree* impPrimitiveNamedIntrinsic(NamedIntrinsic intrinsic, CORINFO_CLASS_HANDLE clsHnd, diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 89f7d40128c6a..7db5358aee5f3 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -233,9 +233,8 @@ var_types Compiler::impImportCall(OPCODE opcode, const bool isTailCall = canTailCall && (tailCallFlags != 0); - call = - impIntrinsic(newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken->token, isReadonlyCall, - isTailCall, pConstrainedResolvedToken, callInfo->thisTransform, &ni, &isSpecialIntrinsic); + call = impIntrinsic(newobjThis, clsHnd, methHnd, sig, mflags, pResolvedToken, isReadonlyCall, isTailCall, + pConstrainedResolvedToken, callInfo->thisTransform, &ni, &isSpecialIntrinsic); if (compDonotInline()) { @@ -2301,7 +2300,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, CORINFO_METHOD_HANDLE method, CORINFO_SIG_INFO* sig, unsigned methodFlags, - int memberRef, + CORINFO_RESOLVED_TOKEN* pResolvedToken, bool readonlyCall, bool tailCall, CORINFO_RESOLVED_TOKEN* pConstrainedResolvedToken, @@ -2312,6 +2311,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, bool mustExpand = false; bool isSpecial = false; const bool isIntrinsic = (methodFlags & CORINFO_FLG_INTRINSIC) != 0; + int memberRef = pResolvedToken->token; NamedIntrinsic ni = lookupNamedIntrinsic(method); @@ -2455,7 +2455,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, { assert(ni > NI_SRCS_UNSAFE_START); assert(!mustExpand); - return impSRCSUnsafeIntrinsic(ni, clsHnd, method, sig); + return impSRCSUnsafeIntrinsic(ni, clsHnd, method, sig, pResolvedToken); } else { @@ -3905,10 +3905,11 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, return retNode; } -GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, - CORINFO_CLASS_HANDLE clsHnd, - CORINFO_METHOD_HANDLE method, - CORINFO_SIG_INFO* sig) +GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, + CORINFO_CLASS_HANDLE clsHnd, + CORINFO_METHOD_HANDLE method, + CORINFO_SIG_INFO* sig, + CORINFO_RESOLVED_TOKEN* pResolvedToken) { // NextCallRetAddr requires a CALL, so return nullptr. if (info.compHasNextCallRetAddr) @@ -3989,34 +3990,32 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, if (sig->sigInst.methInstCount == 1) { - const CORINFO_CLASS_HANDLE inst = sig->sigInst.methInst[0]; + CORINFO_SIG_INFO exactSig; + info.compCompHnd->getMethodSig(pResolvedToken->hMethod, &exactSig); + const CORINFO_CLASS_HANDLE inst = exactSig.sigInst.methInst[0]; assert(inst != nullptr); - if ((info.compCompHnd->getClassAttribs(inst) & CORINFO_FLG_SHAREDINST) == 0) - { - GenTree* op = impPopStack().val; - assert(op->TypeIs(TYP_REF)); + GenTree* op = impPopStack().val; + assert(op->TypeIs(TYP_REF)); - JITDUMP("Expanding Unsafe.As<%s>(...)\n", eeGetClassName(inst)); + JITDUMP("Expanding Unsafe.As<%s>(...)\n", eeGetClassName(inst)); - bool isExact, isNonNull; - CORINFO_CLASS_HANDLE oldClass = gtGetClassHandle(op, &isExact, &isNonNull); - if ((oldClass != NO_CLASS_HANDLE) && info.compCompHnd->isMoreSpecificType(inst, oldClass)) - { - JITDUMP("Unsafe.As: Keep using old '%s' type as it is more specific\n", - eeGetClassName(oldClass)); - return op; - } + bool isExact, isNonNull; + CORINFO_CLASS_HANDLE oldClass = gtGetClassHandle(op, &isExact, &isNonNull); + if ((oldClass != NO_CLASS_HANDLE) && !info.compCompHnd->isMoreSpecificType(oldClass, inst)) + { + JITDUMP("Unsafe.As: Keep using old '%s' type as it is more specific\n", eeGetClassName(oldClass)); + return op; + } - // In order to change the class handle of the object we need to spill it to a temp - // and update class info for that temp. - unsigned localNum = lvaGrabTemp(true DEBUGARG("updating class info")); - impAssignTempGen(localNum, op); + // In order to change the class handle of the object we need to spill it to a temp + // and update class info for that temp. + unsigned localNum = lvaGrabTemp(true DEBUGARG("updating class info")); + impAssignTempGen(localNum, op); - // NOTE: we still can't say for sure that it is the exact type of the argument - lvaSetClass(localNum, inst, /*isExact*/ false); - return gtNewLclvNode(localNum, TYP_REF); - } + // NOTE: we still can't say for sure that it is the exact type of the argument + lvaSetClass(localNum, inst, /*isExact*/ false); + return gtNewLclvNode(localNum, TYP_REF); } // ldarg.0 From 68aa4a68482cd9ee1954f0d253e77b689512e911 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 9 May 2023 18:54:35 +0200 Subject: [PATCH 5/5] Address feedback --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/importer.cpp | 2 +- src/coreclr/jit/importercalls.cpp | 7 ++++--- src/coreclr/jit/importervectorization.cpp | 10 +++++----- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d84a882820882..74331ffed7931 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4031,7 +4031,7 @@ class Compiler Statement* impAppendTree(GenTree* tree, unsigned chkLevel, const DebugInfo& di, bool checkConsumedDebugInfo = true); void impAssignTempGen(unsigned lclNum, GenTree* val, - unsigned curLevel = CHECK_SPILL_NONE, + unsigned curLevel, Statement** pAfterStmt = nullptr, const DebugInfo& di = DebugInfo(), BasicBlock* block = nullptr); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 29297a6b46d6c..0e02df0c33e67 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1638,7 +1638,7 @@ GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken // Spilling it to a temp improves CQ (mainly in Tier0) unsigned callLclNum = lvaGrabTemp(true DEBUGARG("spilling helperCall")); - impAssignTempGen(callLclNum, helperCall); + impAssignTempGen(callLclNum, helperCall, CHECK_SPILL_NONE); return gtNewLclvNode(callLclNum, helperCall->TypeGet()); } diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index ea0b07564fbc1..a56f31d9bb3f0 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -4002,16 +4002,17 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, bool isExact, isNonNull; CORINFO_CLASS_HANDLE oldClass = gtGetClassHandle(op, &isExact, &isNonNull); - if ((oldClass != NO_CLASS_HANDLE) && !info.compCompHnd->isMoreSpecificType(oldClass, inst)) + if ((oldClass != NO_CLASS_HANDLE) && + ((oldClass == inst) || !info.compCompHnd->isMoreSpecificType(oldClass, inst))) { - JITDUMP("Unsafe.As: Keep using old '%s' type as it is more specific\n", eeGetClassName(oldClass)); + JITDUMP("Unsafe.As: Keep using old '%s' type\n", eeGetClassName(oldClass)); return op; } // In order to change the class handle of the object we need to spill it to a temp // and update class info for that temp. unsigned localNum = lvaGrabTemp(true DEBUGARG("updating class info")); - impAssignTempGen(localNum, op); + impAssignTempGen(localNum, op, CHECK_SPILL_ALL); // NOTE: we still can't say for sure that it is the exact type of the argument lvaSetClass(localNum, inst, /*isExact*/ false); diff --git a/src/coreclr/jit/importervectorization.cpp b/src/coreclr/jit/importervectorization.cpp index 141073b158e27..a70143fac22fa 100644 --- a/src/coreclr/jit/importervectorization.cpp +++ b/src/coreclr/jit/importervectorization.cpp @@ -645,12 +645,12 @@ GenTree* Compiler::impStringEqualsOrStartsWith(bool startsWith, CORINFO_SIG_INFO strLenOffset + sizeof(int), cmpMode); if (unrolled != nullptr) { - impAssignTempGen(varStrTmp, varStr); + impAssignTempGen(varStrTmp, varStr, CHECK_SPILL_NONE); if (unrolled->OperIs(GT_QMARK)) { // QMARK nodes cannot reside on the evaluation stack unsigned rootTmp = lvaGrabTemp(true DEBUGARG("spilling unroll qmark")); - impAssignTempGen(rootTmp, unrolled); + impAssignTempGen(rootTmp, unrolled, CHECK_SPILL_NONE); unrolled = gtNewLclvNode(rootTmp, TYP_INT); } @@ -797,13 +797,13 @@ GenTree* Compiler::impSpanEqualsOrStartsWith(bool startsWith, CORINFO_SIG_INFO* if (unrolled != nullptr) { // We succeeded, fill the placeholders: - impAssignTempGen(spanObjRef, impGetStructAddr(spanObj, CHECK_SPILL_NONE, true)); - impAssignTempGen(spanDataTmp, spanData); + impAssignTempGen(spanObjRef, impGetStructAddr(spanObj, CHECK_SPILL_NONE, true), CHECK_SPILL_NONE); + impAssignTempGen(spanDataTmp, spanData, CHECK_SPILL_NONE); if (unrolled->OperIs(GT_QMARK)) { // QMARK can't be a root node, spill it to a temp unsigned rootTmp = lvaGrabTemp(true DEBUGARG("spilling unroll qmark")); - impAssignTempGen(rootTmp, unrolled); + impAssignTempGen(rootTmp, unrolled, CHECK_SPILL_NONE); unrolled = gtNewLclvNode(rootTmp, TYP_INT); }