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

[EgorBo]

I am sure there are similar issues but mine comes with a possible fix, let me explain the problem via the following example:

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

static int Foo(int i)
{
    if ((uint)s_Array.Length > (uint)i)
        return s_Array[i];
    return 0;
}

Some of us for sure know that bound check elimination won't work here and you have to save s_Array to a local variable first.
The bound checks won't be removed even for a canonical for loop (but works for foreach since it saves the field to a local first - see Question: Why for loop is 1.3 slower over byte[] than foreach):

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

static int Foo1()
{
    for (int i = 0; i < s_Array.Length; i++) 
        Console.WriteLine(s_Array[i]); // bound check!
}

static int Foo2()
{
    foreach (int i in s_Array)
        Console.WriteLine(i); // no bound checks, foreach introduces a local for s_Array
}

It happens because JIT assigns different VN for loads, e.g.:

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

static int Foo() => s_Array.Length + s_Array.Length;

IR after fgValueNumber for Foo():

*  RETURN    int    $240
\--*  ADD       int    <l:$206, c:$205>
   +--*  ARR_LENGTH int    <l:$201, c:$200>
   |  \--*  IND       ref    <l:$100, c:$140>
   |     \--*  CNS_INT(h) long   0x2291cae2c80 static Fseq[s_Array] $c0
   \--*  ARR_LENGTH int    <l:$203, c:$202>
      \--*  IND       ref    <l:$100, c:$141>
         \--*  CNS_INT(h) long   0x2291cae2c80 static Fseq[s_Array] $c0

^ As you can see both IND have different VNs (140 and 141) thus we see a redundant memory load in the final codegen:

G_M53658_IG01:
G_M53658_IG02:
       48B8802C009097010000 mov      rax, 0x19790002C80
       488B00               mov      rax, gword ptr [rax]
       8B5008               mov      edx, dword ptr [rax+8]
       035008               add      edx, dword ptr [rax+8] ;; redundant load here!
       8BC2                 mov      eax, edx
G_M53658_IG03:
       C3                   ret

So what if we can tell the jit that if that field is static readonly and the type is already statically inited (e.g. when we recompile Foo after a while to promote it to Tier1 from Tier0) we can mark such indirects as invariant? Here is my fix: EgorBo@76d919c
and here is the new codegen for the sample above ^:

G_M53658_IG01:
G_M53658_IG02:
       48B8802C001081020000 mov      rax, 0x28110002C80
       488B00               mov      rax, gword ptr [rax]
       8B4008               mov      eax, dword ptr [rax+8]
       03C0                 add      eax, eax
G_M53658_IG03:
       C3                   ret

As you can see there are no redundant loads here any more. The bound checks are also removed correctly for the other samples above with this fix.

So is it safe to assume such readonly fields are invariant? @dotnet/jit-contrib @jkotas ?

category:cq
theme:value-numbering
skill-level:intermediate
cost:small

[EgorBo]

using System;
using System.Runtime.CompilerServices;
using System.Threading;

public class Program
{
    static void Main()
    {
        // emulate tier0 -> tier1 compilation
        // thus tier1 happens when `Program` is statically inited
        for (int i = 0; i < 200; i++)
        {
            Foo(0);
            Thread.Sleep(15);
        }

        Console.ReadKey();
    }

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

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Foo(int i)
    {
        if ((uint)s_Array.Length > (uint)i)
            return s_Array[i];
        return 0;
    }
}

Current codegen with TieredCompilation disabled for Foo():

G_M29289_IG01:
       56                   push     rsi
       4883EC20             sub      rsp, 32
       8BF1                 mov      esi, ecx
G_M29289_IG02:
       48B9F026C570FD7F0000 mov      rcx, 0x7FFD70C526F0
       BA01000000           mov      edx, 1
       E805A08D5F           call     CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
       48B8782C0010E3010000 mov      rax, 0x1E310002C78
       488B00               mov      rax, gword ptr [rax]
       397008               cmp      dword ptr [rax+8], esi
       7612                 jbe      SHORT G_M29289_IG05
