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

[RyuJIT] static readonly fields can be Invariant#44562

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

Conversation

@EgorBo
Copy link
Member

@EgorBoEgorBo commentedNov 11, 2020
edited
Loading

Closes#38201

It allows to do CSE/Hoist-from-loops optimizations for such indirections and also fixes bounds checks on top ofstatic readonly arrays, e.g.:

publicclassClass1{staticClass1(){}// not necessary, especially with COMPlus_TC_QuickJitForLoops=1 for this casestaticreadonlyint[]s_Array={1,2,3,4};[MethodImpl(MethodImplOptions.NoInlining)]intTest(){intsum=0;for(inti=0;i<s_Array.Length;i++)sum+=s_Array[i];returnsum;}}

Codegen diff:https://www.diffchecker.com/KSiNytCN

JIT-diff (-f --cctors --pmi):

Total bytes of base: 52457617Total bytes of diff: 52437955Total bytes of delta: -19662 (-0.04% of base)    diff is an improvement.Top file regressions (bytes):         279 : Microsoft.CSharp.dasm (0.07% of base)          95 : Microsoft.Extensions.FileSystemGlobbing.dasm (0.33% of base)          71 : System.DirectoryServices.AccountManagement.dasm (0.02% of base)          51 : Microsoft.VisualBasic.Core.dasm (0.01% of base)          42 : System.Runtime.Serialization.Formatters.dasm (0.04% of base)          28 : System.Security.Cryptography.Algorithms.dasm (0.01% of base)          18 : System.CodeDom.dasm (0.01% of base)          14 : System.Security.Cryptography.Cng.dasm (0.01% of base)           5 : Microsoft.Diagnostics.NETCore.Client.dasm (0.05% of base)           2 : Microsoft.Diagnostics.FastSerialization.dasm (0.00% of base)Top file improvements (bytes):      -11214 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.36% of base)       -1345 : System.Private.Xml.dasm (-0.04% of base)        -821 : ILCompiler.Reflection.ReadyToRun.dasm (-0.43% of base)        -570 : System.Private.DataContractSerialization.dasm (-0.08% of base)        -553 : System.Data.Common.dasm (-0.04% of base)        -519 : System.Data.Odbc.dasm (-0.28% of base)        -500 : System.Net.Sockets.dasm (-0.30% of base)        -277 : System.Net.Security.dasm (-0.21% of base)        -247 : System.Data.OleDb.dasm (-0.09% of base)        -245 : System.Diagnostics.FileVersionInfo.dasm (-6.14% of base)        -216 : System.Net.Http.dasm (-0.03% of base)        -211 : FSharp.Core.dasm (-0.01% of base)        -206 : System.Net.HttpListener.dasm (-0.10% of base)        -202 : System.Text.Json.dasm (-0.03% of base)        -185 : System.Collections.Immutable.dasm (-0.01% of base)        -156 : System.Configuration.ConfigurationManager.dasm (-0.05% of base)        -154 : System.Private.CoreLib.dasm (-0.00% of base)        -149 : System.Diagnostics.TraceSource.dasm (-0.37% of base)        -142 : Newtonsoft.Json.dasm (-0.02% of base)        -137 : Microsoft.CodeAnalysis.dasm (-0.01% of base)84 total files with Code Size differences (74 improved, 10 regressed), 187 unchanged.Top method regressions (bytes):         403 (13.05% of base) : Microsoft.CSharp.dasm - ExpressionBinder:GetStandardAndLiftedBinopSignatures(List`1,BinOpArgInfo):bool:this         122 ( 0.77% of base) : System.Text.RegularExpressions.dasm - RegexCompiler:GenerateOneCode():this         118 ( 2.81% of base) : System.Data.Common.dasm - DataTable:SerializeTableSchema(SerializationInfo,StreamingContext,bool):this         103 ( 0.87% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - RegisteredTraceEventParser:GetManifestForRegisteredProvider(Guid):String          95 ( 4.19% of base) : Microsoft.Extensions.FileSystemGlobbing.dasm - PatternBuilder:Build(String):IPattern:this          73 ( 3.80% of base) : System.Private.Xml.dasm - XslAstRewriter:Refactor(XslNode,int):this          70 ( 0.37% of base) : System.Data.Common.dasm - BinaryNode:EvalBinaryOp(int,ExpressionNode,ExpressionNode,DataRow,int,ref):Object:this          51 ( 4.23% of base) : Microsoft.CodeAnalysis.dasm - Parser:ParseNamedTypeSymbol(String,byref,Compilation,ISymbol,List`1)          51 ( 2.23% of base) : System.Private.CoreLib.dasm - TimeZoneInfo:TryCreateAdjustmentRules(String,byref,byref,byref,int):bool          48 ( 1.41% of base) : System.Private.Xml.dasm - XmlAnyConverter:ChangeType(Object,Type,IXmlNamespaceResolver):Object:this          44 ( 7.22% of base) : Microsoft.CodeAnalysis.dasm - Reader:Read(ref,int,int):int:this          44 ( 0.31% of base) : Microsoft.VisualBasic.Core.dasm - VBBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this          44 ( 3.87% of base) : System.Data.OleDb.dasm - OleDbConnectionStringBuilder:get_Keys():ICollection:this          43 ( 2.04% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - MethodCompiler:CreateExplicitInterfaceImplementationStubs(TypeCompilationState,MethodSymbol):this          38 ( 3.30% of base) : System.Runtime.Serialization.Formatters.dasm - ObjectWriter:Write(WriteObjectInfo,NameInfo,NameInfo):this          33 ( 1.95% of base) : System.Net.Sockets.dasm - Socket:Dispose(bool):this          33 ( 0.87% of base) : System.Private.Xml.dasm - XmlSerializationReaderILGen:WriteAttributes(ref,Member,String,LocalBuilder):this          27 ( 1.20% of base) : System.DirectoryServices.AccountManagement.dasm - ADDNLinkedAttrSet:BookmarkAndReset():ResultSetBookmark:this          27 ( 0.73% of base) : System.Private.CoreLib.dasm - DateTimeParse:Lex(int,byref,byref,byref,byref,byref,int):bool          26 ( 4.99% of base) : System.Speech.dasm - TextFragmentEngine:StartProcessUnknownAttributes(Object,byref,String,List`1):thisTop method improvements (bytes):       -5455 (-5.32% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ApplicationServerTraceEventParser:EnumerateTemplates(Func`3,Action`1):this       -2023 (-4.78% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - KernelTraceEventParser:EnumerateTemplates(Func`3,Action`1):this       -1293 (-4.57% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ClrPrivateTraceEventParser:EnumerateTemplates(Func`3,Action`1):this       -1236 (-4.66% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ClrTraceEventParser:EnumerateTemplates(Func`3,Action`1):this        -636 (-4.30% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - AspNetTraceEventParser:EnumerateTemplates(Func`3,Action`1):this        -443 (-7.97% of base) : System.Private.DataContractSerialization.dasm - DataContractCriticalHelper:TryCreateBuiltInDataContract(String,String,byref):bool        -327 (-15.32% of base) : System.Private.Xml.dasm - XmlDocument:.ctor(XmlImplementation):this        -262 (-5.95% of base) : System.Private.Xml.dasm - TypeScope:AddSoapEncodedTypes(String)        -250 (-5.93% of base) : ILCompiler.Reflection.ReadyToRun.dasm - UnwindInfo:ToString():String:this (4 methods)        -245 (-12.23% of base) : System.Diagnostics.FileVersionInfo.dasm - FileVersionInfo:GetVersionInfoForCodePage(long,String):bool:this        -215 (-5.44% of base) : System.Data.Common.dasm - DataTable:DeserializeTableSchema(SerializationInfo,StreamingContext,bool):this        -210 (-4.04% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ClrRundownTraceEventParser:EnumerateTemplates(Func`3,Action`1):this        -202 (-12.68% of base) : System.Text.Json.dasm - JsonSerializerOptions:GetDictionaryKeyConverters():ConcurrentDictionary`2        -159 (-3.95% of base) : ILCompiler.Reflection.ReadyToRun.dasm - GcInfo:ToString():String:this (2 methods)        -132 (-5.51% of base) : ILCompiler.Reflection.ReadyToRun.dasm - InfoHdrSmall:ToString():String:this        -110 (-3.77% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - SymbolTraceEventParser:EnumerateTemplates(Func`3,Action`1):this        -110 (-3.71% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - HeapTraceProviderTraceEventParser:EnumerateTemplates(Func`3,Action`1):this        -109 (-4.57% of base) : System.Private.Xml.dasm - BigNumber:DblToRgbFast(double,ref,byref,byref):bool        -103 (-11.11% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:RightComplex(AvlNode):AvlNode (9 base, 8 diff methods)         -93 (-29.43% of base) : System.Net.Primitives.dasm - CookieTokenizer:TokenFromName(bool):int:thisTop method regressions (percentages):         403 (13.05% of base) : Microsoft.CSharp.dasm - ExpressionBinder:GetStandardAndLiftedBinopSignatures(List`1,BinOpArgInfo):bool:this           8 ( 8.79% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - CRC32:Crc32Update(int,ref):int           4 ( 7.27% of base) : System.Data.Common.dasm - Operators:Priority(int):int          44 ( 7.22% of base) : Microsoft.CodeAnalysis.dasm - Reader:Read(ref,int,int):int:this          25 ( 6.74% of base) : Microsoft.CodeAnalysis.CSharp.dasm - StackOptimizerPass1:VisitArrayInitialization(BoundArrayInitialization):BoundNode:this          16 ( 6.40% of base) : System.ComponentModel.Composition.dasm - GenericServices:GetGenericName(String,ref,int):String          17 ( 5.69% of base) : System.Net.HttpListener.dasm - HttpListenerRequestUriBuilder:AppendOctetsPercentEncoded(StringBuilder,IEnumerable`1)          23 ( 5.02% of base) : System.Drawing.Common.dasm - GPStream:CopyTo(IStream,long,long,long):this          22 ( 5.01% of base) : System.Drawing.Common.dasm - ImageAnimator:UpdateFrames()          26 ( 4.99% of base) : System.Speech.dasm - TextFragmentEngine:StartProcessUnknownAttributes(Object,byref,String,List`1):this           7 ( 4.96% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Candidate:TryGetNamedParamIndex(String,byref):bool:this          12 ( 4.72% of base) : System.Data.Common.dasm - DataRow:get_RowState():int:this           7 ( 4.61% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - ConstructedSymbol:get_TypeArgumentsNoUseSiteDiagnostics():ImmutableArray`1:this           7 ( 4.43% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - CRC32:ComputeCRC32(ref):int          11 ( 4.31% of base) : System.Net.Http.dasm - NameValueHeaderValue:GetNameValueListLength(String,int,ushort,ObjectCollection`1):int          19 ( 4.30% of base) : xunit.runner.utility.netcoreapp10.dasm - StackFrameInfo:FromFailure(IFailureInformation):StackFrameInfo           7 ( 4.27% of base) : Microsoft.VisualBasic.Core.dasm - Symbols:MapToUserDefinedOperator(MethodBase):byte           7 ( 4.27% of base) : System.DirectoryServices.AccountManagement.dasm - ADUtils:EscapeBinaryValue(ref):String           7 ( 4.24% of base) : System.DirectoryServices.AccountManagement.dasm - Utils:ByteArrayToString(ref):String          51 ( 4.23% of base) : Microsoft.CodeAnalysis.dasm - Parser:ParseNamedTypeSymbol(String,byref,Compilation,ISymbol,List`1)Top method improvements (percentages):         -93 (-29.43% of base) : System.Net.Primitives.dasm - CookieTokenizer:TokenFromName(bool):int:this         -93 (-29.43% of base) : System.Net.HttpListener.dasm - CookieTokenizer:TokenFromName(bool):int:this         -24 (-29.27% of base) : System.Private.DataContractSerialization.dasm - ObjectToIdCache:GetPrime(int):int         -20 (-24.69% of base) : System.Net.HttpListener.dasm - HttpListenerResponse:CanSendResponseBody(int):bool         -27 (-22.88% of base) : System.Data.Common.dasm - DataStorage:IsSqlType(Type):bool         -52 (-22.71% of base) : System.Data.Common.dasm - ColumnTypeConverter:ConvertFrom(ITypeDescriptorContext,CultureInfo,Object):Object:this         -15 (-20.55% of base) : System.Private.Xml.dasm - XmlSchemaAttribute:.ctor():this         -25 (-18.94% of base) : System.Private.CoreLib.dasm - OAVariantLib:GetCVTypeFromClass(Type):int         -37 (-18.88% of base) : Microsoft.CodeAnalysis.dasm - WellKnownTypes:AssertEnumAndTableInSync()         -25 (-18.80% of base) : System.Private.Xml.dasm - XmlSchemaElement:.ctor():this         -19 (-18.10% of base) : System.Data.Odbc.dasm - OdbcConnectionStringBuilder:Clear():this         -26 (-17.45% of base) : System.Data.Common.dasm - DataStorage:GetStorageType(Type):int         -19 (-17.12% of base) : System.Data.OleDb.dasm - OleDbConnectionStringBuilder:Clear():this        -327 (-15.32% of base) : System.Private.Xml.dasm - XmlDocument:.ctor(XmlImplementation):this         -51 (-15.22% of base) : System.Data.Common.dasm - ExpressionParser:ScanReserved():this         -65 (-15.15% of base) : xunit.runner.utility.netcoreapp10.dasm - XunitFilters:.ctor():this         -82 (-14.83% of base) : System.Data.Odbc.dasm - DbConnectionOptions:ConvertValueToIntegratedSecurityInternal(String):bool:this         -18 (-14.40% of base) : System.Drawing.Primitives.dasm - KnownColorTable:ArgbToKnownColor(int):Color         -62 (-13.75% of base) : System.Data.Odbc.dasm - DbConnectionOptions:ConvertValueToBooleanInternal(String,String):bool         -62 (-13.75% of base) : System.Data.OleDb.dasm - DbConnectionOptions:ConvertValueToBooleanInternal(String,String):bool912 total methods with Code Size differences (698 improved, 214 regressed), 264931 unchanged.

As far as I can see, the failures are CSE-related, e.g.:

boolTest()=>s_Array.Length==3||s_Array.Length==4;

Codegen diff:https://www.diffchecker.com/INxlsOR7

static readonly fields are marked as invariant only when the host type is already statically inited (we can't change such fields with Reflection viafield.SetValue after that). We even convert such fields into constants for primitive types today.

Tiered JIT helps us here because when we re-compile a method to tier1 the host type is always already inited. The only problem are methods with loops which are always compiled straight to tier1 and it means the compilation most likely happens before the static initialization. As far as I understand, we're going to enableCOMPlus_TC_QuickJitForLoops=1 eventually with OSR so this problem will go away.

Optional:
Mark such fields asGTF_NONFAULTING_IND if we check the content is not null (doesn't really affect the jit-diff report).
Also, the current fix doesn't help withstatic readonly NonPrimitiveValueType = at the moment.

/cc@briansull@sandreenko

@Dotnet-GitSync-BotDotnet-GitSync-Bot added the area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labelNov 11, 2020
@briansull
Copy link
Contributor

I am good with this change.

@AndyAyersMS did you have any concerns here?
I suppose that someone could still change the value using reflection, but I think that we should still be able to optimize this case.

@stephentoub
Copy link
Member

Nice!

@ViktorHofer
Copy link
Member

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script:https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

@AndyAyersMS
Copy link
Member

@EgorBo waiting on you to resolve conflicts.

@JulieLeeMSFT
Copy link
Member

@EgorBo waiting on you to resolve conflicts.

Ping@EgorBo since this PR is inactive for more than 30 days.

@EgorBoEgorBoforce-pushed thejit-static-readonly-field-invariant branch from3ccb654 tob56e447CompareFebruary 18, 2021 09:01
@AndyAyersMS
Copy link
Member

Also just want to point out this doesn't work for shared generic statics, even if we happen to know which exact instantiation we're accessing (because of inlining, say).

EgorBo reacted with thumbs up emoji

@EgorBo
Copy link
MemberAuthor

@AndyAyersMS could you please re-review?

Copy link
Member

@AndyAyersMSAndyAyersMS left a comment

Choose a reason for hiding this comment

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

This should apply more broadly instead of just to the case where the static address can't be RIP-relative.

Also, is

if (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr))

the right check for Arm64?

Can you fix up the "then" case to update tree instead of returning, and then move the new logic you are adding down below the joint point?

@EgorBoEgorBoforce-pushed thejit-static-readonly-field-invariant branch from2772a5d to43360d3CompareFebruary 26, 2021 09:30
@EgorBo
Copy link
MemberAuthor

EgorBo commentedFeb 26, 2021
edited
Loading

This should apply more broadly instead of just to the case where the static address can't be RIP-relative.

Also, is

if (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr))

the right check for Arm64?

Can you fix up the "then" case to update tree instead of returning, and then move the new logic you are adding down below the joint point?

@AndyAyersMS sorry but I don't think I follow, the structure of this method is:

if (objRef) // not static{}else // static{    if (thread-local-static)    {    }    else // normal static    {        if (IMAGE_REL_BASED_REL32 != eeGetRelocTypeHint(fldAddr))        {             // where I mark the field as invariant if the host type is inited            return node; // GT_IND        }        else        {            // assert for being (GTF_FLD_VOLATILE | GTF_FLD_INITCLASS | GTF_COMMON_MASK)            node = GT_CLS_VAR        }        return tree; // always GT_CLS_VAR at this point    }    if (fldOffset  == 0)    {        fgAddFieldSeqForZeroOffset    }    return tree;}

where do you want me to move the "is initialized" logic to?
I couldn't find any initialized static readonly field which isIMAGE_REL_BASED_REL32 and skips the path where I mark them as invariant.

Base automatically changed frommaster tomainMarch 1, 2021 09:07
@AndyAyersMS
Copy link
Member

where do you want me to move the "is initialized" logic to?

As you show in your later commits, there's a divergence in how known-address statics are represented, depending on whether they can be directly addressed via an IP-relative offset or require materialization of the full address constant.

We either need to be able to mark aGT_CLS_VAR as invariant or use theGT_IND form for the invariant cases (as you are doing). Hopefully there's no downside to the latter, but it seems possible there might be.

I don't know why the IR was done this way -- might be something worth revisiting.

@EgorBo
Copy link
MemberAuthor

or use theGT_IND form for the invariant cases

I guess in some cases it might lead to a slightly less efficient codegen (where we don't benefit from being GTF_IND_INVARIANT) but neither jit-diff --cctors nor some of the heave apps I have locally found anything so I decided to not adding more complexity for CLS_VAR.

e.g.static readonly fields of primitive types inside statically inited types never hit this path since they're optimized into constants early. So it's only about value types and objects.

@AndyAyersMS
Copy link
Member

I decided to not adding more complexity for CLS_VAR

Seems reasonable. Can you describe what the impact of using GT_IND for invariant statics is for cases where we can't CSE/Hoist?

You should also look at diffs on x86.

@EgorBo
Copy link
MemberAuthor

EgorBo commentedMar 1, 2021
edited
Loading

I decided to not adding more complexity for CLS_VAR

Seems reasonable. Can you describe what the impact of using GT_IND for invariant statics is for cases where we can't CSE/Hoist?

I only got this delta with that commit:

Total bytes of base: 53052180Total bytes of diff: 53052077Total bytes of delta: -103 (-0.00% of base)    diff is an improvement.Top file improvements (bytes):        -103 : Microsoft.CodeAnalysis.dasm (-0.01% of base)1 total files with Code Size differences (1 improved, 0 regressed), 270 unchanged.Top method improvements (bytes):        -103 (-11.11% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:LeftComplex(AvlNode):AvlNode (9 base, 8 diff methods)Top method improvements (percentages):        -103 (-11.11% of base) : Microsoft.CodeAnalysis.dasm - SmallDictionary`2:LeftComplex(AvlNode):AvlNode (9 base, 8 diff methods)

You should also look at diffs on x86.

@AndyAyersMS X86 always uses theGT_CLS_VAR path, I guess because addresses are twice smaller by default? (the whole block is wrapped with #ifdef TARGET_64BIT`
So the optimization is not kicked in 🤔 - how hard should we try to backporting everything we do for x64 to x86 when it's arch specific?

@AndyAyersMS
Copy link
Member

I only got this delta with that commit:

That may be a spurious diff, given that one case has 8 instances and the other 9. We sometimes see this with PMI.

how hard should we try to backporting everything we do for x64 to x86 when it's arch specific?

Ideally things that are conceptually arch independent would work the same on all architectures. This ends up only being arch specific because of weirdness in the jit IR.

Given that our main perf targets are x64 and arm64 I think it is ok to defer trying to get this working for x86 and arm32. I would open a follow-up issue noting that the split between GT_IND and GT_CLS_VAR is responsible for some missed optimization.

And speaking of Arm64, what happens there? Are we always on the GT_IND path because the type hint always fails?

@EgorBo
Copy link
MemberAuthor

And speaking of Arm64, what happens there? Are we always on the GT_IND path because the type hint always fails?

Yep, as far as I seeeeGetRelocTypeHint always return -1 for Arm64 and any non-windows x64 (an attempt to enable it for *nix 64bit platforms was declineddotnet/coreclr#12831)

so the GT_IND path is always taken there (on all 64bit platforms except Windows)

Copy link
Member

@AndyAyersMSAndyAyersMS left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this.

EgorBo reacted with thumbs up emoji
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

+1 more reviewer

@briansullbriansullbriansull left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@EgorBoEgorBo

Labels

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

Projects

Archived in project

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

RyuJIT: loads for static readonly field should have the same VN

7 participants

@EgorBo@briansull@stephentoub@ViktorHofer@AndyAyersMS@JulieLeeMSFT@Dotnet-GitSync-Bot

[8]ページ先頭

©2009-2025 Movatter.jp