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

EgorBo · 2022-01-13T11:52:36Z

Introduce an internal API bool RuntimeHelpers.IsKnownConstant<T>(T t) (see #11484) and optimize String.Equals with it to simplify codegen for comparisons against const strings. Same for String.StartsWith(char c). Also, JIT's inliner was improved to recognize it.

Unfortunately, string.Empty is not yet recognized as a constant, it needs #44847

Example:

bool Test1(string s) => s == "";
bool Test2(string s) => s.StartsWith('s');

Current codegen

; Method Program:Test1(System.String):bool
       sub      rsp, 40
       mov      r8, 0xD1FFAB1E      ; ""
       mov      rdx, gword ptr [r8]
       cmp      rcx, rdx
       jne      SHORT G_M22776_IG04
       mov      eax, 1
       jmp      SHORT G_M22776_IG07
G_M22776_IG04:
       test     rcx, rcx
       je       SHORT G_M22776_IG05
       mov      r8d, dword ptr [rcx+8]
       test     r8d, r8d
       je       SHORT G_M22776_IG06
G_M22776_IG05:
       xor      eax, eax
       jmp      SHORT G_M22776_IG07
G_M22776_IG06:
       add      rcx, 12
       add      rdx, 12
       add      r8d, r8d
       call     System.SpanHelpers:SequenceEqual(byref,byref,long):bool
G_M22776_IG07:
       nop      
       add      rsp, 40
       ret      
; Total bytes of code: 69


; Method Program:Test2(System.String):bool
       cmp      dword ptr [rcx+8], 0
       je       SHORT G_M8187_IG04
       cmp      word  ptr [rcx+12], 115
       sete     al
       movzx    rax, al
       jmp      SHORT G_M8187_IG05
G_M8187_IG04:
       xor      eax, eax
G_M8187_IG05:
       ret      
; Total bytes of code: 22

New codegen:

; Method Program:Test1(System.String):bool
       test     rcx, rcx
       jne      SHORT G_M22776_IG04
       xor      eax, eax
       jmp      SHORT G_M22776_IG05
G_M22776_IG04:
       cmp      dword ptr [rcx+8], 0
       sete     al
       movzx    rax, al
G_M22776_IG05:
       ret      
; Total bytes of code: 20

; Method Program:Test2(System.String):bool
       cmp      word  ptr [rcx+12], 115
       sete     al
       movzx    rax, al
       ret      
; Total bytes of code: 12

NOTE: no diffs for these functions when args aren't constants

Can be also implemented in Mono (e.g. for Mono-LLVM: https://llvm.org/docs/LangRef.html#llvm-is-constant-intrinsic)

Closes #11484
Closes #63719
Closes #41779

Not sure about "generic" signature here, it might e.g. brake inlining due to runtime-lookup. Perhaps, it's better to introduce overloads for all primitives (or add overloads on demand)? Thoughts? cc @stephentoub @GrabYourPitchforks @jkotas @dotnet/jit-contrib

…-case

ghost · 2022-01-13T11:52:46Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Introduce an internal API bool RuntimeHelpers.IsKnownConstant<T>(T t) (see #11484) and optimize String.Equals with it to simplify codegen for comparisons against const strings. Same for String.StartsWith(char c)

Unfortunately, string.Empty is not yet recognized as a constant, it needs #44847

Example:

bool Test(string s) => s == "";

Current codegen

; Method Program:Test1(System.String):bool
       sub      rsp, 40
       mov      r8, 0xD1FFAB1E      ; ""
       mov      rdx, gword ptr [r8]
       cmp      rcx, rdx
       jne      SHORT G_M22776_IG04
       mov      eax, 1
       jmp      SHORT G_M22776_IG07
G_M22776_IG04:
       test     rcx, rcx
       je       SHORT G_M22776_IG05
       mov      r8d, dword ptr [rcx+8]
       test     r8d, r8d
       je       SHORT G_M22776_IG06
G_M22776_IG05:
       xor      eax, eax
       jmp      SHORT G_M22776_IG07
G_M22776_IG06:
       add      rcx, 12
       add      rdx, 12
       add      r8d, r8d
       call     System.SpanHelpers:SequenceEqual(byref,byref,long):bool
G_M22776_IG07:
       nop      
       add      rsp, 40
       ret      
; Total bytes of code: 69


; Method Program:Test2(System.String):bool
       cmp      dword ptr [rcx+8], 0
       je       SHORT G_M8187_IG04
       cmp      word  ptr [rcx+12], 115
       sete     al
       movzx    rax, al
       jmp      SHORT G_M8187_IG05
G_M8187_IG04:
       xor      eax, eax
G_M8187_IG05:
       ret      
; Total bytes of code: 22

New codegen:

; Method Program:Test1(System.String):bool
       test     rcx, rcx
       jne      SHORT G_M22776_IG04
       xor      eax, eax
       jmp      SHORT G_M22776_IG05
G_M22776_IG04:
       cmp      dword ptr [rcx+8], 0
       sete     al
       movzx    rax, al
       ret      
; Total bytes of code: 20


; Method Program:Test2(System.String):bool
       cmp      word  ptr [rcx+12], 115
       sete     al
       movzx    rax, al
       ret      
; Total bytes of code: 12

TODO: Mono-Interp, Mono and Mono-LLVM.

Closes #11484
Closes #63719
Closes #41779

Not sure about "generic" signature here, it might e.g. brake inlining due to runtime-lookup. Perhaps, it's better to introduce overloads for all primitives? Thoughts? cc @stephentoub @GrabYourPitchforks @jkotas @dotnet/jit-contrib

Author:	EgorBo
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs

…son.cs Co-authored-by: Miha Zupan <[email protected]>

EgorBo · 2022-01-13T12:43:53Z

This API helps to declare optimizations directly in C# code, because str == "" optimization declared in JIT would look like this: EgorBo@7c4ef9f quite verbose

src/coreclr/jit/importer.cpp

src/coreclr/jit/fgbasic.cpp

…me-1 into jit-isknownconstant

src/coreclr/jit/morph.cpp

src/coreclr/jit/importer.cpp

src/coreclr/jit/morph.cpp

jkotas · 2022-01-13T14:59:15Z

src/coreclr/jit/importer.cpp


+            case NI_System_Runtime_CompilerServices_RuntimeHelpers_IsKnownConstant:
+            {
+                if (opts.OptimizationEnabled())


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

Thanks, removed

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

src/coreclr/jit/morph.cpp

src/coreclr/jit/compiler.hpp

jkotas · 2022-01-13T15:05:27Z

src/coreclr/jit/importer.cpp

+                if (opts.OptimizationEnabled())
+                {
+                    GenTree* op1 = impPopStack().val;
+                    if (op1->OperIsConst())


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

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

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

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

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.)

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

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

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.

Co-authored-by: SingleAccretion <[email protected]>

EgorBo · 2022-01-13T19:43:20Z

void Foo(int a, ...)
{
    var _ = RuntimeHelpers.IsKnownConstant(a);
    // a lot of code
}

A hack to force inliner to inline Foo if a is a constant at its callsite 😄

jkotas · 2022-01-13T19:45:48Z

src/coreclr/jit/inlinepolicy.cpp

+
+    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"


For my education, what is the callsite info that is missing for IsPrejitRoot?

@jkotas It's a separate run during prejitting where we just take every function (without context) and try to memorize all those which will never be inlined (by reporting to VM "mark this function as noinline" here). During that, we opportunistically assume the context is the most inliner friendly: all imaginary arguments are constants, imaginary callsite has PGO data and is hot, etc.. there was a better/detailed explanation from Andy somewhere

Right, we leverage prejitting to mark methods that will never be inlined so we don't spend any jit time analyzing them.

EgorBo · 2022-01-13T22:33:04Z

Jit-diff (-f --crossgen):

Total bytes of base: 63159582
Total bytes of diff: 63155432
Total bytes of delta: -4150 (-0.01 % of base)
Total relative delta: -5.39
    diff is an improvement.
    relative diff is an improvement.


Total byte diff includes 6 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    2 unique methods,        6 unique bytes

Top file regressions (bytes):
          67 : System.Management.dasm (0.02% of base)
          54 : System.Speech.dasm (0.01% of base)
          32 : System.Net.Http.dasm (0.00% of base)
          29 : System.DirectoryServices.AccountManagement.dasm (0.01% of base)

Top file improvements (bytes):
       -1002 : System.Private.Xml.dasm (-0.02% of base)
        -677 : System.Data.Common.dasm (-0.04% of base)
        -672 : System.ServiceModel.Syndication.dasm (-0.41% of base)
        -271 : System.Configuration.ConfigurationManager.dasm (-0.07% of base)
        -269 : System.Security.Cryptography.Pkcs.dasm (-0.06% of base)
        -212 : System.Security.Cryptography.Cng.dasm (-0.10% of base)
        -183 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base)
        -182 : FSharp.Core.dasm (-0.00% of base)
        -132 : System.Private.CoreLib.dasm (-0.00% of base)
        -105 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
         -83 : System.Security.Cryptography.dasm (-0.02% of base)
         -54 : Microsoft.CodeAnalysis.dasm (-0.00% of base)
         -53 : ILCompiler.TypeSystem.ReadyToRun.dasm (-0.02% of base)
         -53 : ILCompiler.TypeSystem.dasm (-0.02% of base)
         -48 : xunit.runner.utility.netcoreapp10.dasm (-0.02% of base)
         -48 : xunit.console.dasm (-0.05% of base)
         -41 : System.IO.Compression.dasm (-0.04% of base)
         -41 : System.ServiceProcess.ServiceController.dasm (-0.12% of base)
         -40 : Newtonsoft.Json.dasm (-0.00% of base)
         -39 : System.Private.Xml.Linq.dasm (-0.02% of base)

33 total files with Code Size differences (29 improved, 4 regressed), 241 unchanged.

Top method regressions (bytes):
         404 ( 1.84% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpCommandLineParser:Parse(IEnumerable`1,String,String,String):CSharpCommandLineArguments:this
          68 ( 0.46% of base) : System.Net.Http.dasm - <SendAsyncCore>d__60:MoveNext():this
          67 ( 0.58% of base) : System.Management.dasm - ManagementClassGenerator:GenerateProperties():this
          29 ( 1.85% of base) : System.Speech.dasm - XmlParser:ParseToken(IElement,XmlReader):IToken:this
          20 ( 0.58% of base) : System.DirectoryServices.AccountManagement.dasm - ADStoreCtx:FindPrincipalByIdentRefHelper(Type,String,String,DateTime,bool):Principal:this
          20 ( 0.48% of base) : System.Speech.dasm - XmlParser:ParseGrammar(XmlReader,IGrammar):this
          19 ( 0.72% of base) : System.Data.OleDb.dasm - OleDbMetaDataFactory:GetDataSourceInformationTable(OleDbConnection,OleDbConnectionInternal):DataTable:this
          10 ( 1.05% of base) : System.Private.Xml.dasm - ContainerAction:CompileStylesheetAttributes(Compiler):this
           9 ( 0.97% of base) : System.DirectoryServices.AccountManagement.dasm - ADStoreCtx:ScanACLForChangePasswordRight(ActiveDirectorySecurity,byref,byref,byref,byref)
           8 ( 0.35% of base) : System.Speech.dasm - XmlParser:ParseRule(IGrammar,XmlReader):IRule:this
           6 ( 0.68% of base) : System.Private.CoreLib.dasm - EventSource:AddProviderEnumKind(ManifestBuilder,FieldInfo,String)
           5 ( 0.74% of base) : System.Private.Xml.dasm - XmlSerializationWriter:GetQualifiedName(String,String):String:this
           4 ( 8.00% of base) : System.Private.CoreLib.dasm - String:Equals(String,String):bool
           3 ( 0.39% of base) : Microsoft.CodeAnalysis.dasm - MetadataHelpers:SplitQualifiedName(String):ImmutableArray`1
           3 (     ∞ of base) : System.Private.CoreLib.dasm - RuntimeHelpers:IsKnownConstant(String):bool (0 base, 1 diff methods)
           3 (     ∞ of base) : System.Private.CoreLib.dasm - RuntimeHelpers:IsKnownConstant(ushort):bool (0 base, 1 diff methods)
           2 ( 0.13% of base) : System.Speech.dasm - XmlParser:ParseRuleRef(IElement,XmlReader):IRuleRef:this
           2 ( 1.22% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:UriEqual(Uri,String,String,XmlResolver):bool:this
           1 ( 0.10% of base) : System.Private.Xml.dasm - XmlCustomFormatter:FromDefaultValue(Object,String):String
           1 ( 0.11% of base) : System.Speech.dasm - XmlParser:ParseItem(IElement,IRule,XmlReader):IItem:this

Top method improvements (bytes):
        -279 (-3.71% of base) : Microsoft.CodeAnalysis.CSharp.dasm - SyntaxFacts:GetKeywordKind(String):ushort
        -276 (-3.05% of base) : System.Data.Common.dasm - XmlTreeGen:SchemaTree(XmlDocument,XmlWriter,DataSet,DataTable,bool):this
        -239 (-12.13% of base) : System.ServiceModel.Syndication.dasm - Rss20FeedFormatter:ReadMediaEnclosure(XmlReader,Uri):SyndicationLink:this
        -203 (-1.43% of base) : System.Data.Common.dasm - XmlTreeGen:HandleTable(DataTable,XmlDocument,XmlElement,bool):XmlElement:this
        -186 (-4.15% of base) : System.Private.Xml.dasm - XmlSchemaInference:FindMatchingElement(bool,XmlReader,XmlSchemaComplexType,byref,byref,XmlSchema,bool):XmlSchemaElement:this
        -115 (-6.71% of base) : System.Security.Cryptography.Pkcs.dasm - EncodeHelpers:EncodeKeyTransRecipientInfo(CmsRecipient,HeapBlockRetainer):long
        -108 (-1.43% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Imports:FromSyntax(CSharpSyntaxNode,InContainerBinder,ConsList`1,bool):Imports
        -101 (-2.82% of base) : System.Configuration.ConfigurationManager.dasm - MgmtConfigurationRecord:GetConfigDeclarationUpdates(int):SectionUpdates:this
         -97 (-1.58% of base) : System.Data.Common.dasm - XSDSchema:LoadSchema(XmlSchemaSet,DataSet):this
         -94 (-1.12% of base) : System.Private.Xml.dasm - SchemaCollectionPreprocessor:Preprocess(XmlSchema,String,int):this
         -90 (-1.42% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ProviderManifest:ParseProviderEvents(Func`2,bool):this
         -90 (-2.25% of base) : System.Data.Common.dasm - XSDSchema:InstantiateTable(XmlSchemaElement,XmlSchemaComplexType,bool):DataTable:this
         -84 (-1.18% of base) : System.Private.Xml.dasm - XmlSerializationWriterCodeGen:GenerateMembersElement(XmlMembersMapping):String:this
         -80 (-2.50% of base) : System.Private.Xml.dasm - CompilerScopeManager`1:IsExNamespace(String):bool:this (8 methods)
         -78 (-6.61% of base) : System.ServiceModel.Syndication.dasm - Rss20FeedFormatter:ReadCategory(XmlReader,SyndicationCategory):this
         -77 (-7.51% of base) : System.Security.Cryptography.Pkcs.dasm - DSACmsSignature:Sign(ReadOnlySpan`1,HashAlgorithmName,X509Certificate2,AsymmetricAlgorithm,bool,byref,byref,byref):bool:this
         -76 (-1.35% of base) : System.Private.Xml.dasm - XmlSerializationWriterILGen:GenerateMembersElement(XmlMembersMapping):String:this
         -75 (-2.47% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Lexer:ScanXmlCrefToken(byref):bool:this
         -74 (-8.09% of base) : System.Security.Cryptography.Cng.dasm - <>c:<OpenProcessor>b__15_0(HashAlgorithmName):RsaPaddingProcessor:this
         -74 (-6.58% of base) : System.Private.Xml.dasm - XsltInput:ReadAttribute(byref):bool:this

Top method regressions (percentages):
           3 (     ∞ of base) : System.Private.CoreLib.dasm - RuntimeHelpers:IsKnownConstant(String):bool (0 base, 1 diff methods)
           3 (     ∞ of base) : System.Private.CoreLib.dasm - RuntimeHelpers:IsKnownConstant(ushort):bool (0 base, 1 diff methods)
           4 ( 8.00% of base) : System.Private.CoreLib.dasm - String:Equals(String,String):bool
          29 ( 1.85% of base) : System.Speech.dasm - XmlParser:ParseToken(IElement,XmlReader):IToken:this
         404 ( 1.84% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpCommandLineParser:Parse(IEnumerable`1,String,String,String):CSharpCommandLineArguments:this
           2 ( 1.22% of base) : System.Private.Xml.dasm - XmlTextReaderImpl:UriEqual(Uri,String,String,XmlResolver):bool:this
          10 ( 1.05% of base) : System.Private.Xml.dasm - ContainerAction:CompileStylesheetAttributes(Compiler):this
           9 ( 0.97% of base) : System.DirectoryServices.AccountManagement.dasm - ADStoreCtx:ScanACLForChangePasswordRight(ActiveDirectorySecurity,byref,byref,byref,byref)
           5 ( 0.74% of base) : System.Private.Xml.dasm - XmlSerializationWriter:GetQualifiedName(String,String):String:this
          19 ( 0.72% of base) : System.Data.OleDb.dasm - OleDbMetaDataFactory:GetDataSourceInformationTable(OleDbConnection,OleDbConnectionInternal):DataTable:this
           6 ( 0.68% of base) : System.Private.CoreLib.dasm - EventSource:AddProviderEnumKind(ManifestBuilder,FieldInfo,String)
          20 ( 0.58% of base) : System.DirectoryServices.AccountManagement.dasm - ADStoreCtx:FindPrincipalByIdentRefHelper(Type,String,String,DateTime,bool):Principal:this
          67 ( 0.58% of base) : System.Management.dasm - ManagementClassGenerator:GenerateProperties():this
          20 ( 0.48% of base) : System.Speech.dasm - XmlParser:ParseGrammar(XmlReader,IGrammar):this
          68 ( 0.46% of base) : System.Net.Http.dasm - <SendAsyncCore>d__60:MoveNext():this
           3 ( 0.39% of base) : Microsoft.CodeAnalysis.dasm - MetadataHelpers:SplitQualifiedName(String):ImmutableArray`1
           8 ( 0.35% of base) : System.Speech.dasm - XmlParser:ParseRule(IGrammar,XmlReader):IRule:this
           2 ( 0.13% of base) : System.Speech.dasm - XmlParser:ParseRuleRef(IElement,XmlReader):IRuleRef:this
           1 ( 0.11% of base) : System.Speech.dasm - XmlParser:ParseItem(IElement,IRule,XmlReader):IItem:this
           1 ( 0.10% of base) : System.Private.Xml.dasm - XmlCustomFormatter:FromDefaultValue(Object,String):String

Top method improvements (percentages):
         -37 (-63.79% of base) : System.Private.CoreLib.dasm - TextInfo:IsInvariantLocale(String):bool
         -46 (-30.67% of base) : FSharp.Core.dasm - LayoutModule:isEmptyL(Layout):bool
         -33 (-23.08% of base) : System.Data.OleDb.dasm - OleDbConnection:CreateCommand():OleDbCommand:this
         -46 (-22.89% of base) : System.ServiceModel.Syndication.dasm - <>c:<TryReadDocumentationFromExtension>b__102_0(SyndicationElementExtension):bool:this
         -46 (-22.89% of base) : System.ServiceModel.Syndication.dasm - <>c:<TryReadSkipDaysFromExtension>b__105_0(SyndicationElementExtension):bool:this
         -46 (-22.89% of base) : System.ServiceModel.Syndication.dasm - <>c:<TryReadSkipHoursFromExtension>b__104_0(SyndicationElementExtension):bool:this
         -46 (-22.89% of base) : System.ServiceModel.Syndication.dasm - <>c:<TryReadTextInputFromExtension>b__107_0(SyndicationElementExtension):bool:this
         -46 (-22.89% of base) : System.ServiceModel.Syndication.dasm - <>c:<TryReadTimeToLiveFromExtension>b__103_0(SyndicationElementExtension):bool:this
         -41 (-16.27% of base) : System.IO.Compression.dasm - ZipArchiveEntry:set_FullName(String):this
         -42 (-15.97% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - EventPipeEventMetaDataHeader:ReadMetadataCommon(PinnedStreamReader):this
         -59 (-14.79% of base) : System.Private.Xml.dasm - QilGenerator:CompileLangAttribute(String,bool):QilNode:this
         -41 (-14.64% of base) : System.ServiceProcess.ServiceController.dasm - ServiceBase:set_ServiceName(String):this
         -65 (-13.49% of base) : System.ServiceModel.Syndication.dasm - SyndicationFeed:ShouldSkipWritingElements(String,String):bool:this
        -239 (-12.13% of base) : System.ServiceModel.Syndication.dasm - Rss20FeedFormatter:ReadMediaEnclosure(XmlReader,Uri):SyndicationLink:this
         -40 (-11.59% of base) : System.Private.CoreLib.dasm - CultureData:nativeEnumTimeFormats(String,int,bool):ref
         -39 (-11.14% of base) : FSharp.Core.dasm - PatternsModule:decodeAssemblyRef(InputState,String):Assembly
         -51 (-10.76% of base) : System.Private.CoreLib.dasm - TextInfo:NlsChangeCase(long,int,long,int,bool):this
         -47 (-10.49% of base) : FSharp.Core.dasm - PatternsModule:u_NamedType(InputState):Type
         -51 (-8.70% of base) : Microsoft.CodeAnalysis.dasm - SymbolDeclaredCompilationEvent:ToString():String:this
         -74 (-8.09% of base) : System.Security.Cryptography.Cng.dasm - <>c:<OpenProcessor>b__15_0(HashAlgorithmName):RsaPaddingProcessor:this

137 total methods with Code Size differences (115 improved, 22 regressed), 280879 unchanged.

All regressions are block layout-related, can be fixed with ? true : false hack but I hope we'll address that in the jit separately (F-sub, convert RETURN(cond) to JTRUE(cond) early, etc).
Jit-diff is way bigger with #44847 (I'll file a better version of it later), see #63734 (comment)

it's ready for review

…me-1 into jit-isknownconstant

EgorBo · 2022-01-14T20:50:21Z

In theory, we could also unroll comparisons this way for small strings, like this:

Unfortunately, inliner early outs for methods larger than 128 byte of IL code - it won't even bother pre-scanning its IR (for better TP) so it won't find IsKnownConstant.
We have an issue somewhere for "partial prescan" - it's when inliner does scan large methods e.g. only first 30 bytes of IL and tries to find motivational observations to keep going e.g. "foldable branch"/

stephentoub · 2022-01-14T20:53:22Z

Unfortunately, inliner early outs for methods larger than 128 byte of IL code

Maybe we need an [AggressivelyConsiderInlinining] :)

EgorBo · 2022-01-19T14:20:58Z

@dotnet/jit-contrib PTAL

EgorBo · 2022-01-19T14:22:52Z

src/coreclr/jit/morph.cpp

            if ((cnsIndex < length) && (str != nullptr))
            {
-                GenTree* cnsCharNode = gtNewIconNode(str[cnsIndex], elemTyp);
+                GenTree* cnsCharNode = gtNewIconNode(str[cnsIndex], TYP_INT);


Unrelated change: "str"[0] used to be folded into a CNS_INT with TYP_USHORT type - no idea how it didn't assert anywhere

src/coreclr/jit/importer.cpp

jakobbotsch

LGTM. Are we worried about the extra IL in these heavily used functions? Do we know of other tools/compilers/interpreters that might get tripped up?

EgorBo · 2022-01-19T15:52:29Z

LGTM. Are we worried about the extra IL in these heavily used functions? Do we know of other tools/compilers/interpreters that might get tripped up?

Good question - the function always return false by default so Mono should be able to fold these branches and be not impacted (both mini and LLVM). Unfortunately, I wasn't able to properly implement this intrinsic for Mono because it doesn't propagate constants during inlining. I'll check Mono-Interp but since it's also already pretty advanced and is able to fold branches before actual interpretation I assume it's also not impacted anyhow by this change.

EgorBo · 2022-01-19T17:34:44Z

internal class Program
{
    static void Main() => Test("");

    [MethodImpl(MethodImplOptions.NoInlining)]
    static bool Test(string s) => s == "";
}

Diffs for Test (via MONO_VERBOSE_METHOD=Test):
Mono-Interpreter: no diffs (https://www.diffchecker.com/TvbZsuxV)
Mono JIT: no diffs (https://www.diffchecker.com/SERaz4TJ)
Mono LLVM: no diffs

EgorBo added 5 commits January 12, 2022 23:06

Don't reuse args in EnC

9f55fa6

Merge branch 'main' of https://github.com/dotnet/runtime into fix-enc…

7d3ef94

…-case

Introduce RuntimeHelpers.IsKnownConstant and use for 'str == ""'

0a717aa

Remove unrelated change, add gtFoldExpr

407a953

Also, optimize String.StartsWith

9d45461

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 13, 2022

ghost assigned EgorBo Jan 13, 2022

Update comment

2f8a034

MihaZupan reviewed Jan 13, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs Outdated Show resolved Hide resolved

Update src/libraries/System.Private.CoreLib/src/System/String.Compari…

4883a46

…son.cs Co-authored-by: Miha Zupan <[email protected]>

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved

stephentoub reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/fgbasic.cpp Outdated Show resolved Hide resolved

EgorBo added 4 commits January 13, 2022 17:09

Delay expansion to morph, replace generic signature with overloads

aa88cf9

Merge branch 'jit-isknownconstant' of https://github.com/EgorBo/runti…

64843f0

…me-1 into jit-isknownconstant

Make string overload nullable

2ebdae9

Use GTF_ALL_EFFECT

e264d27

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

EgorBo added 2 commits January 13, 2022 17:41

Address feedback

f08a182

fix copy-paste

8e5fd0d

jkotas reviewed Jan 13, 2022

View reviewed changes

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/compiler.hpp Outdated Show resolved Hide resolved

SingleAccretion reviewed Jan 13, 2022

View reviewed changes

src/coreclr/jit/compiler.hpp Show resolved Hide resolved

jkotas reviewed Jan 13, 2022

View reviewed changes

EgorBo and others added 2 commits January 13, 2022 18:09

Update src/coreclr/jit/compiler.hpp

e2ebf30

Co-authored-by: SingleAccretion <[email protected]>

Update src/coreclr/jit/compiler.hpp

895abc4

Co-authored-by: SingleAccretion <[email protected]>

jkotas mentioned this pull request Jan 13, 2022

Skip flaky DllImport tests on Mac #63636

Closed

Only boost for constant-arg cases

e0e3e51

jkotas reviewed Jan 13, 2022

View reviewed changes

Remove redundant opts.OptimizationEnabled()s

5b2fbfe

EgorBo added 5 commits January 14, 2022 13:18

Update importer.cpp

30e1855

Update String.Comparison.cs

3cb8402

Update String.Comparison.cs

7c7c264

Add tests

a9b85fc

Merge branch 'jit-isknownconstant' of https://github.com/EgorBo/runti…

a3e5f3c

…me-1 into jit-isknownconstant

Fix unrelated invalid IR, we should not import TYP_USHORT constant

4947947

EgorBo mentioned this pull request Jan 17, 2022

Remove ternary workaround for string.IsNullOrEmpty #63095

Merged

EgorBo commented Jan 19, 2022

View reviewed changes

jakobbotsch reviewed Jan 19, 2022

View reviewed changes

src/coreclr/jit/importer.cpp Show resolved Hide resolved

jakobbotsch reviewed Jan 19, 2022

View reviewed changes

src/coreclr/jit/importer.cpp Show resolved Hide resolved

jakobbotsch approved these changes Jan 19, 2022

View reviewed changes

EgorBo merged commit 65db54e into dotnet:main Jan 19, 2022

JulieLeeMSFT mentioned this pull request Jan 25, 2022

What's new in .NET 7 Preview 1 [WIP] dotnet/core#7106

Closed

EgorBo mentioned this pull request Jan 31, 2022

JIT: Import string.Empty as "" #64530

Merged

EgorBo mentioned this pull request Feb 4, 2022

Move IsKnownConstant handling to ExtendedDefaultPolicy #64809

Merged

ghost locked as resolved and limited conversation to collaborators Feb 19, 2022

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

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

Uh oh!

Conversation

EgorBo commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example:

Current codegen

New codegen:

Uh oh!

ghost commented Jan 13, 2022

Example:

Current codegen

New codegen:

Uh oh!

Uh oh!

EgorBo commented Jan 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EgorBo commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Jan 14, 2022

Uh oh!

EgorBo commented Jan 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jakobbotsch left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Jan 19, 2022

Uh oh!

EgorBo commented Jan 19, 2022

Uh oh!

EgorBo commented Jan 13, 2022 •

edited

Loading

stephentoub Jan 13, 2022 •

edited

Loading

EgorBo commented Jan 13, 2022 •

edited

Loading

EgorBo commented Jan 13, 2022 •

edited

Loading

EgorBo commented Jan 14, 2022 •

edited

Loading