G_M29289_IG03:
       3B7008               cmp      esi, dword ptr [rax+8]
       7315                 jae      SHORT G_M29289_IG07
       4863D6               movsxd   rdx, esi
       8B449010             mov      eax, dword ptr [rax+4*rdx+16]
G_M29289_IG04:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret      
G_M29289_IG05:
       33C0                 xor      eax, eax
G_M29289_IG06:
       4883C420             add      rsp, 32
       5E                   pop      rsi
       C3                   ret      
G_M29289_IG07:
       E884F24F5F           call     CORINFO_HELP_RNGCHKFAIL
       CC                   int3

Current codegen with TieredCompilation for Tier1:

G_M29289_IG01:
       4883EC28             sub      rsp, 40
G_M29289_IG02:
       48B8782C0090DB020000 mov      rax, 0x2DB90002C78
       488B00               mov      rax, gword ptr [rax]
       394808               cmp      dword ptr [rax+8], ecx
       7611                 jbe      SHORT G_M29289_IG05
G_M29289_IG03:
       3B4808               cmp      ecx, dword ptr [rax+8]
       7313                 jae      SHORT G_M29289_IG07
       4863D1               movsxd   rdx, ecx
       8B449010             mov      eax, dword ptr [rax+4*rdx+16]
G_M29289_IG04:
       4883C428             add      rsp, 40
       C3                   ret
G_M29289_IG05:
       33C0                 xor      eax, eax
G_M29289_IG06:
       4883C428             add      rsp, 40
       C3                   ret
G_M29289_IG07:
       E87DF2505F           call     CORINFO_HELP_RNGCHKFAIL
       CC                   int3

New codegen (with my fix) with TieredCompilation for Tier1:

G_M29289_IG01:
G_M29289_IG02:
       48B8782C92DEAE010000 mov      rax, 0x1AEDE922C78
       488B00               mov      rax, gword ptr [rax]
       8B5008               mov      edx, dword ptr [rax+8]
       3BD1                 cmp      edx, ecx
       7608                 jbe      SHORT G_M29289_IG05
G_M29289_IG03:
       4863D1               movsxd   rdx, ecx
       8B449010             mov      eax, dword ptr [rax+4*rdx+16]
G_M29289_IG04:
       C3                   ret
G_M29289_IG05:
       33C0                 xor      eax, eax
G_M29289_IG06:
       C3                   ret

[AndyAyersMS]

Yes, this should be safe.

Note the array length field is special. You should verify that the array elements are still considered variant:

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

static int Foo1()
{
   int sum = 0;
   for (int i = 0; i < s_Array.Length; i++) 
        sum += s_Array[i] + s_Array[i] + s_Array[0]; // no cse or hoist of the element access
   return sum;
}

[EgorBo]

Ah, right.
But it should work for immutable value types e.g. Guid and array.Length. It turns out it already works for SIMD types.

[EgorBo]

e.g.:

private static readonly Guid v1 = Guid.NewGuid();

[MethodImpl(MethodImplOptions.NoInlining)]
static bool Foo(int i)
{
    return v1 == v1;
}

G_M14484_IG01:
       4883EC48             sub      rsp, 72
       C5F877               vzeroupper
                                                ;; bbWeight=1    PerfScore 1.25
G_M14484_IG02:
       48B9782C0010F8010000 mov      rcx, 0x1F810002C78
       488B09               mov      rcx, gword ptr [rcx]
       C5FA6F4108           vmovdqu  xmm0, xmmword ptr [rcx+8]
       C5FA7F442438         vmovdqu  xmmword ptr [rsp+38H], xmm0
       48B9782C0010F8010000 mov      rcx, 0x1F810002C78           ;; dup!
       488B09               mov      rcx, gword ptr [rcx]         ;; dup!
       C5FA6F4108           vmovdqu  xmm0, xmmword ptr [rcx+8]    ;; dup!
       C5FA7F442428         vmovdqu  xmmword ptr [rsp+28H], xmm0
       488D4C2438           lea      rcx, bword ptr [rsp+38H]
       488D542428           lea      rdx, bword ptr [rsp+28H]
       E82A70FFFF           call     System.Guid:op_Equality(System.Guid,System.Guid):bool
       90                   nop
                                                ;; bbWeight=1    PerfScore 12.75
