JIT doesn't always fold 'add' instructions into 'lea' which immediately follows it

[GrabYourPitchforks]

Repro code:

{
class Program
{
    static int ReadInt32Field(object @this, nuint offset)
    {
        ref byte b = ref Unsafe.As<RawObjData>(@this).Data;
        b = ref Unsafe.Subtract(ref b, IntPtr.Size);
        b = ref Unsafe.AddByteOffset(ref b, (nint)offset);
        return Unsafe.ReadUnaligned<int>(ref b);
    }
}

sealed class RawObjData
{
    public byte Data;
}

; Method ConsoleApp62.Program:ReadInt32Field(System.Object,long):int
G_M62650_IG01:
						;; bbWeight=1    PerfScore 0.00

G_M62650_IG02:
       cmp      dword ptr [rcx], ecx
       add      rcx, 8
       mov      eax, dword ptr [rcx+rdx-8]
						;; bbWeight=1    PerfScore 4.25

G_M62650_IG03:
       ret      
						;; bbWeight=1    PerfScore 1.00
; Total bytes of code: 11

In this codegen, both the add and the mov instruction can be collapsed into a single mov eax, dword ptr [rcx + rdx] instruction, where the +8 and the -8 cancel each other out.

We see this sometimes with very low-level object manipulation in corelib. For example, the codegen listed at the top of #51548 (working on an Array.Clear optimization) has this pattern. We even have a specialized intrinsic as a temporary workaround in extremely hot code paths. (This intrinsic is used in Memory<T>.get_Span, for instance.)

It'd be great if JIT could somehow collapse these sequences where they occur. That would ideally result in better codegen and would allow us to remove this intrinsic from the code base.

[benaadams]

Increasing the arguments to addressing is not always a clear win as on Intel 3+ operand addressing has a latency of 3 clocks and 1 port available to execute on vs 2 operand addressing which is 1 clock and has a choice of 2 ports...

However... going from add + 3 op addressing mov; to a single 2 op addressing mov is clearly a win 😁

[AndyAyersMS]

This is likely another case for forward substitution: #6973.

When a chain of computations are expressed via inlines the trees get broken and nothing glues them back together.

[tannergooding]

For Unsafe.Add (and a few other Unsafe.* methods) in particular, can we treat it as an intrinsic and elide the need to "inline" completely?

I imagine that would also be beneficial for inlining heuristics and just for general throughput in heavily optimized code.

[AndyAyersMS]

Sure, seems like a good idea.

We will need to invest in forward sub soonish as we can't intrinsify our way out of everything.

[tannergooding]

We will need to invest in forward sub soonish as we can't intrinsify our way out of everything.

Definitely. I was thinking about this from the perspective of:

• Something we could do for .NET 6
• Something that is likely additionally beneficial given its usage in perf oriented code
• Having to do forward substitution for many 1 IL instruction methods doesn't seem great

[tannergooding]

I imagine the forward substitution will be important for scenarios like generic-math, given that T + T will be a constrained call and the operator is likely simple. Likewise these scenarios are probably much harder to intrinsify than Unsafe 😄.

Unsafe itself might also be slightly difficult to intrinsify given it is in an external to S.P.Corelib binary. That isn't unprecedented, as we did it previously for System.Numerics.Vectors.dll, but its also not the greatest thing to have.

[JulieLeeMSFT]

Moving to future but with priority 1 to consider for .NET7.
@AndyAyersMS please pull it in to .NET6 if you think we can do it in .NET6.

[tannergooding]

I prototyped treating most of Internal.Runtime.CompilerServices.Unsafe as Intrinsic: main...tannergooding:unsafe-intrinsics

This resulted in the following for the --pmi --frameworks diffs:

Found 426 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 53358028
Total bytes of diff: 53352095
Total bytes of delta: -5933 (-0.01% of base)
    diff is an improvement.


Top file regressions (bytes):
         218 : Microsoft.CodeAnalysis.dasm (0.01% of base)
         101 : System.Linq.Parallel.dasm (0.00% of base)
          40 : System.Net.Quic.dasm (0.06% of base)
          24 : System.ComponentModel.TypeConverter.dasm (0.01% of base)
          23 : System.Linq.Expressions.dasm (0.00% of base)
          18 : Microsoft.Extensions.DependencyModel.dasm (0.03% of base)
          16 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          15 : System.IO.Packaging.dasm (0.02% of base)
          14 : System.Runtime.Numerics.dasm (0.02% of base)
          11 : dotnet-Microsoft.XmlSerializer.Generator.dasm (0.03% of base)
          11 : xunit.console.dasm (0.01% of base)
           9 : System.IO.FileSystem.Watcher.dasm (0.07% of base)
           8 : System.Text.Encodings.Web.dasm (0.03% of base)
           6 : System.DirectoryServices.dasm (0.00% of base)
           5 : Microsoft.Extensions.Logging.Console.dasm (0.01% of base)
           4 : xunit.execution.dotnet.dasm (0.00% of base)
           4 : System.Runtime.Caching.dasm (0.01% of base)
           3 : System.Management.dasm (0.00% of base)
           3 : Microsoft.Diagnostics.NETCore.Client.dasm (0.03% of base)
           3 : System.Drawing.Primitives.dasm (0.01% of base)

Top file improvements (bytes):
       -1767 : System.Formats.Asn1.dasm (-1.87% of base)
       -1527 : System.Private.CoreLib.dasm (-0.03% of base)
        -735 : System.Security.Cryptography.Primitives.dasm (-1.76% of base)
        -412 : System.Security.Cryptography.Pkcs.dasm (-0.10% of base)
        -347 : System.Memory.dasm (-0.13% of base)
        -219 : System.Security.Cryptography.Algorithms.dasm (-0.07% of base)
        -167 : System.Security.Cryptography.Cng.dasm (-0.10% of base)
        -167 : System.Text.Json.dasm (-0.02% of base)
        -143 : System.Net.Http.dasm (-0.02% of base)
         -92 : System.Security.Cryptography.X509Certificates.dasm (-0.06% of base)
         -85 : System.Private.Uri.dasm (-0.10% of base)
         -83 : System.Text.RegularExpressions.dasm (-0.03% of base)
         -64 : Microsoft.Extensions.Primitives.dasm (-0.25% of base)
         -55 : System.Net.Sockets.dasm (-0.03% of base)
         -51 : System.Collections.Immutable.dasm (-0.00% of base)
         -43 : System.Net.Primitives.dasm (-0.07% of base)
         -42 : System.Private.Xml.Linq.dasm (-0.03% of base)
         -38 : System.IO.FileSystem.dasm (-0.04% of base)
         -37 : System.Net.Ping.dasm (-0.23% of base)
         -33 : System.Diagnostics.DiagnosticSource.dasm (-0.04% of base)

84 total files with Code Size differences (58 improved, 26 regressed), 187 unchanged.

Top method regressions (bytes):
         178 ( 0.31% of base) : Microsoft.CodeAnalysis.dasm - ArrayBuilder`1:ToDictionary(Func`2,IEqualityComparer`1):Dictionary`2:this (64 methods)
         144 ( 0.88% of base) : System.Text.Json.dasm - ObjectWithParameterizedConstructorConverter`1:TryLookupConstructorParameter(byref,byref,JsonSerializerOptions,byref):bool:this (8 methods)
         103 (19.77% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):Vector`1
          97 (21.60% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):double
          97 (21.70% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):Nullable`1
          90 (20.22% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):ubyte
          90 (20.22% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):short
          90 (20.22% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):int
          90 (20.13% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):long
          79 (14.29% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):__Canon
          63 ( 1.01% of base) : System.Linq.Parallel.dasm - OrderedIntersectQueryOperatorEnumerator`1:MoveNext(byref,byref):bool:this (8 methods)
          58 ( 0.36% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:EnsureCapacity(int,int):this (8 methods)
          49 ( 3.07% of base) : System.Private.CoreLib.dasm - Task:RunContinuations(Object):this
          40 ( 6.01% of base) : System.Net.Http.dasm - MultiArrayBuffer:GrowAvailableSpace(int):this
          40 ( 6.01% of base) : System.Net.Quic.dasm - MultiArrayBuffer:GrowAvailableSpace(int):this
          38 ( 0.43% of base) : System.Linq.Parallel.dasm - OrderedExceptQueryOperatorEnumerator`1:MoveNext(byref,byref):bool:this (8 methods)
          27 (36.00% of base) : System.Linq.Expressions.dasm - KeyedStack`2:TryPop(Vector`1):__Canon:this
          25 ( 2.23% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:InsertAt(int,short,int,int):this
          25 ( 2.23% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:InsertAt(int,int,int,int):this
          25 ( 2.23% of base) : System.Numerics.Tensors.dasm - CompressedSparseTensor`1:InsertAt(int,long,int,int):this

Top method improvements (bytes):
       -1265 (-13.73% of base) : System.Formats.Asn1.dasm - AsnWriter:WriteUtcTimeCore(Asn1Tag,DateTimeOffset):this
        -735 (-13.87% of base) : System.Security.Cryptography.Primitives.dasm - <ReadAsyncCore>d__44:MoveNext():this
        -619 (-10.28% of base) : System.Text.Json.dasm - JsonDocument:Parse(ReadOnlySpan`1,JsonReaderOptions,byref,byref)
        -360 (-2.79% of base) : System.Formats.Asn1.dasm - AsnWriter:WriteGeneralizedTimeCore(Asn1Tag,DateTimeOffset,bool):this
        -128 (-0.69% of base) : System.Text.Json.dasm - JsonValue`1:TryConvertJsonElement(byref):bool:this (64 methods)
         -98 (-1.06% of base) : System.Private.CoreLib.dasm - TlsOverPerCoreLockedStacksArrayPool`1:Trim():bool:this (10 methods)
         -98 (-4.92% of base) : System.Private.CoreLib.dasm - ReadOnlyMemory`1:Equals(Object):bool:this (8 methods)
         -90 (-2.05% of base) : System.Security.Cryptography.Pkcs.dasm - Rfc3161TstInfo:DecodeCore(byref,Asn1Tag,ReadOnlyMemory`1,byref)
         -88 (-3.05% of base) : System.Private.CoreLib.dasm - Dictionary`2:FindValue(__Canon):byref:this (5 methods)
         -85 (-5.09% of base) : System.Text.Json.dasm - JsonTypeInfo:GetProperty(ReadOnlySpan`1,byref,byref):JsonPropertyInfo:this
         -85 (-5.19% of base) : System.Text.Json.dasm - JsonTypeInfo:GetParameter(ReadOnlySpan`1,byref,byref):JsonParameterInfo:this
         -83 (-23.85% of base) : System.Numerics.Tensors.dasm - DenseTensor`1:IndexOf(__Canon):int:this
         -70 (-12.20% of base) : System.Private.CoreLib.dasm - MemoryExtensions:Overlaps(ReadOnlySpan`1,ReadOnlySpan`1):bool (8 methods)
         -68 (-9.39% of base) : System.Memory.dasm - ArrayBufferWriter`1:get_WrittenMemory():ReadOnlyMemory`1:this (8 methods)
         -65 (-13.08% of base) : System.Memory.dasm - Base64:DecodeFromUtf8InPlace(Span`1,byref):int
         -54 (-2.41% of base) : System.Memory.dasm - ReadOnlySequence`1:GetFirstBufferSlow(Object,bool):ReadOnlyMemory`1:this (8 methods)
         -54 (-11.23% of base) : System.Memory.dasm - Base64:EncodeToUtf8InPlace(Span`1,int,byref):int
         -51 (-1.58% of base) : System.Numerics.Tensors.dasm - DenseTensor`1:CopyTo(ref,int):this (8 methods)
         -51 (-1.15% of base) : System.Memory.dasm - ReadOnlySequence`1:TryGet(byref,byref,bool):bool:this (8 methods)
         -51 (-28.81% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:InterlockedExchange(byref,ImmutableArray`1):ImmutableArray`1 (8 methods)

Top method regressions (percentages):
          13 (81.25% of base) : Microsoft.CodeAnalysis.dasm - MultiDictionary`2:ContainsKey(__Canon):bool:this
          13 (81.25% of base) : System.IO.Packaging.dasm - OrderedDictionary`2:Contains(__Canon):bool:this
          17 (70.83% of base) : Microsoft.CodeAnalysis.dasm - OrderPreservingMultiDictionary`2:ContainsKey(__Canon):bool:this
          27 (36.00% of base) : System.Linq.Expressions.dasm - KeyedStack`2:TryPop(Vector`1):__Canon:this
           7 (24.14% of base) : System.Private.CoreLib.dasm - String:GetHashCode():int:this
          11 (23.40% of base) : System.Private.CoreLib.dasm - OrdinalCaseSensitiveComparer:GetHashCode(String):int:this
          97 (21.70% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):Nullable`1
          97 (21.60% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):double
          90 (20.22% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):ubyte
          90 (20.22% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):short
          90 (20.22% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):int
          90 (20.13% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):long
         103 (19.77% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):Vector`1
           5 (17.86% of base) : System.Private.CoreLib.dasm - Buffer:Memcpy(long,int,ref,int,int)
          21 (15.67% of base) : System.Private.CoreLib.dasm - OrdinalComparer:GetHashCode(String):int:this (3 methods)
          11 (14.67% of base) : xunit.console.dasm - HashCodeCombiner:Add(String):this
          11 (14.67% of base) : Microsoft.Extensions.DependencyModel.dasm - HashCodeCombiner:Add(String):this
          79 (14.29% of base) : System.Text.Json.dasm - JsonSerializer:ReadUsingMetadata(ReadOnlySpan`1,JsonTypeInfo):__Canon
          11 ( 8.46% of base) : dotnet-Microsoft.XmlSerializer.Generator.dasm - Sgen:GetTempAssemblyName(AssemblyName,String):String
          11 ( 8.46% of base) : System.Private.Xml.dasm - Compiler:GetTempAssemblyName(AssemblyName,String):String

Top method improvements (percentages):
         -51 (-28.81% of base) : System.Collections.Immutable.dasm - ImmutableInterlocked:InterlockedExchange(byref,ImmutableArray`1):ImmutableArray`1 (8 methods)
         -83 (-23.85% of base) : System.Numerics.Tensors.dasm - DenseTensor`1:IndexOf(__Canon):int:this
         -16 (-21.92% of base) : System.Private.CoreLib.dasm - Container:TryGetValueWorker(__Canon,byref):bool:this
          -3 (-18.75% of base) : System.Private.CoreLib.dasm - Unsafe:Write(long,__Canon)
          -3 (-18.75% of base) : System.Private.CoreLib.dasm - Unsafe:Write(byref,__Canon)
         -47 (-17.28% of base) : System.Private.CoreLib.dasm - Enumerator:Dispose():this (49 methods)
          -7 (-16.67% of base) : System.Private.CoreLib.dasm - String:InternalCopy(String,long,int)
        -735 (-13.87% of base) : System.Security.Cryptography.Primitives.dasm - <ReadAsyncCore>d__44:MoveNext():this
       -1265 (-13.73% of base) : System.Formats.Asn1.dasm - AsnWriter:WriteUtcTimeCore(Asn1Tag,DateTimeOffset):this
          -6 (-13.64% of base) : System.Private.CoreLib.dasm - String:CompareOrdinalHelper(String,int,int,String,int,int):int
         -65 (-13.08% of base) : System.Memory.dasm - Base64:DecodeFromUtf8InPlace(Span`1,byref):int
         -70 (-12.20% of base) : System.Private.CoreLib.dasm - MemoryExtensions:Overlaps(ReadOnlySpan`1,ReadOnlySpan`1):bool (8 methods)
         -54 (-11.23% of base) : System.Memory.dasm - Base64:EncodeToUtf8InPlace(Span`1,int,byref):int
          -2 (-11.11% of base) : System.Private.CoreLib.dasm - RuntimeHelpers:GetRawArrayData(Array):byref
         -24 (-11.06% of base) : System.Private.CoreLib.dasm - ReadOnlySpan`1:op_Equality(ReadOnlySpan`1,ReadOnlySpan`1):bool (8 methods)
         -24 (-11.06% of base) : System.Private.CoreLib.dasm - Span`1:op_Equality(Span`1,Span`1):bool (8 methods)
         -21 (-11.05% of base) : System.Formats.Asn1.dasm - SpanBasedEncoding:GetBytes(ref,int,int,ref,int):int:this
         -21 (-11.05% of base) : System.Formats.Asn1.dasm - SpanBasedEncoding:GetChars(ref,int,int,ref,int):int:this
          -3 (-10.34% of base) : Microsoft.CodeAnalysis.dasm - DocumentationCommentIncludeCache:KeyHashCode(String):int
        -619 (-10.28% of base) : System.Text.Json.dasm - JsonDocument:Parse(ReadOnlySpan`1,JsonReaderOptions,byref,byref)

1122 total methods with Code Size differences (825 improved, 297 regressed), 268629 unchanged.

Most of the diffs are due to fewer blocks. For example, CastHelpers:ChkCastAny goes from 11 blocks to 8 blocks and tracks one fewer locals. In more extreme cases, like SpanHelpers:IndexOf(byref,ushort,int):int it drops from 27 single block inlinees down to 12 blocks and is able to track 4 fewer locals/temps. Buffer:Memmove(byref,byref,long) is another where it drops from 43 single block inlinees down to 0 and has 26 fewer locals/temps to track.

Most of the diffs come from lea and the like being folded into the relevant memory access. For example, in Buffer:Memmove the following happens several times, saving 19 bytes total:

- lea      r10, bword ptr [rcx+16]
  vmovdqu  xmm0, xmmword ptr [rdx+16]
- vmovdqu  xmmword ptr [r10], xmm0
+ vmovdqu  xmmword ptr [rcx+16], xmm0

So I think its probably worth investigating this further and looking at applying the same to System.Runtime.CompilerServices.Unsafe.

Notably this also triggers https://github.com/dotnet/runtime/issues/10821 on Unsafe.Add and it wasn't clear to me if it was because I was constructing a node incorrectly (they looked to preserve TYP_BYREF as expected) or if this is something else.

@jkotas, @AndyAyersMS

[tannergooding]

Andy indicated (on Discord) I might need to do something like https://github.com/dotnet/coreclr/pull/21217/files to avoid the assert in #10821

[jkotas]

So I think its probably worth investigating this further and looking at applying the same to System.Runtime.CompilerServices.Unsafe.

System.Runtime.CompilerServices.Unsafe is OOB package today. Treating methods in OOBs as intrinsics tends to be problematic since the JIT needs to be forward compatible with future version of the OOB package. (We used to have this setup with Vector<T> on .NET Framework for a while.)

#45475 (comment) has some discussion about deprecating System.Runtime.CompilerServices.Unsafe OOB. So this optimization is another ref-count on that.