Fold IND(frozenObj, CNS) #85127

EgorBo · 2023-04-20T22:53:44Z

Needed for #85014 (to fold _methodPtr field access in a frozen delegate)

CQ improvement example:

static int Case1()
{
    // Read 4 bytes from a frozen object
    return Unsafe.As<char, int>(
        ref MemoryMarshal.GetReference("hello world".AsSpan()));
}

static bool Case2()
{
    return RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
}

Current codegen (Main):

; Assembly listing for method Program:Case1():int
       mov      rax, 0x13200309C10      ; 'hello world'
       add      rax, 12
       mov      eax, dword ptr [rax]
       ret      
; Total bytes of code 17


; Assembly listing for method Program:Case2():bool
       mov      rax, 0x1F480209C4C
       mov      rdx, 0x20002000200020
       or       rdx, qword ptr [rax]
       mov      rax, 0x64006E00690077
       xor      rax, rdx
       mov      rdx, 0x1F480209C52
       mov      rcx, 0x20002000200020
       or       rcx, qword ptr [rdx]
       mov      rdx, 0x730077006F0064
       xor      rdx, rcx
       or       rax, rdx
       sete     al
       movzx    rax, al
       ret      
; Total bytes of code 82

New codegen (PR):

; Assembly listing for method Program:Case1():int
       mov      eax, 0x650068
       ret      
; Total bytes of code 6


; Assembly listing for method Program:Case2():bool
       mov      eax, 1
       ret      
; Total bytes of code 6

ghost · 2023-04-20T22:53:54Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Needed for #85014 (to fold _methodPtr field access in a frozen delegate)

CQ improvement example:

static int Case1()
{
    // Read 4 bytes from a frozen object
    return Unsafe.As<char, int>(
        ref MemoryMarshal.GetReference("hello world".AsSpan()));
}

static bool Case2()
{
    return RuntimeInformation.IsOSPlatform(OSPlatform.Windows);
}

Current codegen (Main):

; Assembly listing for method Program:Case1():int
       mov      rax, 0x13200309C10      ; 'hello world'
       add      rax, 12
       mov      eax, dword ptr [rax]
       ret      
; Total bytes of code 17


; Assembly listing for method Program:Case2():bool
       mov      rax, 0x1F480209C4C
       mov      rdx, 0x20002000200020
       or       rdx, qword ptr [rax]
       mov      rax, 0x64006E00690077
       xor      rax, rdx
       mov      rdx, 0x1F480209C52
       mov      rcx, 0x20002000200020
       or       rcx, qword ptr [rdx]
       mov      rdx, 0x730077006F0064
       xor      rdx, rcx
       or       rax, rdx
       sete     al
       movzx    rax, al
       ret      
; Total bytes of code 82

New codegen (PR):

; Assembly listing for method Program:Case1():int
       mov      eax, 0x650068
       ret      
; Total bytes of code 6


; Assembly listing for method Program:Case2():bool
       mov      eax, 1
       ret      
; Total bytes of code 6

Author:	EgorBo
Assignees:	EgorBo
Labels:	`area-CodeGen-coreclr`
Milestone:	-

EgorBo · 2023-04-21T01:28:54Z

@jkotas can you please take a look at the VM side?

I targeted two cases here:

RuntimeInformation.IsOSPlatform where early unrolling produces IND(ADD(FROZEN, CNS)) node that I fold here - it doesn't apply to NAOT due to NativeAOT: Type is not pre-initialized #83043
Technically it should fold Delegate._methodPtr if we e.g. declare it as

static readonly Func<int,int> MyFunc = (x,y) => ...;

and with help of PGO we should be able to inline it at its invocation without GDV checks. For that we'd need to teach JIT to emit FOH allocators in static constructors so it's not happening today yet. So overall no strong motivation for this PR, just decided to give my prototype a try and can close if you think there are not much value here 🙂

Jit-diffs:

Total bytes of base: 56533471
Total bytes of diff: 56531587
Total bytes of delta: -1884 (-0.00 % of base)
Total relative delta: NaN
    diff is an improvement.
    relative diff is a regression.


