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

emit test for bounds checks against a 0 index #42295

Merged
merged 4 commits into from
Sep 21, 2020

Conversation

nathan-moore
Copy link
Contributor

Since array length is always non-negative, when we have a bounds check against a constant 0 index, we can emit a slightly cheaper/smaller test.

Found 156 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of diff: -3809 (-0.012% of base)
    diff is an improvement.

Top file improvements (bytes):
        -891 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.027% of base)
        -597 : System.Private.CoreLib.dasm (-0.018% of base)
        -254 : System.Private.Xml.dasm (-0.008% of base)
        -121 : System.Data.Common.dasm (-0.011% of base)
        -101 : Microsoft.VisualBasic.Core.dasm (-0.024% of base)
         -77 : System.Net.Http.dasm (-0.013% of base)
         -68 : Microsoft.CodeAnalysis.dasm (-0.009% of base)
         -67 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.003% of base)
         -61 : System.Linq.Expressions.dasm (-0.002% of base)
         -57 : System.Runtime.Numerics.dasm (-0.083% of base)
         -51 : Microsoft.CodeAnalysis.CSharp.dasm (-0.002% of base)
         -51 : System.Private.DataContractSerialization.dasm (-0.007% of base)
         -50 : System.Drawing.Common.dasm (-0.018% of base)
         -44 : Newtonsoft.Json.dasm (-0.007% of base)
         -40 : System.CodeDom.dasm (-0.024% of base)
         -38 : System.Data.Odbc.dasm (-0.021% of base)
         -35 : FSharp.Core.dasm (-0.004% of base)
         -35 : System.Text.Json.dasm (-0.012% of base)
         -34 : System.Net.Primitives.dasm (-0.052% of base)
         -34 : System.ComponentModel.TypeConverter.dasm (-0.015% of base)

156 total files with Code Size differences (156 improved, 0 regressed), 111 unchanged.

