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.

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

Fold constant string null checks #1377

Closed
wants to merge 1 commit into from
Closed

Conversation

mikedn
Copy link
Contributor

@mikedn mikedn commented Jan 7, 2020

GT_CNS_STR can't ever produce a null value so we can constant fold "str" != null and similar expressions early, before GT_CNS_STR gets expanded into an IND(GT_CNS_INT) which is more problematic to recognize as non null.

Fixes #1368

@mikedn
Copy link
Contributor Author

mikedn commented Jan 7, 2020

Example from #1368
Before:

G_M26703_IG01:
       56                   push     rsi
       4883EC20             sub      rsp, 32
                                                ;; bbWeight=1    PerfScore 1.25
G_M26703_IG02:
       48B898314E43CE010000 mov      rax, 0x1CE434E3198
       48833800             cmp      gword ptr [rax], 0
       740E                 je       SHORT G_M26703_IG06
                                                ;; bbWeight=1    PerfScore 3.25
G_M26703_IG03:
       4885C9               test     rcx, rcx
       7442                 je       SHORT G_M26703_IG07
                                                ;; bbWeight=0.50 PerfScore 0.63
G_M26703_IG04:
       488BC1               mov      rax, rcx
                                                ;; bbWeight=1    PerfScore 0.25
G_M26703_IG05:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret
                                                ;; bbWeight=1    PerfScore 1.75
G_M26703_IG06:
       48B9C8B9D1A3FA7F0000 mov      rcx, 0x7FFAA3D1B9C8
       E81E178B5F           call     CORINFO_HELP_NEWSFAST

After:

G_M26703_IG01:
       56                   push     rsi
       4883EC20             sub      rsp, 32
                                                ;; bbWeight=1    PerfScore 1.25
G_M26703_IG02:
       4885C9               test     rcx, rcx
       7409                 je       SHORT G_M26703_IG05
                                                ;; bbWeight=1    PerfScore 1.25
G_M26703_IG03:
       488BC1               mov      rax, rcx
                                                ;; bbWeight=1    PerfScore 0.25
G_M26703_IG04:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret
                                                ;; bbWeight=1    PerfScore 1.75
G_M26703_IG05:
       48B9C8B9D2A3FA7F0000 mov      rcx, 0x7FFAA3D2B9C8
       E82E178A5F           call     CORINFO_HELP_NEWSFAST

@jkotas jkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 7, 2020
@mikedn
Copy link
Contributor Author

mikedn commented Jan 7, 2020

Diff summary:

Total bytes of diff: -4786 (-0.02% of base)
    diff is an improvement.
Top file improvements by size (bytes):
       -2357 : Newtonsoft.Json.dasm (-0.38% of base)
       -1622 : System.Private.CoreLib.dasm (-0.05% of base)
        -594 : System.Private.Xml.dasm (-0.02% of base)
         -68 : System.Reflection.Metadata.dasm (-0.02% of base)
         -51 : System.Data.Common.dasm (-0.00% of base)
8 total files with size differences (8 improved, 0 regressed), 101 unchanged.
Top method regressions by size (bytes):
          52 ( 1.03% of base) : System.Private.CoreLib.dasm - ManifestBuilder:CreateManifestString():String:this
           1 ( 0.15% of base) : System.Private.CoreLib.dasm - MethodBase:AppendParameters(byref,ref,int)
Top method improvements by size (bytes):
        -290 (-28.66% of base) : System.Private.CoreLib.dasm - LicenseInteropProxy:.ctor():this
        -240 (-15.13% of base) : Newtonsoft.Json.dasm - JsonSerializerInternalReader:ReadMetadataPropertiesToken(JTokenReader,byref,byref,JsonProperty,JsonContainerContract,JsonProperty,Object,byref,byref):bool:this
        -204 (-10.03% of base) : Newtonsoft.Json.dasm - JsonSerializerInternalReader:ResolvePropertyAndCreatorValues(JsonObjectContract,JsonProperty,JsonReader,Type):List`1:this
        -160 (-5.91% of base) : Newtonsoft.Json.dasm - JsonTextReader:ReadStringIntoBuffer(ushort):this
        -158 (-2.25% of base) : Newtonsoft.Json.dasm - <ExecuteFilter>d__4:MoveNext():bool:this (8 methods)
Top method regressions by size (percentage):
          52 ( 1.03% of base) : System.Private.CoreLib.dasm - ManifestBuilder:CreateManifestString():String:this
           1 ( 0.15% of base) : System.Private.CoreLib.dasm - MethodBase:AppendParameters(byref,ref,int)
Top method improvements by size (percentage):
        -290 (-28.66% of base) : System.Private.CoreLib.dasm - LicenseInteropProxy:.ctor():this
         -45 (-26.16% of base) : System.Private.DataContractSerialization.dasm - JsonReaderWriterFactory:CreateJsonWriter(Stream,Encoding,bool,bool):XmlDictionaryWriter
         -68 (-22.59% of base) : System.Reflection.Metadata.dasm - ManagedPEBuilder:CreateSections():ImmutableArray`1:this
         -49 (-20.16% of base) : System.Private.CoreLib.dasm - SynchronizationContext:GetWinRTSynchronizationContext(Object):SynchronizationContext
         -96 (-18.05% of base) : System.Private.CoreLib.dasm - AppDomain:GetThreadPrincipal():IPrincipal:this
106 total methods with size differences (104 improved, 2 regressed), 146499 unchanged.

The CreateManifestString regression is due to additional CSEs. Same for AppendParameters

@EgorBo
Copy link
Member

EgorBo commented Jan 8, 2020

Heh, this PR along with #1378 should eliminate string.IsNullOrEmpty(...) completely for constant strings (it's value == null || 0u >= (uint)value.Length)

@mikedn mikedn closed this Jan 8, 2020
@EgorBo
Copy link
Member

EgorBo commented Apr 14, 2020

@mikedn May I ask why you closed this?
It looks quite useful for Spans of chars, e.g.:

ReadOnlySpan<char> span = "hello";

currently produces a redundant nullcheck over ldstr.

@drieseng
Copy link
Contributor

@EgorBo Mike decided to stop contributing. He maintains a fork where you can surely pick up great changes.

@EgorBo
Copy link
Member

EgorBo commented Apr 18, 2020

@EgorBo Mike decided to stop contributing. He maintains a fork where you can surely pick up great changes.

Ah, I see, that's sad, I learned a lot from his PRs and feedback 😢

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliding inlined null checks against string constants at JIT time
4 participants