Top file improvements (bytes):
        -718 : Microsoft.Diagnostics.Tools.RuntimeClient.dasm (-6.54% of base)
        -439 : Microsoft.DotNet.XUnitExtensions.dasm (-1.73% of base)
        -308 : System.Runtime.Serialization.Schema.dasm (-0.59% of base)
        -162 : System.Private.CoreLib.dasm (-0.00% of base)
         -76 : Microsoft.Extensions.DependencyModel.dasm (-0.10% of base)
         -67 : Microsoft.CSharp.dasm (-0.02% of base)
         -27 : System.Security.Cryptography.dasm (-0.00% of base)
         -26 : System.Management.dasm (-0.01% of base)
         -23 : System.ComponentModel.TypeConverter.dasm (-0.01% of base)
         -18 : System.Net.Sockets.dasm (-0.01% of base)
          -6 : System.Diagnostics.EventLog.dasm (-0.00% of base)
          -6 : Newtonsoft.Json.Bson.dasm (-0.01% of base)
          -3 : Microsoft.NET.WebAssembly.Webcil.dasm (-0.02% of base)
          -3 : Newtonsoft.Json.dasm (-0.00% of base)
          -2 : System.Net.Http.WinHttpHandler.dasm (-0.00% of base)

15 total files with Code Size differences (15 improved, 0 regressed), 261 unchanged.