G_M14484_IG03:
       4883C448             add      rsp, 72
       C3                   ret

[EgorBo]

jit-diff (--pmi --cctors) is quite promising:

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies [invoking .cctors] for  default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: -20484 (-0.04% of base)
    diff is an improvement.
Top file regressions (bytes):
         302 : Microsoft.CSharp.dasm (0.08% of base)
          95 : Microsoft.Extensions.FileSystemGlobbing.dasm (0.34% of base)
          47 : System.Runtime.Caching.dasm (0.08% of base)
          42 : System.Runtime.Serialization.Formatters.dasm (0.04% of base)
          31 : System.DirectoryServices.AccountManagement.dasm (0.01% of base)
          14 : System.Security.Cryptography.Algorithms.dasm (0.00% of base)
          14 : System.Security.Cryptography.Cng.dasm (0.01% of base)
           5 : Microsoft.Diagnostics.NETCore.Client.dasm (0.05% of base)
           5 : System.IO.Compression.dasm (0.01% of base)
Top file improvements (bytes):
      -11199 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.39% of base)
       -1373 : System.Private.Xml.dasm (-0.04% of base)
        -794 : ILCompiler.Reflection.ReadyToRun.dasm (-0.57% of base)
        -661 : System.Data.Common.dasm (-0.05% of base)
        -609 : System.Private.DataContractSerialization.dasm (-0.08% of base)
        -560 : System.Net.Sockets.dasm (-0.30% of base)
        -435 : System.Data.Odbc.dasm (-0.23% of base)
        -350 : System.Collections.Immutable.dasm (-0.03% of base)
        -321 : System.Net.Security.dasm (-0.24% of base)
        -314 : System.Net.Http.dasm (-0.04% of base)
        -300 : System.Data.OleDb.dasm (-0.11% of base)
        -262 : System.Net.HttpListener.dasm (-0.13% of base)
        -245 : System.Diagnostics.FileVersionInfo.dasm (-6.11% of base)
        -217 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.00% of base)
        -211 : System.Private.CoreLib.dasm (-0.00% of base)
        -202 : System.Text.Json.dasm (-0.03% of base)
        -191 : FSharp.Core.dasm (-0.01% of base)
        -171 : Newtonsoft.Json.dasm (-0.02% of base)
        -160 : System.Net.Primitives.dasm (-0.25% of base)
        -149 : System.Diagnostics.TraceSource.dasm (-0.37% of base)
