Skip to content

Fold bound checks for static readonly arrays/strings - #77593

Merged
EgorBo merged 44 commits into
dotnet:mainfrom
EgorBo:fold-static-readonly-array-length
Nov 1, 2022
Merged

Fold bound checks for static readonly arrays/strings#77593
EgorBo merged 44 commits into
dotnet:mainfrom
EgorBo:fold-static-readonly-array-length

Conversation

@EgorBo

@EgorBoEgorBo commented Oct 28, 2022

Copy link
Copy Markdown
Member

Closes#76578
Closes#42087

This PR folds obj.Length into a constant for static readonly initialized arrays/strings - as a side effect - it removes bound checks. Example:

staticreadonlyint[]MyArr={1,2,3,4,5};staticreadonlystringMyStr=(42).ToString();intTest1()=>MyArr[3];intTest2()=>MyStr.Length;

Codegen diff:

; Assembly listing for method Program:Test1():int:this
- sub rsp, 40
mov rax, 0xD1FFAB1E
mov rax, gword ptr [rax]
- cmp dword ptr [rax+08H], 3- jbe SHORT G_M18247_IG04
mov eax, dword ptr [rax+1CH]
- add rsp, 40
ret -G_M18247_IG04:- call CORINFO_HELP_RNGCHKFAIL- int3 -; Total bytes of code 37+; Total bytes of code 17
; Assembly listing for method Program:Test2():int:this
- mov rax, 0xD1FFAB1E- mov rax, gword ptr [rax]- mov eax, dword ptr [rax+08H]+ mov eax, 2 ;; folded to just 2
ret -; Total bytes of code 17+; Total bytes of code 6

Jit-diffs (-f -pmi --cctors):

