From 9f55fa6d7c0f931051bd13b43f887e98da3b837a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 12 Jan 2022 23:06:31 +0300 Subject: [PATCH 01/23] Don't reuse args in EnC --- src/coreclr/jit/lsra.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 9308836ff78fcb..7d6a74ed12801d 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -2808,7 +2808,7 @@ regNumber LinearScan::allocateReg(Interval* currentInterval, bool wasAssigned = regSelector->foundUnassignedReg() && (assignedInterval != nullptr) && (assignedInterval->physReg == foundReg); unassignPhysReg(availablePhysRegRecord ARM_ARG(currentInterval->registerType)); - if (regSelector->isMatchingConstant()) + if (regSelector->isMatchingConstant() && !compiler->opts.compDbgEnC) { assert(assignedInterval->isConstant); refPosition->treeNode->SetReuseRegVal(); From 0a717aa70e5c513401c88f106074cb5e53d9e5f1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 14:09:18 +0300 Subject: [PATCH 02/23] Introduce RuntimeHelpers.IsKnownConstant and use for 'str == ""' --- src/coreclr/jit/fgbasic.cpp | 6 +++ src/coreclr/jit/importer.cpp | 48 +++++++++++++++---- src/coreclr/jit/namedintrinsiclist.h | 1 + .../CompilerServices/RuntimeHelpers.cs | 6 +++ .../src/System/String.Comparison.cs | 14 ++++++ 5 files changed, 65 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 73695932cb5439..11621a75248218 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1145,6 +1145,12 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed break; } + case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: + pushedStack.PushConstant(); + foldableIntrinsc = true; + // we can add an additional boost if arg is really a const + break; + // These are foldable if the first argument is a constant case NI_System_Type_get_IsValueType: case NI_System_Type_GetTypeFromHandle: diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 33e5443c1eee4a..508c560ca70fe4 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3879,19 +3879,25 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, return new (this, GT_LABEL) GenTree(GT_LABEL, TYP_I_IMPL); } - if (((ni == NI_System_Runtime_CompilerServices_RuntimeHelpers_CreateSpan) || - (ni == NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray)) && - IsTargetAbi(CORINFO_CORERT_ABI)) + switch (ni) { // CreateSpan must be expanded for NativeAOT - mustExpand = true; - } + case NI_System_Runtime_CompilerServices_RuntimeHelpers_CreateSpan: + case NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray: + mustExpand = IsTargetAbi(CORINFO_CORERT_ABI); + break; - if ((ni == NI_System_ByReference_ctor) || (ni == NI_System_ByReference_get_Value) || - (ni == NI_System_Activator_AllocatorOf) || (ni == NI_System_Activator_DefaultConstructorOf) || - (ni == NI_System_Object_MethodTableOf) || (ni == NI_System_EETypePtr_EETypePtrOf)) - { - mustExpand = true; + case NI_System_ByReference_ctor: + case NI_System_ByReference_get_Value: + case NI_System_Activator_AllocatorOf: + case NI_System_Activator_DefaultConstructorOf: + case NI_System_Object_MethodTableOf: + case NI_System_EETypePtr_EETypePtrOf: + mustExpand = true; + break; + + default: + break; } GenTree* retNode = nullptr; @@ -4007,6 +4013,24 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, break; } + case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: + { + GenTree* op1 = impPopStack().val; + if (op1->OperIsConst()) // || gtFoldExpr(op1)->OperIsConst()) + { + // op1 is a known constant, replace with 'true'. + retNode = gtNewIconNode(1); + // We can also consider FTN_ADDR and typeof(T) here + } + else + { + // op1 is not a known constant, replace with 'false'. + // TODO: delay till e.g. morph and try again, maybe op1 will become a const. + retNode = gtNewIconNode(0); + } + break; + } + case NI_System_Activator_AllocatorOf: case NI_System_Activator_DefaultConstructorOf: case NI_System_Object_MethodTableOf: @@ -5352,6 +5376,10 @@ NamedIntrinsic Compiler::lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method) { result = NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray; } + else if (strcmp(methodName, "IsKnownConstant") == 0) + { + result = NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant; + } } else if (strncmp(namespaceName, "System.Runtime.Intrinsics", 25) == 0) { diff --git a/src/coreclr/jit/namedintrinsiclist.h b/src/coreclr/jit/namedintrinsiclist.h index 25733a41a9c9ed..7b2108d3edd057 100644 --- a/src/coreclr/jit/namedintrinsiclist.h +++ b/src/coreclr/jit/namedintrinsiclist.h @@ -78,6 +78,7 @@ enum NamedIntrinsic : unsigned short NI_System_Runtime_CompilerServices_RuntimeHelpers_CreateSpan, NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray, + NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant, NI_System_String_get_Chars, NI_System_String_get_Length, diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs index 8187320ec97683..2f830f3b5793c5 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs @@ -117,5 +117,11 @@ internal static bool IsPrimitiveType(this CorElementType et) /// This method is intended for compiler use rather than use directly in code. T must be one of byte, sbyte, char, short, ushort, int, long, ulong, float, or double. [Intrinsic] public static unsafe ReadOnlySpan CreateSpan(RuntimeFieldHandle fldHandle) => new ReadOnlySpan(GetSpanDataFrom(fldHandle, typeof(T).TypeHandle, out int length), length); + + /// + /// Returns true if input is a compile-time constant + /// + [Intrinsic] + internal static bool IsKnownConstant(T t) => false; } } diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index 9d068b4da0bc92..435c4afa262b55 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -681,8 +681,22 @@ public bool Equals([NotNullWhen(true)] string? value, StringComparison compariso } // Determines whether two Strings match. + [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool Equals(string? a, string? b) { + // Replace SequenceEqual(strA, strB) with + // strB.Length == 0 if strB is a compile-time constant representing empty string + // Otherwise, the whole branch is eliminated + if (RuntimeHelpers.IsKnownConstant(a) && a?.Length == 0) + { + return b?.Length == 0; + } + + if (RuntimeHelpers.IsKnownConstant(b) && b?.Length == 0) + { + return a?.Length == 0; + } + if (object.ReferenceEquals(a, b)) { return true; From 407a9532c27d5b693a61ac35a8e24e4814f913c0 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 14:19:20 +0300 Subject: [PATCH 03/23] Remove unrelated change, add gtFoldExpr --- src/coreclr/jit/importer.cpp | 2 +- src/coreclr/jit/lsra.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 508c560ca70fe4..f819e43a7736aa 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4016,7 +4016,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: { GenTree* op1 = impPopStack().val; - if (op1->OperIsConst()) // || gtFoldExpr(op1)->OperIsConst()) + if (op1->OperIsConst() || gtFoldExpr(op1)->OperIsConst()) { // op1 is a known constant, replace with 'true'. retNode = gtNewIconNode(1); diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 7d6a74ed12801d..9308836ff78fcb 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -2808,7 +2808,7 @@ regNumber LinearScan::allocateReg(Interval* currentInterval, bool wasAssigned = regSelector->foundUnassignedReg() && (assignedInterval != nullptr) && (assignedInterval->physReg == foundReg); unassignPhysReg(availablePhysRegRecord ARM_ARG(currentInterval->registerType)); - if (regSelector->isMatchingConstant() && !compiler->opts.compDbgEnC) + if (regSelector->isMatchingConstant()) { assert(assignedInterval->isConstant); refPosition->treeNode->SetReuseRegVal(); From 9d45461bac4e5f95dec4ce4a695e36444496810d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 14:42:44 +0300 Subject: [PATCH 04/23] Also, optimize String.StartsWith --- .../src/System/String.Comparison.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index 435c4afa262b55..d333262a83a629 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -1027,7 +1027,15 @@ public bool StartsWith(string value, bool ignoreCase, CultureInfo? culture) return referenceCulture.CompareInfo.IsPrefix(this, value, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None); } - public bool StartsWith(char value) => Length != 0 && _firstChar == value; + [MethodImpl(MethodImplOptions.AggressiveInlining)] + public bool StartsWith(char value) + { + if (RuntimeHelpers.IsKnownConstant(value)) + { + return _firstChar == value; + } + return Length != 0 && _firstChar == value; + } internal static void CheckStringComparison(StringComparison comparisonType) { From 2f8a0343696e744eef50ac77eade8306e8ffcb93 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 15:02:21 +0300 Subject: [PATCH 05/23] Update comment --- .../System.Private.CoreLib/src/System/String.Comparison.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index d333262a83a629..efd8c15615e643 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -684,9 +684,8 @@ public bool Equals([NotNullWhen(true)] string? value, StringComparison compariso [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool Equals(string? a, string? b) { - // Replace SequenceEqual(strA, strB) with - // strB.Length == 0 if strB is a compile-time constant representing empty string - // Otherwise, the whole branch is eliminated + // if either a or b are "" - optimize Equals to just 'str?.Length == 0' + // Otherwise, these two blocks are eliminated since IsKnownConstant is a jit-time constant if (RuntimeHelpers.IsKnownConstant(a) && a?.Length == 0) { return b?.Length == 0; From 4883a468616b4cc44c664691772286add6e72e8a Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 13 Jan 2022 15:07:20 +0300 Subject: [PATCH 06/23] Update src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs Co-authored-by: Miha Zupan --- .../System.Private.CoreLib/src/System/String.Comparison.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index efd8c15615e643..c0131f4645b081 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -1029,7 +1029,7 @@ public bool StartsWith(string value, bool ignoreCase, CultureInfo? culture) [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool StartsWith(char value) { - if (RuntimeHelpers.IsKnownConstant(value)) + if (RuntimeHelpers.IsKnownConstant(value) && value != '\0') { return _firstChar == value; } From aa88cf9d6602f3bbf229a8b8cf02e2a8b27f6474 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 17:09:09 +0300 Subject: [PATCH 07/23] Delay expansion to morph, replace generic signature with overloads --- src/coreclr/jit/compiler.hpp | 22 ++++++++-- src/coreclr/jit/fgbasic.cpp | 26 +++++------ src/coreclr/jit/importer.cpp | 28 +++++++----- src/coreclr/jit/morph.cpp | 44 +++++++++++++++++-- .../CompilerServices/RuntimeHelpers.cs | 12 +++-- 5 files changed, 97 insertions(+), 35 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 8f2d8059cc4cab..94ea99d657a1da 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4504,10 +4504,11 @@ inline static bool StructHasNoPromotionFlagSet(DWORD attribs) return ((attribs & CORINFO_FLG_DONT_PROMOTE) != 0); } -/***************************************************************************** - * This node should not be referenced by anyone now. Set its values to garbage - * to catch extra references - */ +//------------------------------------------------------------------------------ +// DEBUG_DESTROY_NODE: sets value of tree to garbage to catch extra references +// +// Arguments: +// tree: This node should not be referenced by anyone now inline void DEBUG_DESTROY_NODE(GenTree* tree) { @@ -4529,6 +4530,19 @@ inline void DEBUG_DESTROY_NODE(GenTree* tree) #endif } +//------------------------------------------------------------------------------ +// DEBUG_DESTROY_NODE: sets value of trees to garbage to catch extra references +// +// Arguments: +// tree, ...rest: These nodes should not be referenced by anyone now + +template +void DEBUG_DESTROY_NODE(GenTree* tree, T... rest) +{ + DEBUG_DESTROY_NODE(tree); + DEBUG_DESTROY_NODE(rest...); +} + //------------------------------------------------------------------------------ // lvRefCnt: access reference count for this local var // diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 11621a75248218..224718c9e52f44 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1113,12 +1113,12 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed { ni = lookupNamedIntrinsic(methodHnd); - bool foldableIntrinsc = false; + bool foldableIntrinsic = false; if (IsMathIntrinsic(ni)) { // Most Math(F) intrinsics have single arguments - foldableIntrinsc = FgStack::IsConstantOrConstArg(pushedStack.Top(), impInlineInfo); + foldableIntrinsic = FgStack::IsConstantOrConstArg(pushedStack.Top(), impInlineInfo); } else { @@ -1131,7 +1131,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case NI_System_GC_KeepAlive: { pushedStack.PushUnknown(); - foldableIntrinsc = true; + foldableIntrinsic = true; break; } @@ -1147,7 +1147,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: pushedStack.PushConstant(); - foldableIntrinsc = true; + foldableIntrinsic = true; // we can add an additional boost if arg is really a const break; @@ -1165,10 +1165,10 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case NI_Vector128_Create: #endif { - // Top() in order to keep it as is in case of foldableIntrinsc + // Top() in order to keep it as is in case of foldableIntrinsic if (FgStack::IsConstantOrConstArg(pushedStack.Top(), impInlineInfo)) { - foldableIntrinsc = true; + foldableIntrinsic = true; } break; } @@ -1183,7 +1183,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed if (FgStack::IsConstantOrConstArg(pushedStack.Top(0), impInlineInfo) && FgStack::IsConstantOrConstArg(pushedStack.Top(1), impInlineInfo)) { - foldableIntrinsc = true; + foldableIntrinsic = true; pushedStack.PushConstant(); } break; @@ -1192,31 +1192,31 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed case NI_IsSupported_True: case NI_IsSupported_False: { - foldableIntrinsc = true; + foldableIntrinsic = true; pushedStack.PushConstant(); break; } #if defined(TARGET_XARCH) && defined(FEATURE_HW_INTRINSICS) case NI_Vector128_get_Count: case NI_Vector256_get_Count: - foldableIntrinsc = true; + foldableIntrinsic = true; pushedStack.PushConstant(); // TODO: check if it's a loop condition - we unroll such loops. break; case NI_Vector256_get_Zero: case NI_Vector256_get_AllBitsSet: - foldableIntrinsc = true; + foldableIntrinsic = true; pushedStack.PushUnknown(); break; #elif defined(TARGET_ARM64) && defined(FEATURE_HW_INTRINSICS) case NI_Vector64_get_Count: case NI_Vector128_get_Count: - foldableIntrinsc = true; + foldableIntrinsic = true; pushedStack.PushConstant(); break; case NI_Vector128_get_Zero: case NI_Vector128_get_AllBitsSet: - foldableIntrinsc = true; + foldableIntrinsic = true; pushedStack.PushUnknown(); break; #endif @@ -1228,7 +1228,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } } - if (foldableIntrinsc) + if (foldableIntrinsic) { compInlineResult->Note(InlineObservation::CALLSITE_FOLDABLE_INTRINSIC); handled = true; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index f819e43a7736aa..eba9df6e20b3a8 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4015,18 +4015,24 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: { - GenTree* op1 = impPopStack().val; - if (op1->OperIsConst() || gtFoldExpr(op1)->OperIsConst()) - { - // op1 is a known constant, replace with 'true'. - retNode = gtNewIconNode(1); - // We can also consider FTN_ADDR and typeof(T) here - } - else + if (opts.OptimizationEnabled()) { - // op1 is not a known constant, replace with 'false'. - // TODO: delay till e.g. morph and try again, maybe op1 will become a const. - retNode = gtNewIconNode(0); + GenTree* op1 = impPopStack().val; + if (op1->OperIsConst() || gtFoldExpr(op1)->OperIsConst()) + { + // op1 is a known constant, replace with 'true'. + impPopStack(); + retNode = gtNewIconNode(1); + JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to true early\n"); + // We can also consider FTN_ADDR and typeof(T) here + } + else + { + // op1 is not a known constant, we'll do the expansion in morph + retNode = new (this, GT_INTRINSIC) GenTreeIntrinsic(TYP_INT, op1, ni, method); + JITDUMP("\nConverting RuntimeHelpers.IsKnownConstant to:\n"); + DISPTREE(retNode); + } } break; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 1ee03c84a398f0..74de6b2abb058d 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11523,9 +11523,11 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) case GT_RUNTIMELOOKUP: return fgMorphTree(op1); -#ifdef TARGET_ARM case GT_INTRINSIC: - if (tree->AsIntrinsic()->gtIntrinsicName == NI_System_Math_Round) + { + NamedIntrinsic ni = tree->AsIntrinsic()->gtIntrinsicName; +#ifdef TARGET_ARM + if (ni == NI_System_Math_Round) { switch (tree->TypeGet()) { @@ -11537,8 +11539,44 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) unreached(); } } - break; #endif + if (ni == NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant) + { + // Should be expanded by the time it reaches CSE phase + assert(!optValnumCSE_phase); + + JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to "); + if (op1->OperIsConst()) + { + // We're lucky to catch a constant here while importer was not + JITDUMP("true\n"); + DEBUG_DESTROY_NODE(tree, op1); + tree = gtNewIconNode(1); + } + else + { + GenTree* op1SideEffects = nullptr; + gtExtractSideEffList(op1, &op1SideEffects); + if (op1SideEffects != nullptr) + { + DEBUG_DESTROY_NODE(tree); + // Keep side-effects of op1 + tree = gtNewOperNode(GT_COMMA, TYP_INT, op1SideEffects, gtNewIconNode(0)); + JITDUMP("false with side effects:\n") + DISPTREE(tree); + } + else + { + JITDUMP("true\n"); + DEBUG_DESTROY_NODE(tree, op1); + tree = gtNewIconNode(0); + } + } + INDEBUG(tree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + return tree; + } + } + break; case GT_PUTARG_TYPE: return fgMorphTree(tree->AsUnOp()->gtGetOp1()); diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs index 2f830f3b5793c5..7154b89a105f24 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs @@ -118,10 +118,14 @@ internal static bool IsPrimitiveType(this CorElementType et) [Intrinsic] public static unsafe ReadOnlySpan CreateSpan(RuntimeFieldHandle fldHandle) => new ReadOnlySpan(GetSpanDataFrom(fldHandle, typeof(T).TypeHandle, out int length), length); - /// - /// Returns true if input is a compile-time constant - /// + + // The following intrinsics return true if input is a compile-time constant + // Feel free to add more overloads on demand + + [Intrinsic] + internal static bool IsKnownConstant(string t) => false; + [Intrinsic] - internal static bool IsKnownConstant(T t) => false; + internal static bool IsKnownConstant(char t) => false; } } From 2ebdae996dbde2512c52c3bf30309a895061ee8b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 17:11:10 +0300 Subject: [PATCH 08/23] Make string overload nullable --- .../src/System/Runtime/CompilerServices/RuntimeHelpers.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs index 7154b89a105f24..498055ad6a1c51 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs @@ -123,7 +123,7 @@ internal static bool IsPrimitiveType(this CorElementType et) // Feel free to add more overloads on demand [Intrinsic] - internal static bool IsKnownConstant(string t) => false; + internal static bool IsKnownConstant(string? t) => false; [Intrinsic] internal static bool IsKnownConstant(char t) => false; From e264d2792bb50dcf47c735e0c87c7f6794ccb663 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 17:19:59 +0300 Subject: [PATCH 09/23] Use GTF_ALL_EFFECT --- src/coreclr/jit/importer.cpp | 2 +- src/coreclr/jit/morph.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index eba9df6e20b3a8..8146edd6390623 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4018,7 +4018,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, if (opts.OptimizationEnabled()) { GenTree* op1 = impPopStack().val; - if (op1->OperIsConst() || gtFoldExpr(op1)->OperIsConst()) + if (op1->OperIsConst()) { // op1 is a known constant, replace with 'true'. impPopStack(); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 74de6b2abb058d..4a847291dbedc9 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11556,7 +11556,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) else { GenTree* op1SideEffects = nullptr; - gtExtractSideEffList(op1, &op1SideEffects); + gtExtractSideEffList(op1, &op1SideEffects, GTF_ALL_EFFECT); if (op1SideEffects != nullptr) { DEBUG_DESTROY_NODE(tree); From f08a182240c9e1e4bed9af06681d857efabf2dfb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 17:41:22 +0300 Subject: [PATCH 10/23] Address feedback --- src/coreclr/jit/gentree.cpp | 3 ++ src/coreclr/jit/importer.cpp | 1 - src/coreclr/jit/morph.cpp | 82 ++++++++++++++++++------------------ 3 files changed, 45 insertions(+), 41 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 70000ed09ca12b..2615132e10f931 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10827,6 +10827,9 @@ void Compiler::gtDispTree(GenTree* tree, case NI_System_Object_GetType: printf(" objGetType"); break; + case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: + printf(" isKnownConst"); + break; default: unreached(); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 8146edd6390623..1a10b03ecb36c8 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4021,7 +4021,6 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, if (op1->OperIsConst()) { // op1 is a known constant, replace with 'true'. - impPopStack(); retNode = gtNewIconNode(1); JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to true early\n"); // We can also consider FTN_ADDR and typeof(T) here diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 4a847291dbedc9..837285c48e7775 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11523,11 +11523,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) case GT_RUNTIMELOOKUP: return fgMorphTree(op1); +#ifdef TARGET_ARM case GT_INTRINSIC: { - NamedIntrinsic ni = tree->AsIntrinsic()->gtIntrinsicName; -#ifdef TARGET_ARM - if (ni == NI_System_Math_Round) + if (tree->AsIntrinsic()->gtIntrinsicName == NI_System_Math_Round) { switch (tree->TypeGet()) { @@ -11539,44 +11538,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) unreached(); } } -#endif - if (ni == NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant) - { - // Should be expanded by the time it reaches CSE phase - assert(!optValnumCSE_phase); - - JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to "); - if (op1->OperIsConst()) - { - // We're lucky to catch a constant here while importer was not - JITDUMP("true\n"); - DEBUG_DESTROY_NODE(tree, op1); - tree = gtNewIconNode(1); - } - else - { - GenTree* op1SideEffects = nullptr; - gtExtractSideEffList(op1, &op1SideEffects, GTF_ALL_EFFECT); - if (op1SideEffects != nullptr) - { - DEBUG_DESTROY_NODE(tree); - // Keep side-effects of op1 - tree = gtNewOperNode(GT_COMMA, TYP_INT, op1SideEffects, gtNewIconNode(0)); - JITDUMP("false with side effects:\n") - DISPTREE(tree); - } - else - { - JITDUMP("true\n"); - DEBUG_DESTROY_NODE(tree, op1); - tree = gtNewIconNode(0); - } - } - INDEBUG(tree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); - return tree; - } } - break; +#endif case GT_PUTARG_TYPE: return fgMorphTree(tree->AsUnOp()->gtGetOp1()); @@ -13261,6 +13224,45 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) } break; + case GT_INTRINSIC: + if (tree->AsIntrinsic()->gtIntrinsicName == + NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant) + { + // Should be expanded by the time it reaches CSE phase + assert(!optValnumCSE_phase); + + JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to "); + if (op1->OperIsConst()) + { + // We're lucky to catch a constant here while importer was not + JITDUMP("true\n"); + DEBUG_DESTROY_NODE(tree, op1); + tree = gtNewIconNode(1); + } + else + { + GenTree* op1SideEffects = nullptr; + gtExtractSideEffList(op1, &op1SideEffects, GTF_ALL_EFFECT); + if (op1SideEffects != nullptr) + { + DEBUG_DESTROY_NODE(tree); + // Keep side-effects of op1 + tree = gtNewOperNode(GT_COMMA, TYP_INT, op1SideEffects, gtNewIconNode(0)); + JITDUMP("false with side effects:\n") + DISPTREE(tree); + } + else + { + JITDUMP("true\n"); + DEBUG_DESTROY_NODE(tree, op1); + tree = gtNewIconNode(0); + } + } + INDEBUG(tree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); + return tree; + } + break; + default: break; } From 8e5fd0db440489c05ad17793f4a1b57308859263 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 17:47:45 +0300 Subject: [PATCH 11/23] fix copy-paste --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 837285c48e7775..7dcb4e25c9a631 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13253,7 +13253,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) } else { - JITDUMP("true\n"); + JITDUMP("false\n"); DEBUG_DESTROY_NODE(tree, op1); tree = gtNewIconNode(0); } From e2ebf308d10e48f51908713d9bab05d65f074071 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 13 Jan 2022 18:09:40 +0300 Subject: [PATCH 12/23] Update src/coreclr/jit/compiler.hpp Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> --- src/coreclr/jit/compiler.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 94ea99d657a1da..35f0c4e3375f6f 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4509,7 +4509,7 @@ inline static bool StructHasNoPromotionFlagSet(DWORD attribs) // // Arguments: // tree: This node should not be referenced by anyone now - +// inline void DEBUG_DESTROY_NODE(GenTree* tree) { #ifdef DEBUG From 895abc4ea2762b57f3eae30e599a608a9bf5a7d0 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 13 Jan 2022 18:09:46 +0300 Subject: [PATCH 13/23] Update src/coreclr/jit/compiler.hpp Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> --- src/coreclr/jit/compiler.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index 35f0c4e3375f6f..309b7e1c78b110 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -4535,7 +4535,7 @@ inline void DEBUG_DESTROY_NODE(GenTree* tree) // // Arguments: // tree, ...rest: These nodes should not be referenced by anyone now - +// template void DEBUG_DESTROY_NODE(GenTree* tree, T... rest) { From 45bfe7c06e908c45952c3675ada2c0804d4034cf Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 18:12:15 +0300 Subject: [PATCH 14/23] Address feedback --- src/coreclr/jit/importer.cpp | 29 +++++++++++++---------------- src/coreclr/jit/morph.cpp | 3 +-- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 1a10b03ecb36c8..b2238c9ca25ce8 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -4015,23 +4015,20 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: { - if (opts.OptimizationEnabled()) + GenTree* op1 = impPopStack().val; + if (op1->OperIsConst()) { - GenTree* op1 = impPopStack().val; - if (op1->OperIsConst()) - { - // op1 is a known constant, replace with 'true'. - retNode = gtNewIconNode(1); - JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to true early\n"); - // We can also consider FTN_ADDR and typeof(T) here - } - else - { - // op1 is not a known constant, we'll do the expansion in morph - retNode = new (this, GT_INTRINSIC) GenTreeIntrinsic(TYP_INT, op1, ni, method); - JITDUMP("\nConverting RuntimeHelpers.IsKnownConstant to:\n"); - DISPTREE(retNode); - } + // op1 is a known constant, replace with 'true'. + retNode = gtNewIconNode(1); + JITDUMP("\nExpanding RuntimeHelpers.IsKnownConstant to true early\n"); + // We can also consider FTN_ADDR and typeof(T) here + } + else + { + // op1 is not a known constant, we'll do the expansion in morph + retNode = new (this, GT_INTRINSIC) GenTreeIntrinsic(TYP_INT, op1, ni, method); + JITDUMP("\nConverting RuntimeHelpers.IsKnownConstant to:\n"); + DISPTREE(retNode); } break; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 7dcb4e25c9a631..bb93c711057f54 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -11525,7 +11525,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) #ifdef TARGET_ARM case GT_INTRINSIC: - { if (tree->AsIntrinsic()->gtIntrinsicName == NI_System_Math_Round) { switch (tree->TypeGet()) @@ -11538,7 +11537,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) unreached(); } } - } + break; #endif case GT_PUTARG_TYPE: From 80f9ddd50cc67179e9969280ea9bc73480133636 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 19:15:26 +0300 Subject: [PATCH 15/23] Remove [MethodImpl(MethodImplOptions.AggressiveInlining)], handle it in the inliner --- src/coreclr/jit/fgbasic.cpp | 10 ++++++- src/coreclr/jit/inline.def | 2 ++ src/coreclr/jit/inlinepolicy.cpp | 27 ++++++++++++++++++- src/coreclr/jit/inlinepolicy.h | 4 +++ .../src/System/String.Comparison.cs | 2 -- 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 224718c9e52f44..659b6bc976c552 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1146,9 +1146,17 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: + if (FgStack::IsConstantOrConstArg(pushedStack.Top(), impInlineInfo)) + { + compInlineResult->Note(InlineObservation::CALLEE_CONST_ARG_FEEDS_ISCONST); + } + else + { + compInlineResult->Note(InlineObservation::CALLEE_ARG_FEEDS_ISCONST); + } + // RuntimeHelpers.IsKnownConstant is always folded into a const pushedStack.PushConstant(); foldableIntrinsic = true; - // we can add an additional boost if arg is really a const break; // These are foldable if the first argument is a constant diff --git a/src/coreclr/jit/inline.def b/src/coreclr/jit/inline.def index a28daf4e980fdc..a519471a3caec2 100644 --- a/src/coreclr/jit/inline.def +++ b/src/coreclr/jit/inline.def @@ -70,6 +70,8 @@ INLINE_OBSERVATION(ARG_FEEDS_CONSTANT_TEST, bool, "argument feeds constant t INLINE_OBSERVATION(ARG_FEEDS_TEST, bool, "argument feeds test", INFORMATION, CALLEE) INLINE_OBSERVATION(ARG_FEEDS_CAST, int, "argument feeds castclass or isinst", INFORMATION, CALLEE) INLINE_OBSERVATION(ARG_FEEDS_RANGE_CHECK, bool, "argument feeds range check", INFORMATION, CALLEE) +INLINE_OBSERVATION(ARG_FEEDS_ISCONST, bool, "argument feeds IsKnownConstant", INFORMATION, CALLEE) +INLINE_OBSERVATION(CONST_ARG_FEEDS_ISCONST, bool, "const argument feeds IsKnownConstant", INFORMATION, CALLEE) INLINE_OBSERVATION(ARG_STRUCT, int, "arg is a struct passed by value", INFORMATION, CALLEE) INLINE_OBSERVATION(RETURNS_STRUCT, bool, "returns a struct by value", INFORMATION, CALLEE) INLINE_OBSERVATION(ARG_STRUCT_FIELD_ACCESS, int, "ldfld/stfld over arg (struct)", INFORMATION, CALLEE) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 1ec47378872d54..46a740806936e4 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -326,6 +326,14 @@ void DefaultPolicy::NoteBool(InlineObservation obs, bool value) m_ArgFeedsRangeCheck++; break; + case InlineObservation::CALLEE_CONST_ARG_FEEDS_ISCONST: + m_ConstArgFeedsIsKnownConst = true; + break; + + case InlineObservation::CALLEE_ARG_FEEDS_ISCONST: + m_ArgFeedsIsKnownConst = true; + break; + case InlineObservation::CALLEE_UNSUPPORTED_OPCODE: propagate = true; break; @@ -727,6 +735,21 @@ double DefaultPolicy::DetermineMultiplier() JITDUMP("\nInline candidate has arg that feeds range check. Multiplier increased to %g.", multiplier); } + if (m_ConstArgFeedsIsKnownConst) + { + // if we use RuntimeHelpers.IsKnownConstant we most likely expect our function to be always inlined + // at least in the case of constant arguments + multiplier += 15; + JITDUMP("\nConstant argument feeds RuntimeHelpers.IsKnownConstant. Multiplier increased to %g.", multiplier); + } + + if (m_ArgFeedsIsKnownConst) + { + // In IsPrejitRoot we don't have callsite info so let's assume we have a constant here in order to avoid "baked" noinline + multiplier += m_IsPrejitRoot ? 10 : 2; + JITDUMP("\nConstant argument feeds RuntimeHelpers.IsKnownConstant. Multiplier increased to %g.", multiplier); + } + if (m_ConstantArgFeedsConstantTest > 0) { multiplier += 3.0; @@ -1381,7 +1404,7 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) bbLimit += 5 + m_Switch * 10; } bbLimit += m_FoldableBranch + m_FoldableSwitch * 10; - if ((unsigned)value > bbLimit) + if ((unsigned)value > bbLimit && !m_ConstArgFeedsIsKnownConst && !m_ArgFeedsIsKnownConst) { SetNever(InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS); } @@ -2638,6 +2661,8 @@ void DiscretionaryPolicy::DumpData(FILE* file) const fprintf(file, ",%u", m_ArgFeedsConstantTest); fprintf(file, ",%u", m_MethodIsMostlyLoadStore ? 1 : 0); fprintf(file, ",%u", m_ArgFeedsRangeCheck); + fprintf(file, ",%u", m_ConstArgFeedsIsKnownConst ? 1 : 0); + fprintf(file, ",%u", m_ArgFeedsIsKnownConst ? 1 : 0); fprintf(file, ",%u", m_ConstantArgFeedsConstantTest); fprintf(file, ",%d", m_CalleeNativeSizeEstimate); fprintf(file, ",%d", m_CallsiteNativeSizeEstimate); diff --git a/src/coreclr/jit/inlinepolicy.h b/src/coreclr/jit/inlinepolicy.h index f5f200e9f5bd71..05e8539982ee88 100644 --- a/src/coreclr/jit/inlinepolicy.h +++ b/src/coreclr/jit/inlinepolicy.h @@ -110,6 +110,8 @@ class DefaultPolicy : public LegalPolicy , m_CallsiteIsInLoop(false) , m_IsNoReturn(false) , m_IsNoReturnKnown(false) + , m_ConstArgFeedsIsKnownConst(false) + , m_ArgFeedsIsKnownConst(false) { // empty } @@ -178,6 +180,8 @@ class DefaultPolicy : public LegalPolicy bool m_CallsiteIsInLoop : 1; bool m_IsNoReturn : 1; bool m_IsNoReturnKnown : 1; + bool m_ConstArgFeedsIsKnownConst : 1; + bool m_ArgFeedsIsKnownConst : 1; }; // ExtendedDefaultPolicy is a slightly more aggressive variant of diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index c0131f4645b081..085807f2b8e659 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -681,7 +681,6 @@ public bool Equals([NotNullWhen(true)] string? value, StringComparison compariso } // Determines whether two Strings match. - [MethodImpl(MethodImplOptions.AggressiveInlining)] public static bool Equals(string? a, string? b) { // if either a or b are "" - optimize Equals to just 'str?.Length == 0' @@ -1026,7 +1025,6 @@ public bool StartsWith(string value, bool ignoreCase, CultureInfo? culture) return referenceCulture.CompareInfo.IsPrefix(this, value, ignoreCase ? CompareOptions.IgnoreCase : CompareOptions.None); } - [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool StartsWith(char value) { if (RuntimeHelpers.IsKnownConstant(value) && value != '\0') From eebd8d155a99cc10800e24dfb7f0b8629e4a5668 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 19:16:29 +0300 Subject: [PATCH 16/23] clean up --- src/coreclr/jit/inlinepolicy.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 46a740806936e4..685a0d3d01ba81 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -745,7 +745,8 @@ double DefaultPolicy::DetermineMultiplier() if (m_ArgFeedsIsKnownConst) { - // In IsPrejitRoot we don't have callsite info so let's assume we have a constant here in order to avoid "baked" noinline + // In IsPrejitRoot we don't have callsite info so let's assume we have a constant here in order to avoid "baked" + // noinline multiplier += m_IsPrejitRoot ? 10 : 2; JITDUMP("\nConstant argument feeds RuntimeHelpers.IsKnownConstant. Multiplier increased to %g.", multiplier); } From e0e3e5199f2565789523e7ed288062cdd8bc791f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 13 Jan 2022 21:56:21 +0300 Subject: [PATCH 17/23] Only boost for constant-arg cases --- src/coreclr/jit/inlinepolicy.cpp | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp index 685a0d3d01ba81..0ea9135b82c249 100644 --- a/src/coreclr/jit/inlinepolicy.cpp +++ b/src/coreclr/jit/inlinepolicy.cpp @@ -735,19 +735,12 @@ double DefaultPolicy::DetermineMultiplier() JITDUMP("\nInline candidate has arg that feeds range check. Multiplier increased to %g.", multiplier); } - if (m_ConstArgFeedsIsKnownConst) + if (m_ConstArgFeedsIsKnownConst || (m_ArgFeedsIsKnownConst && m_IsPrejitRoot)) { // if we use RuntimeHelpers.IsKnownConstant we most likely expect our function to be always inlined - // at least in the case of constant arguments - multiplier += 15; - JITDUMP("\nConstant argument feeds RuntimeHelpers.IsKnownConstant. Multiplier increased to %g.", multiplier); - } - - if (m_ArgFeedsIsKnownConst) - { - // In IsPrejitRoot we don't have callsite info so let's assume we have a constant here in order to avoid "baked" - // noinline - multiplier += m_IsPrejitRoot ? 10 : 2; + // at least in the case of constant arguments. In IsPrejitRoot we don't have callsite info so let's + // assume we have a constant here in order to avoid "baked" noinline + multiplier += 20; JITDUMP("\nConstant argument feeds RuntimeHelpers.IsKnownConstant. Multiplier increased to %g.", multiplier); } @@ -1395,7 +1388,7 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) { SetNever(InlineObservation::CALLEE_DOES_NOT_RETURN); } - else if (!m_IsForceInline && !m_HasProfile) + else if (!m_IsForceInline && !m_HasProfile && !m_ConstArgFeedsIsKnownConst && !m_ArgFeedsIsKnownConst) { unsigned bbLimit = (unsigned)JitConfig.JitExtDefaultPolicyMaxBB(); if (m_IsPrejitRoot) @@ -1405,7 +1398,8 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value) bbLimit += 5 + m_Switch * 10; } bbLimit += m_FoldableBranch + m_FoldableSwitch * 10; - if ((unsigned)value > bbLimit && !m_ConstArgFeedsIsKnownConst && !m_ArgFeedsIsKnownConst) + + if ((unsigned)value > bbLimit) { SetNever(InlineObservation::CALLEE_TOO_MANY_BASIC_BLOCKS); } From 5b2fbfe6754acd4a22f4519252a8eb5f0c3d221c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 14 Jan 2022 01:30:32 +0300 Subject: [PATCH 18/23] Remove redundant opts.OptimizationEnabled()s --- src/coreclr/jit/importer.cpp | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index b2238c9ca25ce8..ab94202d52cfaa 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3941,29 +3941,19 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_String_get_Length: { GenTree* op1 = impPopStack().val; - if (opts.OptimizationEnabled()) + if (op1->OperIs(GT_CNS_STR)) { - if (op1->OperIs(GT_CNS_STR)) + // Optimize `ldstr + String::get_Length()` to CNS_INT + // e.g. "Hello".Length => 5 + GenTreeIntCon* iconNode = gtNewStringLiteralLength(op1->AsStrCon()); + if (iconNode != nullptr) { - // Optimize `ldstr + String::get_Length()` to CNS_INT - // e.g. "Hello".Length => 5 - GenTreeIntCon* iconNode = gtNewStringLiteralLength(op1->AsStrCon()); - if (iconNode != nullptr) - { - retNode = iconNode; - break; - } + retNode = iconNode; + break; } - GenTreeArrLen* arrLen = gtNewArrLen(TYP_INT, op1, OFFSETOF__CORINFO_String__stringLen, compCurBB); - op1 = arrLen; - } - else - { - /* Create the expression "*(str_addr + stringLengthOffset)" */ - op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, - gtNewIconNode(OFFSETOF__CORINFO_String__stringLen, TYP_I_IMPL)); - op1 = gtNewOperNode(GT_IND, TYP_INT, op1); } + GenTreeArrLen* arrLen = gtNewArrLen(TYP_INT, op1, OFFSETOF__CORINFO_String__stringLen, compCurBB); + op1 = arrLen; // Getting the length of a null string should throw op1->gtFlags |= GTF_EXCEPT; @@ -4309,7 +4299,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_Threading_Thread_get_ManagedThreadId: { - if (opts.OptimizationEnabled() && impStackTop().val->OperIs(GT_RET_EXPR)) + if (impStackTop().val->OperIs(GT_RET_EXPR)) { GenTreeCall* call = impStackTop().val->AsRetExpr()->gtInlineCandidate->AsCall(); if (call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC) @@ -4332,7 +4322,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_Threading_Interlocked_Or: case NI_System_Threading_Interlocked_And: { - if (opts.OptimizationEnabled() && compOpportunisticallyDependsOn(InstructionSet_Atomics)) + if (compOpportunisticallyDependsOn(InstructionSet_Atomics)) { assert(sig->numArgs == 2); GenTree* op2 = impPopStack().val; From 30e18554042cb9fb722f15d51eacec7100d42583 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 14 Jan 2022 13:18:56 +0300 Subject: [PATCH 19/23] Update importer.cpp --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index ab94202d52cfaa..df3066d5bc94a6 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3884,7 +3884,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // CreateSpan must be expanded for NativeAOT case NI_System_Runtime_CompilerServices_RuntimeHelpers_CreateSpan: case NI_System_Runtime_CompilerServices_RuntimeHelpers_InitializeArray: - mustExpand = IsTargetAbi(CORINFO_CORERT_ABI); + mustExpand |= IsTargetAbi(CORINFO_CORERT_ABI); break; case NI_System_ByReference_ctor: From 3cb840200c0b8dcd71b39f7367d2cd30117042a1 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 14 Jan 2022 13:29:22 +0300 Subject: [PATCH 20/23] Update String.Comparison.cs --- .../src/System/String.Comparison.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index 085807f2b8e659..9d5cf7b5dbd482 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -685,14 +685,14 @@ public static bool Equals(string? a, string? b) { // if either a or b are "" - optimize Equals to just 'str?.Length == 0' // Otherwise, these two blocks are eliminated since IsKnownConstant is a jit-time constant - if (RuntimeHelpers.IsKnownConstant(a) && a?.Length == 0) + if (RuntimeHelpers.IsKnownConstant(a) && a != null && a.Length == 0) { - return b?.Length == 0; + return b != null && b.Length == 0; } - if (RuntimeHelpers.IsKnownConstant(b) && b?.Length == 0) + if (RuntimeHelpers.IsKnownConstant(b) && b != null && b.Length == 0) { - return a?.Length == 0; + return a != null && a.Length == 0; } if (object.ReferenceEquals(a, b)) From 7c7c26461891189c744c3db8334618dcffb6971c Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 14 Jan 2022 13:34:01 +0300 Subject: [PATCH 21/23] Update String.Comparison.cs --- .../System.Private.CoreLib/src/System/String.Comparison.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs index 9d5cf7b5dbd482..6a059351f376ac 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs @@ -683,8 +683,8 @@ public bool Equals([NotNullWhen(true)] string? value, StringComparison compariso // Determines whether two Strings match. public static bool Equals(string? a, string? b) { - // if either a or b are "" - optimize Equals to just 'str?.Length == 0' - // Otherwise, these two blocks are eliminated since IsKnownConstant is a jit-time constant + // Transform 'str == ""' to 'str != null && str.Length == 0' if either a or b are jit-time + // constants. Otherwise, these two blocks are eliminated if (RuntimeHelpers.IsKnownConstant(a) && a != null && a.Length == 0) { return b != null && b.Length == 0; From a9b85fce27e5f2169d61a4313fe405ad012d34d2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 14 Jan 2022 17:14:14 +0300 Subject: [PATCH 22/23] Add tests --- .../JIT/opt/IsKnownConstant/StringEquals.cs | 99 +++++++++++++++++++ .../opt/IsKnownConstant/StringEquals.csproj | 9 ++ 2 files changed, 108 insertions(+) create mode 100644 src/tests/JIT/opt/IsKnownConstant/StringEquals.cs create mode 100644 src/tests/JIT/opt/IsKnownConstant/StringEquals.csproj diff --git a/src/tests/JIT/opt/IsKnownConstant/StringEquals.cs b/src/tests/JIT/opt/IsKnownConstant/StringEquals.cs new file mode 100644 index 00000000000000..4a109ae5d65483 --- /dev/null +++ b/src/tests/JIT/opt/IsKnownConstant/StringEquals.cs @@ -0,0 +1,99 @@ +// 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.Runtime.CompilerServices; + +public class Program +{ + public static int Main() + { + AssertEquals(true, TestEquals1("")); + AssertEquals(false, TestEquals1(null)); + AssertEquals(false, TestEquals1("1")); + + AssertEquals(true, TestEquals2("")); + AssertEquals(false, TestEquals2(null)); + AssertEquals(false, TestEquals2("1")); + + AssertEquals(true, TestEquals3("")); + AssertEquals(false, TestEquals3(null)); + AssertEquals(false, TestEquals3("1")); + + AssertEquals(true, TestEquals4("")); + AssertEquals(false, TestEquals4(null)); + AssertEquals(false, TestEquals4("1")); + + AssertEquals(true, TestEquals5("")); + AssertEquals(false, TestEquals5(null)); + AssertEquals(false, TestEquals5("1")); + + AssertEquals(false, TestEquals6("")); + AssertEquals(true, TestEquals6(null)); + AssertEquals(false, TestEquals6("1")); + + AssertEquals(false, TestEquals7()); + AssertEquals(false, TestEquals8()); + + AssertEquals(true, TestEquals5("")); + AssertEquals(false, TestEquals5(null)); + AssertEquals(false, TestEquals5("1")); + + AssertEquals(true, TestStartWith("c")); + AssertEquals(false, TestStartWith("C")); + AssertThrowsNRE(() => TestStartWith(null)); + + return 100; + } + + private static void AssertEquals(bool expected, bool actual, [CallerLineNumber]int l = 0) + { + if (expected != actual) + throw new InvalidOperationException(); + } + + private static void AssertThrowsNRE(Action a) + { + try + { + a(); + } + catch (NullReferenceException) + { + return; + } + throw new InvalidOperationException(); + } + + private static string NullStr() => null; + private static string EmptyStr() => ""; + private static string NonEmptyStr() => "1"; + + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals1(string str) => str == ""; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals2(string str) => str == string.Empty; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals3(string str) => "" == str; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals4(string str) => string.Empty == str; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals5(string str) => string.Empty == str; + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals6(string str) => string.Equals(NullStr(), str); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals7() => string.Equals(NullStr(), EmptyStr()); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestEquals8() => string.Equals(NullStr(), NonEmptyStr()); + + [MethodImpl(MethodImplOptions.NoInlining)] + private static bool TestStartWith(string str) => str.StartsWith('c'); +} diff --git a/src/tests/JIT/opt/IsKnownConstant/StringEquals.csproj b/src/tests/JIT/opt/IsKnownConstant/StringEquals.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/opt/IsKnownConstant/StringEquals.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From 4947947d8e63eadd09f42ffce6a6cc69b6a295fe Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 15 Jan 2022 00:09:38 +0300 Subject: [PATCH 23/23] Fix unrelated invalid IR, we should not import TYP_USHORT constant --- src/coreclr/jit/fgbasic.cpp | 2 +- src/coreclr/jit/morph.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp index 659b6bc976c552..d7949f08612e87 100644 --- a/src/coreclr/jit/fgbasic.cpp +++ b/src/coreclr/jit/fgbasic.cpp @@ -1146,7 +1146,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed } case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant: - if (FgStack::IsConstantOrConstArg(pushedStack.Top(), impInlineInfo)) + if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo)) { compInlineResult->Note(InlineObservation::CALLEE_CONST_ARG_FEEDS_ISCONST); } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bb93c711057f54..230c77887b572f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5198,7 +5198,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) asIndex->Arr()->AsStrCon()->gtSconCPX, &length); if ((cnsIndex < length) && (str != nullptr)) { - GenTree* cnsCharNode = gtNewIconNode(str[cnsIndex], elemTyp); + GenTree* cnsCharNode = gtNewIconNode(str[cnsIndex], TYP_INT); INDEBUG(cnsCharNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED); return cnsCharNode; }