[RyuJIT] static readonly fields can be Invariant

[EgorBo]

Closes #38201

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

public class Class1
{
    static Class1() {} // not necessary, especially with COMPlus_TC_QuickJitForLoops=1 for this case

    static readonly int[] s_Array = { 1, 2, 3, 4 };

    [MethodImpl(MethodImplOptions.NoInlining)]
    int Test()
    {
        int sum = 0;
        for (int i = 0; i < s_Array.Length; i++)
            sum += s_Array[i];
        return sum;
    }
}

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

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

Total bytes of base: 52457617
Total bytes of diff: 52437955
Total 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):this

Top 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:this

Top 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):bool

912 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.:

bool Test() => 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 via field.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 enable COMPlus_TC_QuickJitForLoops=1 eventually with OSR so this problem will go away.

Optional:
Mark such fields as GTF_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 with static readonly NonPrimitiveValueType = at the moment.

/cc @briansull @sandreenko

[briansull]

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]

Nice!

[ViktorHofer]

// 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]

@EgorBo waiting on you to resolve conflicts.

[JulieLeeMSFT]

@EgorBo waiting on you to resolve conflicts.

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

[AndyAyersMS]

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]

@AndyAyersMS could you please re-review?

[AndyAyersMS]

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?

[EgorBo]

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 is IMAGE_REL_BASED_REL32 and skips the path where I mark them as invariant.

[AndyAyersMS]

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 a GT_CLS_VAR as invariant or use the GT_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]

or use the GT_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]

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]

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: 53052180
Total bytes of diff: 53052077
Total 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 the GT_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]

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]

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 see eeGetRelocTypeHint always return -1 for Arm64 and any non-windows x64 (an attempt to enable it for *nix 64bit platforms was declined dotnet/coreclr#12831)

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

[AndyAyersMS]

Thanks for all your work on this.