[WIP] Ignore GCness when attempting to reuse constants

[mikedn]

While attempting to minimize GT_BLK use (#27053) I've ran into a little CQ regression issue: INITOBJ is sometimes used to zero out both structs and primitive/reference types and the importer uses GT_BLK regardless. I cannot make it use GT_OBJ because ClassLayout is meant to work with structs or with stack allocated reference types, not with actual references. And anyway it doesn't make a lot of sense to generate a block op in the case of primitive/reference types.

The problem is that converting it to STIND.REF(LDNULL) results in code size regressions due to LSRA restrictions on the type of reused constants - they need to have the same GCness. The block version uses a TYP_INT 0 while the non-block version uses TYP_REF 0.

However, I don't see any reason for the GCness of the constant type to matter because null references do not need to be reported to the GC. More generally, no constant needs to be reported to the GC because it cannot ever by a valid GC heap reference.

Removing the restriction also turns out to be a CQ improvement though I wasn't specifically looking for that.

[mikedn]

x64 diff:

Total bytes of diff: -7503 (-0.03% of base)
    diff is an improvement.
Top file improvements by size (bytes):
       -1556 : System.Private.Xml.dasm (-0.05% of base)
       -1136 : System.Private.CoreLib.dasm (-0.04% of base)
        -965 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.04% of base)
        -699 : System.Drawing.Primitives.dasm (-1.96% of base)
        -576 : Microsoft.CodeAnalysis.CSharp.dasm (-0.03% of base)
75 total files with size differences (75 improved, 0 regressed), 54 unchanged.
Top method regressions by size (bytes):
           1 ( 0.11% of base) : System.Net.HttpListener.dasm - CookieParser:GetServer():ref:this
           1 ( 0.44% of base) : System.Private.CoreLib.dasm - AssemblyLoadContext:InternalLoad(struct,struct):ref:this