79 total files with Code Size differences (70 improved, 9 regressed), 188 unchanged.
Top method regressions (bytes):
         403 (13.05% of base) : Microsoft.CSharp.dasm - ExpressionBinder:GetStandardAndLiftedBinopSignatures(List`1,BinOpArgInfo):bool:this
         118 ( 2.81% of base) : System.Data.Common.dasm - DataTable:SerializeTableSchema(SerializationInfo,StreamingContext,bool):this
         103 ( 0.83% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - RegisteredTraceEventParser:GetManifestForRegisteredProvider(Guid):String
         100 ( 4.37% of base) : Microsoft.Extensions.FileSystemGlobbing.dasm - PatternBuilder:Build(String):IPattern:this
          73 ( 3.77% of base) : System.Private.Xml.dasm - XslAstRewriter:Refactor(XslNode,int):this
          51 ( 4.23% of base) : Microsoft.CodeAnalysis.dasm - Parser:ParseNamedTypeSymbol(String,byref,Compilation,ISymbol,List`1)
          51 ( 2.03% of base) : System.Private.CoreLib.dasm - TimeZoneInfo:TryCreateAdjustmentRules(String,byref,byref,byref,int):bool
          50 ( 3.47% of base) : System.DirectoryServices.Protocols.dasm - LdapSessionOptions:ProcessNotifyConnection(long,long,long,String,long,int,SEC_WINNT_AUTH_IDENTITY_EX,Luid,int):bool:this
          48 ( 1.41% of base) : System.Private.Xml.dasm - XmlAnyConverter:ChangeType(Object,Type,IXmlNamespaceResolver):Object:this
          47 ( 2.44% of base) : System.Runtime.Caching.dasm - HostFileChangeMonitor:InitDisposableMembers():this
          44 ( 2.47% of base) : System.Private.Xml.dasm - XmlILOptimizerVisitor:VisitFunction(QilFunction):QilNode:this
          43 ( 2.05% 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
          34 ( 2.40% of base) : System.Net.Sockets.dasm - SocketAsyncEventArgs:FinishOperationSyncSuccess(int,int):this
          30 ( 1.54% 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
          25 ( 6.74% of base) : Microsoft.CodeAnalysis.CSharp.dasm - StackOptimizerPass1:VisitArrayInitialization(BoundArrayInitialization):BoundNode:this
          23 ( 1.77% of base) : Microsoft.CodeAnalysis.dasm - Parser:ParseDeclaredId(String,byref,Compilation,List`1)
          23 ( 5.02% of base) : System.Drawing.Common.dasm - GPStream:CopyTo(IStream,long,long,long):this
          22 ( 1.61% of base) : System.Data.Common.dasm - FunctionNode:Eval(DataRow,int):Object:this
Top method improvements (bytes):
       -5455 (-5.32% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ApplicationServerTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
       -2023 (-4.80% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - KernelTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
       -1293 (-4.60% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ClrPrivateTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
       -1227 (-4.67% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ClrTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
        -636 (-4.36% 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
        -326 (-14.60% 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.43% of base) : System.Data.Common.dasm - DataTable:DeserializeTableSchema(SerializationInfo,StreamingContext,bool):this
        -210 (-4.11% 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
        -167 (-4.15% of base) : ILCompiler.Reflection.ReadyToRun.dasm - GcInfo:ToString():String:this (2 methods)
        -131 (-5.55% of base) : ILCompiler.Reflection.ReadyToRun.dasm - InfoHdrSmall:ToString():String:this
        -113 (-35.76% of base) : System.Net.HttpListener.dasm - CookieTokenizer:TokenFromName(bool):int:this
        -113 (-35.76% of base) : System.Net.Primitives.dasm - CookieTokenizer:TokenFromName(bool):int:this
        -112 (-4.64% of base) : System.Private.Xml.dasm - BigNumber:DblToRgbFast(double,ref,byref,byref):bool
        -110 (-3.82% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - SymbolTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
        -110 (-3.76% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - HeapTraceProviderTraceEventParser:EnumerateTemplates(Func`3,Action`1):this
Top method regressions (percentages):
         403 (13.05% of base) : Microsoft.CSharp.dasm - ExpressionBinder:GetStandardAndLiftedBinopSignatures(List`1,BinOpArgInfo):bool:this
           4 ( 7.27% of base) : System.Data.Common.dasm - Operators:Priority(int):int
          25 ( 6.74% of base) : Microsoft.CodeAnalysis.CSharp.dasm - StackOptimizerPass1:VisitArrayInitialization(BoundArrayInitialization):BoundNode:this
          23 ( 5.02% of base) : System.Drawing.Common.dasm - GPStream:CopyTo(IStream,long,long,long):this
         100 ( 4.37% of base) : Microsoft.Extensions.FileSystemGlobbing.dasm - PatternBuilder:Build(String):IPattern:this
          51 ( 4.23% of base) : Microsoft.CodeAnalysis.dasm - Parser:ParseNamedTypeSymbol(String,byref,Compilation,ISymbol,List`1)
          73 ( 3.77% of base) : System.Private.Xml.dasm - XslAstRewriter:Refactor(XslNode,int):this
          13 ( 3.74% of base) : System.Private.CoreLib.dasm - RegistryKey:GetSubKeyNames():ref:this
          50 ( 3.47% of base) : System.DirectoryServices.Protocols.dasm - LdapSessionOptions:ProcessNotifyConnection(long,long,long,String,long,int,SEC_WINNT_AUTH_IDENTITY_EX,Luid,int):bool:this
          38 ( 3.30% of base) : System.Runtime.Serialization.Formatters.dasm - ObjectWriter:Write(WriteObjectInfo,NameInfo,NameInfo):this
          21 ( 3.28% of base) : System.Net.Mail.dasm - NegotiateStreamPal:MakeSignature(SafeDeleteContext,ref,int,int,byref):int
          16 ( 2.86% of base) : System.Private.CoreLib.dasm - TaskScheduler:GetTaskSchedulersForDebugger():ref
           8 ( 2.84% of base) : System.Data.Common.dasm - DbDataAdapter:Fill(DataSet,int,int,String):int:this
         118 ( 2.81% of base) : System.Data.Common.dasm - DataTable:SerializeTableSchema(SerializationInfo,StreamingContext,bool):this
          11 ( 2.72% of base) : System.ComponentModel.TypeConverter.dasm - ReflectPropertyDescriptor:CanResetValue(Object):bool:this
          44 ( 2.47% of base) : System.Private.Xml.dasm - XmlILOptimizerVisitor:VisitFunction(QilFunction):QilNode:this
          47 ( 2.44% of base) : System.Runtime.Caching.dasm - HostFileChangeMonitor:InitDisposableMembers():this
          34 ( 2.40% of base) : System.Net.Sockets.dasm - SocketAsyncEventArgs:FinishOperationSyncSuccess(int,int):this
           8 ( 2.22% of base) : Microsoft.CodeAnalysis.dasm - SourceText:Write(TextWriter,TextSpan,CancellationToken):this
           6 ( 2.14% of base) : System.Private.Xml.dasm - XmlTextWriter:GeneratePrefix():String:this
Top method improvements (percentages):
        -113 (-35.76% of base) : System.Net.HttpListener.dasm - CookieTokenizer:TokenFromName(bool):int:this
        -113 (-35.76% of base) : System.Net.Primitives.dasm - CookieTokenizer:TokenFromName(bool):int:this
         -21 (-28.38% of base) : System.Net.HttpListener.dasm - HttpListenerResponse:CanSendResponseBody(int):bool
         -21 (-28.00% of base) : System.Private.DataContractSerialization.dasm - ObjectToIdCache:GetPrime(int):int
         -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
         -22 (-19.82% of base) : System.Drawing.Primitives.dasm - KnownColorTable:ArgbToKnownColor(int):Color
         -26 (-19.70% of base) : System.Data.Common.dasm - DataStorage:GetStorageType(Type):int
         -25 (-18.94% of base) : System.Private.CoreLib.dasm - OAVariantLib:GetCVTypeFromClass(Type):int
         -25 (-18.80% of base) : System.Private.Xml.dasm - XmlSchemaElement:.ctor():this
         -19 (-18.10% of base) : System.Data.Odbc.dasm - OdbcConnectionStringBuilder:Clear():this
         -35 (-17.86% of base) : Microsoft.CodeAnalysis.dasm - WellKnownTypes:AssertEnumAndTableInSync()
         -19 (-17.12% of base) : System.Data.OleDb.dasm - OleDbConnectionStringBuilder:Clear():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
        -326 (-14.60% of base) : System.Private.Xml.dasm - XmlDocument:.ctor(XmlImplementation):this
         -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
836 total methods with Code Size differences (707 improved, 129 regressed), 257829 unchanged.
Completed analysis in 31.58s

[Daniel-Svensson]

Nice improvements.
Would the same optimization be valid for instance level initonly fields when accessed outside the constructor?

[AndyAyersMS]

instance level initonly fields

Unfortunately, no -- instance fields can still be modified at runtime.

[BruceForstall]

@briansull

[briansull]

@EgorBo Did you want to put up your PR for this issue?

[EgorBo]

@briansull will take a look tomorrow