Top method improvements (bytes):
        -718 (-79.34% of base) : Microsoft.Diagnostics.Tools.RuntimeClient.dasm - Microsoft.Diagnostics.Tools.RuntimeClient.DiagnosticsIpc.IpcClient:GetTransport(int):System.IO.Stream
        -439 (-39.80% of base) : Microsoft.DotNet.XUnitExtensions.dasm - Microsoft.DotNet.XUnitExtensions.DiscovererHelpers:TestPlatformApplies(int):bool
         -76 (-92.68% of base) : Microsoft.Extensions.DependencyModel.dasm - Microsoft.Extensions.DependencyModel.EnvironmentWrapper:IsWindows():bool:this
         -66 (-1.62% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:ExportClassDataContract(System.Runtime.Serialization.DataContracts.DataContract,System.Runtime.Serialization.ContractCodeDomInfo):this
         -66 (-3.66% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:ExportCollectionDataContract(System.Runtime.Serialization.DataContracts.DataContract,System.Runtime.Serialization.ContractCodeDomInfo):this
         -58 (-2.91% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:ExportEnumDataContract(System.Runtime.Serialization.DataContracts.DataContract,System.Runtime.Serialization.ContractCodeDomInfo):this
         -37 (-23.27% of base) : Microsoft.CSharp.dasm - Microsoft.CSharp.RuntimeBinder.CSharpInvokeConstructorBinder:IsEquivalentTo(Microsoft.CSharp.RuntimeBinder.ICSharpBinder):bool:this
         -37 (-1.61% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:ExportXmlDataContract(System.Runtime.Serialization.DataContracts.XmlDataContract,System.Runtime.Serialization.ContractCodeDomInfo):this
         -30 (-8.57% of base) : Microsoft.CSharp.dasm - Microsoft.CSharp.RuntimeBinder.ComInterop.UnsafeMethods:get_DynamicModule():System.Reflection.Emit.ModuleBuilder
         -29 (-3.40% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:AddKnownTypes(System.Runtime.Serialization.DataContracts.DataContract,System.Runtime.Serialization.ContractCodeDomInfo):this
         -29 (-2.00% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:ExportISerializableDataContract(System.Runtime.Serialization.DataContracts.DataContract,System.Runtime.Serialization.ContractCodeDomInfo):this
         -26 (-11.50% of base) : System.Management.dasm - System.Management.ManagementBaseObject:GetHashCode():int:this
         -23 (-9.58% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.ToolboxItemFilterAttribute:ToString():System.String:this
         -23 (-14.94% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:AddSerializableAttribute(bool,System.CodeDom.CodeTypeDeclaration,System.Runtime.Serialization.ContractCodeDomInfo):this
         -12 (-1.76% of base) : System.Net.Sockets.dasm - System.Net.Sockets.SocketAsyncEventArgs:DoOperationReceiveMessageFrom(System.Net.Sockets.Socket,System.Net.Sockets.SafeSocketHandle,System.Threading.CancellationToken):int:this
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[double](int,bool):double[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[int](int,bool):int[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[long](int,bool):long[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[short](int,bool):short[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[System.Nullable`1[int]](int,bool):System.Nullable`1[int][]

Top method improvements (percentages):
         -76 (-92.68% of base) : Microsoft.Extensions.DependencyModel.dasm - Microsoft.Extensions.DependencyModel.EnvironmentWrapper:IsWindows():bool:this
        -718 (-79.34% of base) : Microsoft.Diagnostics.Tools.RuntimeClient.dasm - Microsoft.Diagnostics.Tools.RuntimeClient.DiagnosticsIpc.IpcClient:GetTransport(int):System.IO.Stream
        -439 (-39.80% of base) : Microsoft.DotNet.XUnitExtensions.dasm - Microsoft.DotNet.XUnitExtensions.DiscovererHelpers:TestPlatformApplies(int):bool
         -37 (-23.27% of base) : Microsoft.CSharp.dasm - Microsoft.CSharp.RuntimeBinder.CSharpInvokeConstructorBinder:IsEquivalentTo(Microsoft.CSharp.RuntimeBinder.ICSharpBinder):bool:this
         -23 (-14.94% of base) : System.Runtime.Serialization.Schema.dasm - System.Runtime.Serialization.CodeExporter:AddSerializableAttribute(bool,System.CodeDom.CodeTypeDeclaration,System.Runtime.Serialization.ContractCodeDomInfo):this
         -26 (-11.50% of base) : System.Management.dasm - System.Management.ManagementBaseObject:GetHashCode():int:this
          -3 (-10.34% of base) : System.Private.CoreLib.dasm - System.RuntimeType:IsGenericCOMObjectImpl():bool:this
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[double](int,bool):double[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[int](int,bool):int[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[long](int,bool):long[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[short](int,bool):short[]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[System.Nullable`1[int]](int,bool):System.Nullable`1[int][]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[System.Numerics.Vector`1[float]](int,bool):System.Numerics.Vector`1[float][]
          -6 (-9.84% of base) : System.Private.CoreLib.dasm - System.GC:<AllocateUninitializedArray>g__AllocateNewUninitializedArray|66_0[ubyte](int,bool):ubyte[]
         -23 (-9.58% of base) : System.ComponentModel.TypeConverter.dasm - System.ComponentModel.ToolboxItemFilterAttribute:ToString():System.String:this
         -30 (-8.57% of base) : Microsoft.CSharp.dasm - Microsoft.CSharp.RuntimeBinder.ComInterop.UnsafeMethods:get_DynamicModule():System.Reflection.Emit.ModuleBuilder
          -3 (-6.67% of base) : System.Private.CoreLib.dasm - System.RuntimeType:get_IsActualEnum():bool:this
          -3 (-6.67% of base) : System.Private.CoreLib.dasm - System.RuntimeType:IsDelegate():bool:this
          -3 (-6.12% of base) : System.Private.CoreLib.dasm - System.GC:AllocateArray[double](int,bool):double[]
          -3 (-6.12% of base) : System.Private.CoreLib.dasm - System.GC:AllocateArray[int](int,bool):int[]

80 total methods with Code Size differences (80 improved, 0 regressed), 359004 unchanged.

src/coreclr/jit/valuenum.cpp

src/coreclr/vm/jitinterface.cpp

MichalStrehovsky · 2023-04-21T02:20:08Z

Do you have any thoughts on how this could apply to AOT?

If we want to be able to eventually read the delegate method pointer, that's doable, but we would need to communicate a reloc and the proposed interface doesn't allow that.

If we had some extra argument where the EE side could say the result is actually a reloc and this is the handle to use, it would be workable. We would just check on the EE side that the offset points to a relocatable part and the attempted read is pointer sized.

EgorBo · 2023-04-21T11:19:14Z

Do you have any thoughts on how this could apply to AOT?

If we want to be able to eventually read the delegate method pointer, that's doable, but we would need to communicate a reloc and the proposed interface doesn't allow that.

If we had some extra argument where the EE side could say the result is actually a reloc and this is the handle to use, it would be workable. We would just check on the EE side that the offset points to a relocatable part and the attempted read is pointer sized.

I've just added support for string literals for NAOT. For delegates I guess we need a real use-case first to make sure logic is tested so I'll extend once we find it, ok? For JIT the use case can be DynamicPGO emitting a GDV check but we currently never allocate delegates on FOH yet

EgorBo · 2023-04-21T11:20:38Z

@jakobbotsch @SingleAccretion can you please review jit side since you reviewed previous similiar changes.

src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs

EgorBo · 2023-04-21T22:28:59Z

/azp list

EgorBo · 2023-04-21T22:29:31Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-04-21T22:29:42Z

Azure Pipelines successfully started running 1 pipeline(s).

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs

src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs

src/coreclr/jit/valuenum.cpp

SingleAccretion · 2023-04-23T13:02:55Z

src/coreclr/jit/valuenum.cpp

+                if (info.compCompHnd->readObject(obj, (uint8_t*)&buffer, size, (int)byteOffset))
+                {
+                    ValueNum vn = vnStore->VNForGenericCon(tree->TypeGet(), buffer);
+                    assert(!vnStore->IsVNObjHandle(vn));
+                    tree->gtVNPair.SetBoth(vn);
+                    return true;


readObject makes getArrayOrStringLength as well as getStringChar redundant.

In general, the special-cased folding for GT_ARR_LENGTH is out-of-sync with this code, which is odd. ARR_LENGTH code tries to deduce the value even out of non-frozen objects.

Good point, I also had that in mind, in theory, more specialized helpers abstract us from knowing exact object layout on JIT side but JIT already does that

In general, the special-cased folding for GT_ARR_LENGTH is out-of-sync with this code, which is odd. ARR_LENGTH code tries to deduce the value even out of non-frozen objects.

I'll think how to remove getArrayOrStringLength as well as getStringChar separately if you don't mind. Currently, getArrayOrStringLength is the only API that can work with object handles which are not frozen.

Added comments

src/coreclr/jit/valuenum.cpp

jkotas · 2023-04-23T14:49:27Z

src/coreclr/vm/jitinterface.cpp

+    OBJECTREF objRef = getObjectFromJitHandle(handle);
+    _ASSERTE(objRef != NULL);
+
+    // Require that the object is immutable, we can relax this later if needed for partially immutable objects


There is nothing wrong with the JIT reading mutable object if it uses the information correctly (e.g. as a guard).

There is nothing wrong with the JIT reading mutable object if it uses the information correctly (e.g. as a guard).

Ah, I should revert the change and move this check back to JIT side since JIT should not fold accesses to mutubale fields in frozen objects on NativeAOT too

@jkotas does VM side look good otherwise (reverted the change)?

The API looks very similar to getReadonlyStaticFieldValue, the difference is in how they specify "this".

Should readObject wrap object references as CORINFO_OBJECT_HANDLE, same as getReadonlyStaticFieldValue?

Can the names be unified (read vs. get) to make it more obvious that these APIs are very similar?

(I understand that you do not need these for current PR. I am ok if you say that these are TODOs.)

Should readObject wrap object references as CORINFO_OBJECT_HANDLE, same as getReadonlyStaticFieldValue?

Not sure I understand this part - isn't already wrapped? the signature is:

bool readObject(CORINFO_OBJECT_HANDLE handle, uint8_t* buffer, int bufferSize, int valueOffset)

Can the names be unified (read vs. get) to make it more obvious that these APIs are very similar?

Yeah I can rename, to, say getObjectData I guess

Or you mean when we take an object and read its GC field? I assume that will never happen now but possible in futute

Or you mean when we take an object and read its GC field? I assume that will never happen now but possible in futute

Yes, this is what I meant.

src/coreclr/jit/valuenum.cpp

EgorBo · 2023-04-23T19:22:17Z

With this PR + if we allocate static readonly objects on FOH (I have a draft) + Dynamic PGO:

(presumably, should work on NativeAOT with a static profile). Profile is needed to devirtualize the delegate call and then the GDV check is folded by this PR

PS: the redundant load seems to be some left-over side effect

jkotas · 2023-04-23T22:05:07Z

src/coreclr/inc/corinfo.h

@@ -3244,6 +3244,13 @@ class ICorDynamicInfo : public ICorStaticInfo
                    bool                    ignoreMovableObjects = true
                    ) = 0;

+    virtual bool getObjectData(


Nit: Both methods return the same thing, but one calls it Value and the other calls Data. Can we use the same name for the thing that the method returns?

Some ideas:

readStaticField + readObject

readStaticFieldData + readObjectData

getStaticFieldContent + getObjectContent

(getReadonlyStaticFieldValue method does not seem to be strictly limited to readonly static fields, so the name can be relaxed).

Decided to go with:

getObjectType -> getObjectContent; getReadonlyStaticFieldValue -> getStaticFieldContent

Put a condition to getObjectContent to give up on types with GC pointers - want to have a real use case to test properly if you don't mind.

… -> getStaticFieldContent

# Conflicts: # src/coreclr/inc/jiteeversionguid.h

EgorBo · 2023-04-26T07:21:59Z

Failures are mono related CI failures: #82637 and #85304

uweigand · 2023-05-09T08:55:41Z

The new ReadOnlySpanGetReferenceAndReadInteger test case added by this PR always fails on the big-endian s390x architecture:


    System.SpanTests.MemoryMarshalTests.ReadOnlySpanGetReferenceAndReadInteger [FAIL]
      Assert.Equal() Failure
      Expected: 6619240
      Actual:   6815845

This seems to be a test case problem, these casts assume little-endian:

            Assert.Equal(6619240, 
                Unsafe.As<byte, int>(ref Unsafe.Add(ref Unsafe.As<char, byte>(
                ref MemoryMarshal.GetReference("hello world 1".AsSpan())), 0)));

            Assert.Equal(7998511687277765888,
                Unsafe.As<byte, long>(ref Unsafe.Add(ref Unsafe.As<char, byte>(
                    ref MemoryMarshal.GetReference("hello world 2".AsSpan())), 1)));

EgorBo · 2023-05-09T11:30:09Z

The new ReadOnlySpanGetReferenceAndReadInteger test case added by this PR always fails on the big-endian s390x architecture:


    System.SpanTests.MemoryMarshalTests.ReadOnlySpanGetReferenceAndReadInteger [FAIL]
      Assert.Equal() Failure
      Expected: 6619240
      Actual:   6815845

This seems to be a test case problem, these casts assume little-endian:

            Assert.Equal(6619240, 
                Unsafe.As<byte, int>(ref Unsafe.Add(ref Unsafe.As<char, byte>(
                ref MemoryMarshal.GetReference("hello world 1".AsSpan())), 0)));

            Assert.Equal(7998511687277765888,
                Unsafe.As<byte, long>(ref Unsafe.Add(ref Unsafe.As<char, byte>(
                    ref MemoryMarshal.GetReference("hello world 2".AsSpan())), 1)));

Oops sorry for breaking that, yes, the test assumes LE, do you want me to file a fix/disable it for BE or you want to do it yourself?

EgorBo · 2023-05-09T11:30:36Z

The new ReadOnlySpanGetReferenceAndReadInteger test case added by this PR always fails on the big-endian s390x architecture:


    System.SpanTests.MemoryMarshalTests.ReadOnlySpanGetReferenceAndReadInteger [FAIL]
      Assert.Equal() Failure
      Expected: 6619240
      Actual:   6815845

This seems to be a test case problem, these casts assume little-endian:

            Assert.Equal(6619240, 
                Unsafe.As<byte, int>(ref Unsafe.Add(ref Unsafe.As<char, byte>(
                ref MemoryMarshal.GetReference("hello world 1".AsSpan())), 0)));

            Assert.Equal(7998511687277765888,
                Unsafe.As<byte, long>(ref Unsafe.Add(ref Unsafe.As<char, byte>(
                    ref MemoryMarshal.GetReference("hello world 2".AsSpan())), 1)));

Oops sorry for breaking that, yes, the test assumes LE, do you want me to file a fix/disable it for BE or you want to do it yourself?

uweigand · 2023-05-09T11:37:48Z

Oops sorry for breaking that, yes, the test assumes LE, do you want me to file a fix/disable it for BE or you want to do it yourself?

If you could fix this, I'd appreciate it - thanks! I think a simple fix could be to just use different expected results depending on BitConverter.IsLittleEndian.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 20, 2023

ghost assigned EgorBo Apr 20, 2023

EgorBo marked this pull request as ready for review April 21, 2023 01:19

EgorBo requested a review from MichalStrehovsky as a code owner April 21, 2023 01:19