Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Jit: Remove bounds checks with tests against length.#40180

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
briansull merged 20 commits intodotnet:masterfromnathan-moore:morphToUnsigned
Oct 6, 2020

Conversation

@nathan-moore
Copy link
Contributor

@nathan-moorenathan-moore commentedJul 31, 2020
edited
Loading

Look through assertions when trying to get the array length if we don't have a better way.

This removes bounds checks in this form:

if(arr.Length>5){arr[4]=1;arr[i%5]=1;}if(arr.Length!=0){arr[0]=1;}

fixes#11623

Total bytes of diff: -12616 (-0.039% of base)    diff is an improvement.Top file improvements (bytes):       -4356 : System.Private.CoreLib.dasm (-0.134% of base)        -898 : System.Private.Xml.dasm (-0.028% of base)        -582 : Microsoft.VisualBasic.Core.dasm (-0.137% of base)        -500 : System.Data.Common.dasm (-0.045% of base)        -394 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.017% of base)        -381 : Microsoft.CodeAnalysis.CSharp.dasm (-0.018% of base)        -378 : Microsoft.CodeAnalysis.dasm (-0.049% of base)        -321 : FSharp.Core.dasm (-0.034% of base)        -278 : System.Private.DataContractSerialization.dasm (-0.038% of base)        -278 : System.Linq.Expressions.dasm (-0.010% of base)        -249 : Newtonsoft.Json.dasm (-0.040% of base)        -239 : System.Text.Json.dasm (-0.079% of base)        -206 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.006% of base)        -201 : System.Runtime.Numerics.dasm (-0.291% of base)        -199 : System.Reflection.Metadata.dasm (-0.061% of base)        -198 : System.Net.Security.dasm (-0.072% of base)        -162 : System.Net.Http.dasm (-0.027% of base)        -148 : System.ComponentModel.TypeConverter.dasm (-0.066% of base)        -136 : System.ComponentModel.Composition.dasm (-0.063% of base)        -132 : System.Memory.dasm (-0.150% of base)72 total files with Code Size differences (72 improved, 0 regressed), 195 unchanged.Top method regressions (bytes):           8 (0.163% of base) : System.Data.Common.dasm - XSDSchema:LoadSchema(XmlSchemaSet,DataSet):this           2 (0.015% of base) : System.Data.Common.dasm - XmlTreeGen:HandleTable(DataTable,XmlDocument,XmlElement,bool):XmlElement:thisTop method improvements (bytes):        -366 (-23.033% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseDateTimeOffsetO(ReadOnlySpan`1,byref,byref,byref):bool        -308 (-20.995% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseDateTimeOffsetR(ReadOnlySpan`1,int,byref,byref):bool        -295 (-15.784% of base) : System.Private.CoreLib.dasm - DateTimeParse:ParseFormatR(ReadOnlySpan`1,byref,byref):bool        -199 (-24.121% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseDateTimeG(ReadOnlySpan`1,byref,byref,byref):bool        -198 (-2.402% of base) : System.Private.CoreLib.dasm - Vector256`1:ToString():String:this (11 methods)        -198 (-2.492% of base) : System.Private.CoreLib.dasm - Vector64`1:ToString():String:this (11 methods)        -174 (-2.257% of base) : System.Private.CoreLib.dasm - Vector128`1:ToString():String:this (11 methods)         -86 (-23.056% of base) : System.Private.CoreLib.dasm - Rune:TryEncodeToUtf8(Span`1,byref):bool:this         -86 (-9.317% of base) : Microsoft.VisualBasic.Core.dasm - Strings:Format(Object,String):String         -83 (-20.048% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseDateTimeOffsetDefault(ReadOnlySpan`1,byref,byref):bool         -78 (-16.810% of base) : System.Net.Security.dasm - SslStream:DetectFraming(ReadOnlySpan`1):int:this         -70 (-0.489% of base) : Microsoft.VisualBasic.Core.dasm - VBBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this         -67 (-3.851% of base) : System.Memory.dasm - SequenceReader`1:TryReadToAnyInternal(byref,ReadOnlySpan`1,bool,int):bool:this (2 methods)         -66 (-2.891% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`1:HeapSort(Span`1) (13 methods)         -65 (-7.096% of base) : System.Memory.dasm - SequenceReader`1:TryReadToAny(byref,ReadOnlySpan`1,bool):bool:this (4 methods)         -60 (-1.823% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - TraceEvent:PayloadString(int,IFormatProvider):String:this         -59 (-7.403% of base) : System.Net.HttpListener.dasm - HttpListenerRequest:SetRequestLine(String):this         -55 (-5.765% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:SetAllDateTimePatterns(ref,ushort):this         -55 (-18.395% of base) : System.Net.Security.dasm - SslStream:GetFrameSize(ReadOnlySpan`1):int:this         -52 (-17.162% of base) : Microsoft.CodeAnalysis.dasm - PathUtilities:GetPathKind(String):intTop method regressions (percentages):           8 (0.163% of base) : System.Data.Common.dasm - XSDSchema:LoadSchema(XmlSchemaSet,DataSet):this           2 (0.015% of base) : System.Data.Common.dasm - XmlTreeGen:HandleTable(DataTable,XmlDocument,XmlElement,bool):XmlElement:thisTop method improvements (percentages):         -17 (-53.125% of base) : System.Collections.Immutable.dasm - ImmutableArrayExtensions:FirstOrDefault(ImmutableArray`1):__Canon         -17 (-43.590% of base) : System.Private.Xml.dasm - Compiler:IsPhantomNamespace(String):bool:this         -28 (-41.791% of base) : Microsoft.CodeAnalysis.CSharp.dasm - ExplicitInterfaceMethodTypeParameterMap:GetOverriddenMethod(SourceMemberMethodSymbol):MethodSymbol:this         -17 (-40.476% of base) : Microsoft.CodeAnalysis.CSharp.dasm - CodeGenerator:IsMultidimensionalInitializer(ImmutableArray`1):bool         -17 (-40.476% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - CodeGenerator:IsMultidimensionalInitializer(ImmutableArray`1):bool:this         -17 (-39.535% of base) : Newtonsoft.Json.Bson.dasm - StringUtils:StartsWith(String,ushort):bool         -17 (-39.535% of base) : Newtonsoft.Json.dasm - StringUtils:StartsWith(String,ushort):bool         -17 (-39.535% of base) : System.Private.Xml.dasm - Compiler:IsPhantomName(QilName):bool:this         -17 (-33.333% of base) : System.Data.Common.dasm - XmlDataDocument:GetNestedParentRelation(DataRow):DataRelation         -27 (-32.927% of base) : System.Collections.Immutable.dasm - ImmutableArrayExtensions:First(ImmutableArray`1):__Canon         -21 (-30.882% of base) : System.Private.Xml.dasm - CSharpHelpers:IsPrefixTwoUnderscore(String):bool         -21 (-30.882% of base) : System.CodeDom.dasm - CSharpHelpers:IsPrefixTwoUnderscore(String):bool         -11 (-29.730% of base) : Microsoft.VisualBasic.Core.dasm - CharType:FromString(String):ushort         -11 (-29.730% of base) : Microsoft.VisualBasic.Core.dasm - Conversions:ToChar(String):ushort         -16 (-29.630% of base) : Microsoft.CodeAnalysis.dasm - PathUtilities:IsDriveRootedAbsolutePath(String):bool         -11 (-27.500% of base) : Microsoft.VisualBasic.Core.dasm - FileSystem:ChDrive(String)         -11 (-25.000% of base) : System.Drawing.Common.dasm - Font:IsVerticalName(String):bool         -26 (-24.762% of base) : System.Private.Xml.dasm - XmlAutoDetectWriter:IsHtmlTag(String):bool         -11 (-24.444% of base) : System.Private.DataContractSerialization.dasm - XmlUTF8NodeWriter:WritePrefix(String):this         -21 (-24.419% of base) : System.Private.Xml.dasm - ValidateNames:StartsWithXml(String):bool672 total methods with Code Size differences (670 improved, 2 regressed), 190515 unchanged.

EgorBo, erozenfeld, sandreenko, am11, samsosa, and itsamelambda reacted with thumbs up emoji
@Dotnet-GitSync-BotDotnet-GitSync-Bot added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labelJul 31, 2020
@sandreenkosandreenko added this to the6.0.0 milestoneJul 31, 2020
@sandreenko
Copy link
Contributor

Hi@nathan-moore, thanks for the change.

@sandreenko Are JIT improvements still being accepted or should I shelve this for the time being?

The master branch is currently blocked for feature changes but it won't be long (~few weeks) until we open in for post-5.0 PRs, so you could keep it as an open PR, I have marked it as 6.0 so we don't mix it with 5.0 fixes.

@nathan-moorenathan-moore changed the titleJit: morph some comparisons to unsigned to remove some bound checks(wip) Jit:remove some bounds checksAug 13, 2020
@sandreenkosandreenko marked this pull request as draftAugust 13, 2020 17:37
@nathan-moorenathan-moore changed the title(wip) Jit:remove some bounds checksJit: Remove bounds checks with tests against length.Aug 27, 2020
@nathan-moorenathan-moore marked this pull request as ready for reviewAugust 27, 2020 23:10
@nathan-moore
Copy link
ContributorAuthor

@sandreenko PTAL

@sandreenko
Copy link
Contributor

PTAL @dotnet/jit-contrib

Copy link
Contributor

@sandreenkosandreenko left a comment

Choose a reason for hiding this comment

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

The change makes sense, thanks@nathan-moore , but I do not have enough expertise in this area to approve,@briansull could you please take a look?

I have left a few requests for comments for unclear parts, also expect outerloop/stress/jitstress testing for this change before the merge.

@briansull
Copy link
Contributor

Taking a look at this

Copy link
Contributor

@briansullbriansull left a comment

Choose a reason for hiding this comment

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

I would like to see some small updates then I will approve.

limit =Limit(Limit::keConstant, info.constVal);
cmpOper = (genTreeOps)info.cmpOper;
}
// Current assertion is of the form i == 100
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should read:

// Is the current assertion a valid constant int32 assertion?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is the pre-existing style for this else if chain. I can change them over, but I personally like having the form there.

Copy link
Contributor

@briansullbriansull left a comment

Choose a reason for hiding this comment

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

Looks Good

@briansullbriansull merged commit30e643e intodotnet:masterOct 6, 2020
@nathan-moorenathan-moore deleted the morphToUnsigned branchOctober 6, 2020 20:54
@am11
Copy link
Member

am11 commentedOct 7, 2020

fixes#11623

@briansull,@nathan-moore fyi: his keyword has closed the issue when PR was merged but the original repro in the issue persists (bound check elides withlength > 4 but not withlength >= 5):#11623 (comment). Should we reopen the issue?

@danmoseley
Copy link
Member

danmoseley commentedOct 16, 2020
edited
Loading

@briansull I happened to notice the question above wasn't answered.

@briansull
Copy link
Contributor

We closed the issue#11623 when PR was merged
but the original repro in the issue persists (bound check elides with length > 4 but not with length >= 5):

I will reopen it.

@ghostghost locked asresolvedand limited conversation to collaboratorsDec 8, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

2 more reviewers

@sandreenkosandreenkosandreenko left review comments

@briansullbriansullbriansull approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@nathan-moorenathan-moore

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

JIT doesn't eliminate bounds checks sometimes

6 participants

@nathan-moore@sandreenko@briansull@am11@danmoseley@Dotnet-GitSync-Bot

[8]ページ先頭

©2009-2025 Movatter.jp