Total bytes of base: 63327742
Total bytes of diff: 63313731
Total bytes of delta: -14011 (-0.02 % of base)
Total relative delta: -35.61
diff is an improvement.
relative diff is an improvement.
Total byte diff includes -206 bytes from reconciling methods
Base had 2 unique methods, 206 unique bytes
Diff had 0 unique methods, 0 unique bytes
Top file regressions (bytes):
21 : System.Resources.Extensions.dasm (0.06% of base)
16 : System.Security.Cryptography.Pkcs.dasm (0.00% of base)
7 : System.IO.Hashing.dasm (0.03% of base)
2 : System.IO.Compression.dasm (0.00% of base)
2 : System.ComponentModel.Composition.Registration.dasm (0.00% of base)
2 : Microsoft.Extensions.FileProviders.Physical.dasm (0.01% of base)
2 : Microsoft.Win32.Registry.dasm (0.01% of base)
Top file improvements (bytes):
-5357 : System.Private.Xml.dasm (-0.13% of base)
-1634 : System.Private.DataContractSerialization.dasm (-0.18% of base)
-1160 : System.Private.CoreLib.dasm (-0.02% of base)
-872 : System.ComponentModel.TypeConverter.dasm (-0.30% of base)
-536 : Microsoft.CSharp.dasm (-0.13% of base)
-429 : Microsoft.CodeAnalysis.dasm (-0.02% of base)
-336 : System.Net.Mail.dasm (-0.19% of base)
-305 : System.Linq.Expressions.dasm (-0.04% of base)
-277 : System.Net.Http.dasm (-0.03% of base)
-266 : System.Data.Common.dasm (-0.02% of base)
-254 : System.ComponentModel.Composition.dasm (-0.07% of base)
-213 : System.Diagnostics.DiagnosticSource.dasm (-0.11% of base)
-191 : System.Text.Json.dasm (-0.02% of base)
-191 : System.Configuration.ConfigurationManager.dasm (-0.05% of base)
-146 : Newtonsoft.Json.dasm (-0.02% of base)
62 total files with Code Size differences (55 improved, 7 regressed), 212 unchanged.
Top method regressions (bytes):
39 (12.00% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback,System.Object,uint,uint):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[double]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[int]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[long]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[short]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[System.Nullable`1[int]]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[System.Numerics.Vector`1[float]]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[ubyte]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
32 ( 3.82% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[System.__Canon]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
32 ( 5.88% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback,System.Object,int,int):this
32 ( 4.22% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback,System.Object,long,long):this
32 ( 3.67% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback,System.Object,System.TimeSpan,System.TimeSpan):this
30 (10.49% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback):this
30 (13.57% of base) : System.Private.CoreLib.dasm - System.Threading.TimerQueueTimer:.ctor(System.Threading.TimerCallback,System.Object,uint,uint,bool):this
26 ( 6.91% of base) : System.Private.CoreLib.dasm - DelayPromise:.ctor(uint):this
Top method improvements (bytes):
-358 (-38.96% of base) : System.Private.CoreLib.dasm - System.Text.EncodingTable:GetEncodings(System.Collections.Generic.Dictionary`2[int,System.Text.EncodingInfo]):System.Text.EncodingInfo[]
-324 (-45.70% of base) : System.Private.CoreLib.dasm - System.Text.EncodingTable:GetEncodings():System.Text.EncodingInfo[]
-215 (-8.61% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationReaderILGen:WriteArray(System.String,System.String,System.Xml.Serialization.ArrayMapping,bool,bool,int,int):this
-213 (-5.32% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationWriterILGen:WriteEnumAndArrayTypes():this
-204 (-6.00% of base) : System.Private.Xml.dasm - System.Xml.Schema.XmlAnyConverter:ChangeType(System.Object,System.Type,System.Xml.IXmlNamespaceResolver):System.Object:this
-170 (-12.72% of base) : System.Private.CoreLib.dasm - System.Convert:ChangeType(System.Object,System.Type,System.IFormatProvider):System.Object
-167 (-6.38% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationReaderILGen:WriteEnumAndArrayTypes():this
-166 (-11.44% of base) : System.Private.CoreLib.dasm - System.Convert:DefaultToType(System.IConvertible,System.Type,System.IFormatProvider):System.Object
-165 (-3.06% of base) : System.Private.DataContractSerialization.dasm - CriticalHelper:ReadCollection(System.Runtime.Serialization.DataContracts.CollectionDataContract):this (2 methods)
-160 (-9.16% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationReaderILGen:WriteText(Member):this
-155 (-18.32% of base) : System.Private.CoreLib.dasm - System.Reflection.Emit.TypeBuilder:DefineDefaultConstructorNoLock(int):System.Reflection.Emit.ConstructorBuilder:this
-140 (-23.53% of base) : System.Private.DataContractSerialization.dasm - System.Xml.XmlUTF8NodeWriter:WriteDeclaration():this
-133 (-13.96% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationReaderILGen:WriteXmlNodeEqual(System.String,System.String,System.String,bool):this
-123 (-9.81% of base) : System.Data.Common.dasm - System.Data.Common.ObjectStorage:GetXmlSerializer(System.Type,System.Xml.Serialization.XmlRootAttribute):System.Xml.Serialization.XmlSerializer
-121 (-11.33% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializationILGen:GenerateBaseSerializer(System.String,System.String,System.String,System.Xml.Serialization.CodeIdentifiers):System.String:this
Top method regressions (percentages):
30 (13.57% of base) : System.Private.CoreLib.dasm - System.Threading.TimerQueueTimer:.ctor(System.Threading.TimerCallback,System.Object,uint,uint,bool):this
39 (12.00% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback,System.Object,uint,uint):this
30 (10.49% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback):this
3 ( 7.14% of base) : System.Drawing.Primitives.dasm - System.Drawing.KnownColorNames:KnownColorToName(int):System.String
3 ( 6.98% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Microsoft.CodeAnalysis.VisualBasic.SyntaxFacts:IsNarrowIdentifierCharacter(ushort):bool
3 ( 6.98% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.WellKnownTypes:GetMetadataName(int):System.String
26 ( 6.91% of base) : System.Private.CoreLib.dasm - DelayPromise:.ctor(uint):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[double]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[int]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[long]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[short]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[System.Nullable`1[int]]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[System.Numerics.Vector`1[float]]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
38 ( 6.02% of base) : System.Private.CoreLib.dasm - CancellationPromise`1[ubyte]:.ctor(System.Threading.Tasks.Task,uint,System.Threading.CancellationToken):this
32 ( 5.88% of base) : System.Private.CoreLib.dasm - System.Threading.Timer:.ctor(System.Threading.TimerCallback,System.Object,int,int):this
Top method improvements (percentages):
-103 (-100.00% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.SmallDictionary`2[System.__Canon,int]:LeftComplex(AvlNode[System.__Canon,int]):AvlNode[System.__Canon,int] (1 base, 0 diff methods)
-103 (-100.00% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.SmallDictionary`2[System.__Canon,int]:RightComplex(AvlNode[System.__Canon,int]):AvlNode[System.__Canon,int] (1 base, 0 diff methods)
-11 (-64.71% of base) : System.Net.Http.dasm - System.Net.Http.HPack.H2StaticTable:get_Count():int
-20 (-52.63% of base) : System.Private.CoreLib.dasm - NullStreamReader:ReadAsync(ushort[],int,int):System.Threading.Tasks.Task`1[int]:this
-20 (-52.63% of base) : System.Private.CoreLib.dasm - NullStreamReader:ReadBlockAsync(ushort[],int,int):System.Threading.Tasks.Task`1[int]:this
-324 (-45.70% of base) : System.Private.CoreLib.dasm - System.Text.EncodingTable:GetEncodings():System.Text.EncodingInfo[]
-74 (-44.58% of base) : System.Private.Xml.dasm - System.Xml.Serialization.ReflectionXmlSerializationReader:GetDefaultConstructor(System.Type):System.Reflection.ConstructorInfo
-23 (-41.82% of base) : System.Net.Http.dasm - System.Net.Http.HttpRuleParser:IsTokenChar(ushort):bool
-89 (-41.20% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.TypeDescriptor:GetAttributes(System.Type):System.ComponentModel.AttributeCollection
-70 (-40.94% of base) : System.Private.DataContractSerialization.dasm - System.Runtime.Serialization.XmlFormatGeneratorStatics:get_HashtableCtor():System.Reflection.ConstructorInfo
-19 (-39.58% of base) : System.Diagnostics.DiagnosticSource.dasm - System.Diagnostics.Metrics.Instrument`1[System.Numerics.Vector`1[float]]:RecordMeasurement(System.Numerics.Vector`1[float]):this
-358 (-38.96% of base) : System.Private.CoreLib.dasm - System.Text.EncodingTable:GetEncodings(System.Collections.Generic.Dictionary`2[int,System.Text.EncodingInfo]):System.Text.EncodingInfo[]
-113 (-37.79% of base) : System.Private.Xml.dasm - System.Xml.Serialization.XmlSerializer:.ctor(System.Type,System.Xml.Serialization.XmlRootAttribute):this
-89 (-37.55% of base) : Newtonsoft.Json.Bson.dasm - Newtonsoft.Json.Bson.BsonDataReader:BytesInSequence(ubyte):int:this
-89 (-37.55% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Bson.BsonReader:BytesInSequence(ubyte):int:this
607 total methods with Code Size differences (428 improved, 179 regressed), 391602 unchanged.

Size regressions are actually improvements, e.g.:
https://www.diffchecker.com/rJTFoUpk (mov reg, imm vs mov reg, [reg])
https://www.diffchecker.com/Rt1IPpnb (idiv is expanded to mul + magic constants)

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 28, 2022
@ghostghost assigned EgorBoOct 28, 2022
@ghost

Copy link
Copy Markdown

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

Issue Details

Closes #76578

This PR fold obj.Length into a constant for static readonly initialized arrays/strings - it removes bound checks. Example:

staticreadonlyint[]MyArr={1,2,3,4,5};staticreadonlystringMyStr=(42).ToString();intTest1()=>MyArr[3];intTest2()=>MyStr.Length;

Old codegen:

; Assembly listing for method Program:Test1():int:thissubrsp,40movrax,0xD1FFAB1Emovrax, gword ptr [rax]cmp dword ptr [rax+08H],3jbe SHORT G_M18247_IG04moveax, dword ptr [rax+1CH]addrsp,40retG_M18247_IG04:call CORINFO_HELP_RNGCHKFAILint3; Total bytes of code 37; Assembly listing for method Program:Test2():int:thismovrax,0xD1FFAB1Emovrax, gword ptr [rax]moveax, dword ptr [rax+08H]ret; Total bytes of code 17

New codegen:

; Assembly listing for method Program:Test1():int:thismovrax,0xD1FFAB1Emovrax, gword ptr [rax]moveax, dword ptr [rax+1CH]ret; Total bytes of code 17; Assembly listing for method Program:Test2():int:thismoveax,2ret; Total bytes of code 6
Author:EgorBo
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@hez2010

hez2010 commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

Would like to see the codegen of method Test1 being optimized to just

mov eax, 4
ret

;)

@EgorBo

Copy link
Copy Markdown
MemberAuthor

Would like to see the codegen of method Test1 being optimized to just

mov eax, 4
ret

;)

Unfortunately it's not legal, array is mutable 😞

@EgorBo
EgorBoforce-pushed the fold-static-readonly-array-length branch from da5a062 to f207378CompareOctober 28, 2022 18:31
@EgorBo
EgorBo marked this pull request as ready for review October 28, 2022 18:32
@EgorBo

Copy link
Copy Markdown
MemberAuthor

@dotnet/jit-contrib @jakobbotsch PTAL
(and @jkotas for vm side)

Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/tools/Common/JitInterface/CorInfoImpl.cs Outdated
@EgorBo
EgorBoforce-pushed the fold-static-readonly-array-length branch from f207378 to 0fe7042CompareOctober 29, 2022 01:28
Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/inc/corinfo.h
Comment threadsrc/coreclr/jit/importer.cpp Outdated
Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/vm/appdomain.hpp Outdated
Comment threadsrc/coreclr/jit/valuenum.cpp Outdated

@jakobbotschjakobbotsch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. The encoding of field sequence in VNF_InvariantNonNullLoad/VNF_InvariantLoad still feels a bit unfortunate to me, but seems ok to be pragmatic about it for now, we can always try to clean this up later. @SingleAccretion do you have an opinion on this?

@SingleAccretion

Copy link
Copy Markdown
Contributor

It does look suboptimal; how much more code would using VNF_PtrToStatic imply?

Comment threadsrc/coreclr/jit/hwintrinsicarm64.cpp
Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
@EgorBo

EgorBo commented Oct 31, 2022

Copy link
Copy Markdown
MemberAuthor

It does look suboptimal; how much more code would using VNF_PtrToStatic imply?

@jakobbotsch@SingleAccretion just for my understanding - what exactly we try to avoid by using that?
Do you mean to use VNF_PtrToStatic encoded into VNF_InvariantLoad? so the unpack will be like

if (GetVNFunc())
{
if (GetVNFunc()..
}

?

@SingleAccretion

Copy link
Copy Markdown
Contributor

Do you mean to use VNF_PtrToStatic encoded into VNF_InvariantLoad?

Yes.

Q: what's the advantage:
A:
- Not using two args for invariant loads where most of them don't use the second argument.
- Naturally extends to more complex load patterns (e. g. readonlyStruct.Field).

@SingleAccretion

SingleAccretion commented Oct 31, 2022

Copy link
Copy Markdown
Contributor

Although, actually, thinking a bit more about it, I realize this is not currently possible due to how the addresses are constant handles (and "renumbering" them in invariant load numbering defeats the purpose).

So my actual suggestion is to encode the addresses for invariant loads as field sequence VNs (GTF_ICON_FIELD_SEQ handles) (given the offset of the implicit struct fields is zero).

Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
@AndyAyersMS

Copy link
Copy Markdown
Member

Couldn't we do this in early prop? Or do we somehow get a lot more cases running it later?

Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
@EgorBo

Copy link
Copy Markdown
MemberAuthor

Couldn't we do this in early prop? Or do we somehow get a lot more cases running it later?

Do you mind if I do it separately, once we get SPMI collection for this and it will be easier to determinate if it's profitable or not to do it in the early prop. This OPT handles ARR_LEN(tmplcl) where tmplcl is either a const frozen handle or IND to a static field

EgorBoand others added 2 commits October 31, 2022 21:31
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
Comment threadsrc/coreclr/jit/valuenum.cpp Outdated
@AndyAyersMS

Copy link
Copy Markdown
Member

Couldn't we do this in early prop? Or do we somehow get a lot more cases running it later?

Do you mind if I do it separately, once we get SPMI collection for this and it will be easier to determinate if it's profitable or not to do it in the early prop. This OPT handles ARR_LEN(tmplcl) where tmplcl is either a const frozen handle or IND to a static field

Fine by me. I was mostly curious why you chose VN over early prop, since early prop already does similar things, and the transformation is probably simpler there.

Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
@EgorBo
EgorBo merged commit bd175da into dotnet:mainNov 1, 2022
@EgorBo
EgorBo deleted the fold-static-readonly-array-length branch November 1, 2022 12:54
@EgorBo

Copy link
Copy Markdown
MemberAuthor

improvements on arm64 dotnet/perf-autofiling-issues#9676

@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Bound checks for static readonly arrays RyuJIT: Extend "cns".Length optimization to support static readonly fields

6 participants

@EgorBo@hez2010@jkotas@SingleAccretion@AndyAyersMS@jakobbotsch