Top method improvements by size (bytes):
         -89 (-4.20% of base) : System.Drawing.Primitives.dasm - ColorTranslator:FromOle(int):struct
         -80 (-2.15% of base) : System.Drawing.Primitives.dasm - ColorTranslator:InitializeHtmlSysColorTable()
         -48 (-2.06% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - OverloadResolution:ResolveUserDefinedBinaryOperator(ref,ref,int,ref,byref,bool):struct
         -42 (-0.04% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ApplicationServerTraceEventParser:EnumerateTemplates(ref,ref):this
         -30 (-2.09% of base) : Microsoft.DotNet.ProjectModel.dasm - ProjectFilesCollection:EnsureInitialized():this
Top method regressions by size (percentage):
           1 ( 0.44% of base) : System.Private.CoreLib.dasm - AssemblyLoadContext:InternalLoad(struct,struct):ref:this
           1 ( 0.11% of base) : System.Net.HttpListener.dasm - CookieParser:GetServer():ref:this
Top method improvements by size (percentage):
          -2 (-25.00% of base) : Newtonsoft.Json.dasm - DynamicProxy`1:TryConvert(ref,ref,byref):bool:this
          -2 (-25.00% of base) : Newtonsoft.Json.dasm - DynamicProxy`1:TryGetMember(ref,ref,byref):bool:this
          -2 (-25.00% of base) : Newtonsoft.Json.dasm - DynamicProxy`1:TryUnaryOperation(ref,ref,byref):bool:this
          -2 (-25.00% of base) : System.Linq.Expressions.dasm - DynamicObject:TryGetMember(ref,byref):bool:this
          -2 (-25.00% of base) : System.Linq.Expressions.dasm - DynamicObject:TryInvokeMember(ref,ref,byref):bool:this
2198 total methods with size differences (2196 improved, 2 regressed), 147132 unchanged.

x86 diff:

Total bytes of diff: -5856 (-0.03% of base)
    diff is an improvement.
Top file improvements by size (bytes):
        -772 : System.Private.CoreLib.dasm (-0.03% of base)
        -702 : System.Private.Xml.dasm (-0.03% of base)
        -479 : System.Drawing.Primitives.dasm (-1.48% of base)
        -380 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.02% of base)
        -324 : System.Net.Http.dasm (-0.08% of base)
79 total files with size differences (79 improved, 0 regressed), 50 unchanged.
Top method improvements by size (bytes):
         -88 (-1.15% of base) : NuGet.Frameworks.dasm - DefaultPortableFrameworkMappings:get_ProfileFrameworks():ref:this
         -63 (-3.52% of base) : System.Drawing.Primitives.dasm - ColorTranslator:FromOle(int):struct
         -56 (-0.18% of base) : Microsoft.CodeAnalysis.dasm - DesktopAssemblyIdentityComparer:.cctor()
         -54 (-2.10% of base) : System.Drawing.Primitives.dasm - ColorTranslator:InitializeHtmlSysColorTable()
         -40 (-0.64% of base) : System.Private.CoreLib.dasm - AsyncStateMachineBox`1:MoveNext(ref):this (23 methods)
Top method improvements by size (percentage):
          -2 (-28.57% of base) : System.Net.Primitives.dasm - CredentialEnumerator:MoveNext(byref):bool:this
          -2 (-28.57% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:TryGetLocalNameAsDictionaryString(byref):bool:this
          -2 (-28.57% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:TryGetNamespaceUriAsDictionaryString(byref):bool:this
          -2 (-28.57% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:TryGetValueAsDictionaryString(byref):bool:this
          -2 (-28.57% of base) : System.Private.DataContractSerialization.dasm - XmlDictionaryReader:IsStartArray(byref):bool:this
1969 total methods with size differences (1969 improved, 0 regressed), 147408 unchanged.

arm64 diff:

Total bytes of diff: -6544 (-0.01% of base)
    diff is an improvement.
Top file improvements by size (bytes):
       -1888 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.01% of base)
        -904 : System.Private.CoreLib.dasm (-0.01% of base)
        -632 : Microsoft.CodeAnalysis.CSharp.dasm (-0.00% of base)
        -456 : System.Net.Http.dasm (-0.02% of base)
        -376 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (-0.00% of base)
50 total files with size differences (50 improved, 0 regressed), 79 unchanged.
Top method improvements by size (bytes):
         -96 (-2.42% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - VisualBasicCompilation:Create(ref,ref,ref,ref,ref,ref,ref,bool):ref (4 methods)
         -96 (-1.42% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:BindInvocationExpressionPossiblyWithoutArguments(ref,int,ref,struct,struct,struct,bool,ref):ref:this (4 methods)
         -96 (-1.81% of base) : System.Net.Http.dasm - HttpWindowsProxy:GetMultiProxy(ref):struct:this (2 methods)
         -88 (-2.59% of base) : System.Private.CoreLib.dasm - RuntimeType:GetMember(ref,int,int):ref:this (2 methods)
         -80 (-0.97% of base) : System.Net.Security.dasm - SecureChannel:GenerateToken(ref,int,int,byref):struct:this (4 methods)
Top method improvements by size (percentage):
          -8 (-5.88% of base) : System.Private.CoreLib.dasm - NullStream:ReadAsync(struct,struct):struct:this (2 methods)
          -8 (-5.56% of base) : System.Runtime.Numerics.dasm - BigInteger:ToByteArray():ref:this (2 methods)
          -8 (-5.26% of base) : System.Private.DataContractSerialization.dasm - DataContractJsonSerializerImpl:.ctor(ref,ref,ref):this (2 methods)
          -8 (-5.26% of base) : System.Runtime.Numerics.dasm - BigInteger:GetByteCount(bool):int:this (2 methods)
         -16 (-4.88% of base) : System.Net.Http.dasm - MultiProxy:get_Empty():struct (2 methods)
448 total methods with size differences (448 improved, 0 regressed), 148591 unchanged.

arm32 diff:

Total bytes of diff: -23660 (-0.03% of base)
    diff is an improvement.
Top file improvements by size (bytes):
       -4840 : Microsoft.CodeAnalysis.VisualBasic.dasm (-0.05% of base)
       -3428 : System.Private.Xml.dasm (-0.05% of base)
       -2740 : Microsoft.CodeAnalysis.CSharp.dasm (-0.03% of base)
       -2332 : System.Private.CoreLib.dasm (-0.03% of base)
        -920 : Microsoft.CodeAnalysis.dasm (-0.03% of base)
82 total files with size differences (82 improved, 0 regressed), 47 unchanged.
Top method improvements by size (bytes):
        -176 (-1.36% of base) : NuGet.Frameworks.dasm - DefaultPortableFrameworkMappings:get_ProfileFrameworks():ref:this (2 methods)
        -172 (-0.06% of base) : Microsoft.Diagnostics.Tracing.TraceEvent.dasm - ApplicationServerTraceEventParser:EnumerateTemplates(ref,ref):this (2 methods)
        -168 (-2.42% of base) : System.Private.Xml.dasm - XmlDocument:.cctor() (4 methods)
        -128 (-1.45% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - OverloadResolution:ResolveUserDefinedBinaryOperator(ref,ref,int,ref,byref,bool):struct (4 methods)
        -112 (-0.59% of base) : Microsoft.CodeAnalysis.VisualBasic.dasm - SynthesizedEventAccessorSymbol:ConstructFieldLikeEventAccessorBody_Regular(ref,bool,ref,ref):ref (4 methods)
Top method improvements by size (percentage):
          -8 (-22.22% of base) : System.Memory.dasm - ReadOnlySequence`1:TryGetString(byref,byref,byref):bool:this (2 methods)
          -8 (-20.00% of base) : Newtonsoft.Json.dasm - DynamicProxy`1:TryConvert(ref,ref,byref):bool:this (4 methods)
          -8 (-20.00% of base) : Newtonsoft.Json.dasm - DynamicProxy`1:TryGetMember(ref,ref,byref):bool:this (4 methods)
          -8 (-20.00% of base) : Newtonsoft.Json.dasm - DynamicProxy`1:TryUnaryOperation(ref,ref,byref):bool:this (4 methods)
          -8 (-20.00% of base) : System.Linq.dasm - EmptyPartition`1:TryGetElementAt(int,byref):ref:this (4 methods)
2880 total methods with size differences (2880 improved, 0 regressed), 146862 unchanged.

[mikedn]

@CarolEidt Any concerns about this change?

[CarolEidt]

As I recall (and my recollection is somewhat faint, as this was a while ago), the original register allocation changes to reuse constants didn't distinguish the GC types, but it had to be added due to some regressions. I'll try to dig into that to see if I can figure that out.

[mikedn]

Thanks, I was afraid that you may say something like that :(. I initially tried to follow through git blame but got lost. Tried again and finally found the change in a dubious merge commit from 2015: 55d7daf

Unfortunately the original commit that contained the change seems to have been lost somehow so there's no information about why the change was made.

Oh well, that's unfortunate. I may need to keep the INITOBJ BLK for now to avoid CQ regressions.

[mikedn]

Actually the original commit is this: 93e7d20

Came from TFS...

[sandreenko]

The comment for the change that added the second check in TFS was:

(emitThisGCrefRegs & regMask) == 0' assert failure.

The problem is that we were reusing a constant zero that was a TYP_REF.
The fix is to not reuse a register if it is not the same GC type.

[mikedn]

Ah, an assert, that doesn't necessarily mean that this change won't work. I also changed codegen to never emit a GC constant 0 so that assert should never be hit. Though I may need to do the same thing for non-0 byrefs, I'll check.

Thanks for the info!

[CarolEidt]

Yes, when @sandreenko mentioned this to me, I suspected that the assert may have been overly aggressive. Given the changes to codegen, it would seem that this is a safe change.

[maryamariyan]

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your coreclr repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

[mikedn]

I'm still looking into potential issues with GC reporting so this will have to wait until after repo consolidation. While it's obvious that there's no need to report constants to GC, the way GC tracking is done is a bit confusing.

For example, the changes I've done in codegen do result in the emitter no longer reporting the constant register to the GC. But there's also genProduceReg, that calls gcMarkRegPtrVal with the constant node's type which may be TYP_REF/TYP_BYREF. So it seems that the duplicate GC tracking could potentially get out of sync somehow.

Perhaps it's better to change all TYP_REF/TYP_BYREF constant nodes to TYP_I_IMPL in lowering (can't be done before because AFAIR VN doesn't like some type mismatches in this area).