Top method improvements (bytes):
         -38 (-1.332% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:HeapSort(Span`1,Span`1,IComparer`1) (11 methods)
         -30 (-1.162% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`2:HeapSort(Span`1,Span`1) (11 methods)
         -22 (-0.285% of base) : System.Private.CoreLib.dasm - Vector128`1:ToString():String:this (11 methods)
         -22 (-0.267% of base) : System.Private.CoreLib.dasm - Vector256`1:ToString():String:this (11 methods)
         -22 (-0.277% of base) : System.Private.CoreLib.dasm - Vector64`1:ToString():String:this (11 methods)
         -15 (-0.169% of base) : System.Private.Xml.dasm - XsltLoader:.ctor():this
         -14 (-0.536% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:HeapSort(Span`1,Comparison`1) (14 methods)
         -14 (-0.491% of base) : System.Net.Http.dasm - KnownHeaders:GetCandidate(StringAccessor):KnownHeader
         -13 (-0.569% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`1:HeapSort(Span`1) (13 methods)
         -12 (-0.259% of base) : System.Data.Common.dasm - FunctionNode:EvalFunction(int,ref,DataRow,int):Object:this
         -11 (-0.197% of base) : System.CodeDom.dasm - VBCodeGenerator:.cctor()
          -8 (-0.726% of base) : CommandLine.dasm - <>c:<get_FormatError>b__11_0(Error):String:this
          -7 (-2.160% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CSharpSyntaxNode:CreateList(IEnumerable`1,bool):GreenNode:this
          -7 (-3.182% of base) : Newtonsoft.Json.Bson.dasm - BsonDataReader:BytesInSequence(ubyte):int:this
          -7 (-3.365% of base) : Newtonsoft.Json.Bson.dasm - BsonDataReader:.cctor()
          -7 (-3.125% of base) : Newtonsoft.Json.dasm - BsonReader:.cctor()
          -7 (-0.421% of base) : System.Data.Common.dasm - SqlDecimal:MpDiv(ReadOnlySpan`1,int,Span`1,int,Span`1,byref,Span`1,byref)
          -6 (-1.575% of base) : System.Private.CoreLib.dasm - Number:FormatExponent(byref,NumberFormatInfo,int,ushort,int,bool)
          -6 (-0.311% of base) : System.Private.CoreLib.dasm - Number:.cctor()
          -6 (-0.286% of base) : System.Private.CoreLib.dasm - CalendarData:CreateInvariant():CalendarData

Top method improvements (percentages):
          -7 (-3.365% of base) : Newtonsoft.Json.Bson.dasm - BsonDataReader:.cctor()
          -7 (-3.182% of base) : Newtonsoft.Json.Bson.dasm - BsonDataReader:BytesInSequence(ubyte):int:this
          -1 (-3.125% of base) : System.Collections.Immutable.dasm - ImmutableArrayExtensions:FirstOrDefault(ImmutableArray`1):__Canon
          -7 (-3.125% of base) : Newtonsoft.Json.dasm - BsonReader:.cctor()
          -1 (-2.703% of base) : Microsoft.VisualBasic.Core.dasm - CharType:FromString(String):ushort
          -1 (-2.703% of base) : Microsoft.VisualBasic.Core.dasm - Conversions:ToChar(String):ushort
          -1 (-2.564% of base) : System.Private.Xml.dasm - Compiler:IsPhantomNamespace(String):bool:this
          -1 (-2.564% of base) : xunit.execution.dotnet.dasm - Pack:UInt16_To_LE(ushort,ref)
          -1 (-2.500% of base) : System.Private.CoreLib.dasm - Statics:ShouldOverrideFieldName(String):bool
          -1 (-2.500% of base) : Microsoft.VisualBasic.Core.dasm - FileSystem:ChDrive(String)
          -1 (-2.500% of base) : System.Private.Xml.dasm - OutputScopeManager:Reset():this
          -1 (-2.500% of base) : xunit.execution.dotnet.dasm - Pack:UInt16_To_BE(ushort,ref)
          -1 (-2.439% of base) : System.Private.Xml.dasm - BigInteger:InitFromDigits(int,int,int):this
          -5 (-2.427% of base) : Microsoft.CodeAnalysis.dasm - UnionCollection`1:Create(ImmutableArray`1,Func`2):ICollection`1
          -1 (-2.381% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CodeGenerator:IsMultidimensionalInitializer(ImmutableArray`1):bool
          -1 (-2.381% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - CodeGenerator:IsMultidimensionalInitializer(ImmutableArray`1):bool:this
          -1 (-2.381% of base) : xunit.execution.dotnet.dasm - Pack:BE_To_UInt16(ref):ushort
          -1 (-2.381% of base) : xunit.execution.dotnet.dasm - Pack:LE_To_UInt16(ref):ushort
          -1 (-2.326% of base) : Newtonsoft.Json.Bson.dasm - StringUtils:StartsWith(String,ushort):bool
          -1 (-2.326% of base) : Newtonsoft.Json.dasm - StringUtils:StartsWith(String,ushort):bool

3127 total methods with Code Size differences (3127 improved, 0 regressed), 188060 unchanged.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 16, 2020
@EgorBo
Copy link
Member

EgorBo commented Sep 16, 2020

Nice!
Can you also remove dead variables in that function:

    GenTree* arrRef    = nullptr;
    int      lenOffset = 0;

@EgorBo
Copy link
Member

EgorBo commented Sep 16, 2020

(oops forgot my approves are "green" in this repo - treat it is a "like" 🙂)

Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM, thank you, do you know if we can do something similar for arm64?

@sandreenko
Copy link
Contributor

cc @dotnet/jit-contrib

@JulieLeeMSFT
Copy link
Member

Thanks @nathan-moore for your contribution.
I am also interested to know if the same can be applied to Arm64.

@EgorBo
Copy link
Member

EgorBo commented Sep 18, 2020

@sandreenko @JulieLeeMSFT

static int Test(int[] arr) => arr[0];

Current codegen:

G_M50965_IG01:
        A9BF7BFD          stp     fp, lr, [sp,#-16]!
        910003FD          mov     fp, sp
                                                ;; bbWeight=1    PerfScore 1.50
G_M50965_IG02:
        B9400801          ldr     w1, [x0,#8]
        7100003F          cmp     w1, #0
        54000089          bls     G_M50965_IG04
        B9401000          ldr     w0, [x0,#16]
                                                ;; bbWeight=1    PerfScore 7.50
G_M50965_IG03:
        A8C17BFD          ldp     fp, lr, [sp],#16
        D65F03C0          ret     lr
                                                ;; bbWeight=1    PerfScore 2.00
G_M50965_IG04:
        97FF2D9A          bl      CORINFO_HELP_RNGCHKFAIL
        D4200000          brk     #0
                                                ;; bbWeight=0    PerfScore 0.00

this

    cmp     w1, #0
    bls     G_M50965_IG04

can be optimized to:

    cbz     w1, G_M50965_IG04  ;; or CBNZ

branch if (not)zero.

@nathan-moore
Copy link
Contributor Author

@sandreenko @EgorBo I looked into what it will take to emit cbz here for arm. There isn't a way to emit it with genJumpToThrowHlpBlk, which is what everything currently uses. You can add a way, but since cbz is internally represented very differently from a normal jump, I don't see a good way of not duplicating the logic, which is what I did here: nathan-moore@8500e53. Does that seem like the right approach?

Also, I checked and there's already a peephole for this in the general case.

@sandreenko
Copy link
Contributor

Thank you @nathan-moore and @EgorBo for the analysis. I think it is valuable to add this optimization for arm64 if it produces similar diffs there. However, it could be done in a separate PR and this one can be merged now.

Thanks @nathan-moore for this optimization.

@sandreenko
Copy link
Contributor

Opened a follow-up for arm64: #42514.

@nathan-moore nathan-moore deleted the setArrAccessTestToZero branch September 21, 2020 22:56
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 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.

5 participants