Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add IsKnownConstant jit helper and optimize 'str == ""' with str.StartsWith('c') #63734

Merged
merged 26 commits into from
Jan 19, 2022
Merged

Add IsKnownConstant jit helper and optimize 'str == ""' with str.StartsWith('c') #63734

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9f55fa6
Don't reuse args in EnC
EgorBo Jan 12, 2022
7d3ef94
Merge branch 'main' of https://github.com/dotnet/runtime into fix-enc…
EgorBo Jan 13, 2022
0a717aa
Introduce RuntimeHelpers.IsKnownConstant and use for 'str == ""'
EgorBo Jan 13, 2022
407a953
Remove unrelated change, add gtFoldExpr
EgorBo Jan 13, 2022
9d45461
Also, optimize String.StartsWith
EgorBo Jan 13, 2022
2f8a034
Update comment
EgorBo Jan 13, 2022
4883a46
Update src/libraries/System.Private.CoreLib/src/System/String.Compari…
EgorBo Jan 13, 2022
aa88cf9
Delay expansion to morph, replace generic signature with overloads
EgorBo Jan 13, 2022
64843f0
Merge branch 'jit-isknownconstant' of https://github.com/EgorBo/runti…
EgorBo Jan 13, 2022
2ebdae9
Make string overload nullable
EgorBo Jan 13, 2022
e264d27
Use GTF_ALL_EFFECT
EgorBo Jan 13, 2022
f08a182
Address feedback
EgorBo Jan 13, 2022
8e5fd0d
fix copy-paste
EgorBo Jan 13, 2022
e2ebf30
Update src/coreclr/jit/compiler.hpp
EgorBo Jan 13, 2022
895abc4
Update src/coreclr/jit/compiler.hpp
EgorBo Jan 13, 2022
45bfe7c
Address feedback
EgorBo Jan 13, 2022
80f9ddd
Remove [MethodImpl(MethodImplOptions.AggressiveInlining)], handle it …
EgorBo Jan 13, 2022
eebd8d1
clean up
EgorBo Jan 13, 2022
e0e3e51
Only boost for constant-arg cases
EgorBo Jan 13, 2022
5b2fbfe
Remove redundant opts.OptimizationEnabled()s
EgorBo Jan 13, 2022
30e1855
Update importer.cpp
EgorBo Jan 14, 2022
3cb8402
Update String.Comparison.cs
EgorBo Jan 14, 2022
7c7c264
Update String.Comparison.cs
EgorBo Jan 14, 2022
a9b85fc
Add tests
EgorBo Jan 14, 2022
a3e5f3c
Merge branch 'jit-isknownconstant' of https://github.com/EgorBo/runti…
EgorBo Jan 14, 2022
4947947
Fix unrelated invalid IR, we should not import TYP_USHORT constant
EgorBo Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions src/coreclr/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
Expand All @@ -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

EgorBo marked this conversation as resolved.
Show resolved Hide resolved
template <typename... T>
void DEBUG_DESTROY_NODE(GenTree* tree, T... rest)
{
DEBUG_DESTROY_NODE(tree);
DEBUG_DESTROY_NODE(rest...);
}

//------------------------------------------------------------------------------
// lvRefCnt: access reference count for this local var
//
Expand Down
30 changes: 18 additions & 12 deletions src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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;
}

Expand All @@ -1145,6 +1145,12 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant:
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
case NI_System_Type_get_IsValueType:
case NI_System_Type_GetTypeFromHandle:
Expand All @@ -1159,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;
}
Expand All @@ -1177,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;
Expand All @@ -1186,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
Expand All @@ -1222,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;
Expand Down
3 changes: 3 additions & 0 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
53 changes: 43 additions & 10 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -4007,6 +4013,29 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis,
break;
}

case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant:
{
if (opts.OptimizationEnabled())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to check OptimizationEnabled here? We have have early out for disabled optimizations before this switch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unnecessary OptimizationEnabled check is in a few other intrinsics as well. You can delete it there too while you are on it.

{
GenTree* op1 = impPopStack().val;
if (op1->OperIsConst())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String literals get fetched via extra indirections. How does this work? Where do the extra indirections get the GTK_CONST flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, the PR description mentions that this does not work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string.Empty and static readonly string don't work (but can be arranged). Normal string literals do work because we import them as GT_CNS_STR and expand to indirections later in morph

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly for diffs, we prefer string.Empty over "" in BCL, also all comparisons against empty strings were manually converted to str?.Length == 0 over the codebase in https://github.com/dotnet/runtime/pull/36443/files

Copy link
Member

@stephentoub stephentoub Jan 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also all comparisons against empty strings were manually converted to str?.Length == 0 over the codebase in https://github.com/dotnet/runtime/pull/36443/files

To be clear, they weren't converted to str?.Length == 0, or at least not 99% of them (I just re-skimmed the diff and didn't see any of those). These were all cases where we'd already checked the string for null, and so a simple length check was sufficient and clear, or where the string was being checked for null or empty, and thus it was clearer and more consistent with guidelines to use IsNullOrEmpty. I'd have pushed back on a change that tried to use str?.Length == 0 everywhere :)

(That said, it speaks to just how common == "" actually is in code.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub ah makes sense for me now why your analyzer found 100 cases in #63719

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should CA1820 be removed when all these changes defeat its perf improvement reasoning?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should CA1820 be removed when all these changes defeat its perf improvement reasoning?

It's already disabled by default. If there ends up being zero benefit after these improvements (including subsequent ones to get string.Empty and the like), we could consider deleting it entirely from the SDK. I wouldn't rush to that, though.

{
// 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;
}

case NI_System_Activator_AllocatorOf:
case NI_System_Activator_DefaultConstructorOf:
case NI_System_Object_MethodTableOf:
Expand Down Expand Up @@ -5352,6 +5381,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)
{
Expand Down
42 changes: 41 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11525,6 +11525,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)

#ifdef TARGET_ARM
case GT_INTRINSIC:
{
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
if (tree->AsIntrinsic()->gtIntrinsicName == NI_System_Math_Round)
{
switch (tree->TypeGet())
Expand All @@ -11537,7 +11538,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
unreached();
}
}
break;
}
#endif

case GT_PUTARG_TYPE:
Expand Down Expand Up @@ -13223,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("false\n");
DEBUG_DESTROY_NODE(tree, op1);
tree = gtNewIconNode(0);
}
}
INDEBUG(tree->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
return tree;
}
break;

default:
break;
}
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/namedintrinsiclist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
10 changes: 10 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,5 +117,15 @@ internal static bool IsPrimitiveType(this CorElementType et)
/// <remarks>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.</remarks>
[Intrinsic]
public static unsafe ReadOnlySpan<T> CreateSpan<T>(RuntimeFieldHandle fldHandle) => new ReadOnlySpan<T>(GetSpanDataFrom(fldHandle, typeof(T).TypeHandle, out int length), length);


// 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(char t) => false;
}
}
23 changes: 22 additions & 1 deletion src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,8 +681,21 @@ 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'
// Otherwise, these two blocks are eliminated since IsKnownConstant is a jit-time constant
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;
Expand Down Expand Up @@ -1013,7 +1026,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) && value != '\0')
{
return _firstChar == value;
}
return Length != 0 && _firstChar == value;
}

internal static void CheckStringComparison(StringComparison comparisonType)
{
Expand Down