JIT: Skip more covariance checks

[EgorBo]

Closes #107080
Closes #96787

void Test(List<string> l)
{
    l[0] = "abc";
}

Was:

; Assembly listing for method Test
       push     rbx
       sub      rsp, 32
       mov      rbx, rdx
       cmp      dword ptr [rbx+0x10], 0
       je       SHORT G_M54492_IG04
       mov      rcx, gword ptr [rbx+0x08]
       xor      edx, edx
       mov      r8, 0x1FE6C3654A0      ; 'abc'
       call     CORINFO_HELP_ARRADDR_ST
       inc      dword ptr [rbx+0x14]
       add      rsp, 32
       pop      rbx
       ret      
G_M54492_IG04:
       call     [System.ThrowHelper:ThrowArgumentOutOfRange_IndexMustBeLessException()]
       int3     
; Total bytes of code 51

Now:

; Assembly listing for method Test
       sub      rsp, 40
       cmp      dword ptr [rdx+0x10], 0
       je       SHORT G_M54492_IG04
       mov      rax, gword ptr [rdx+0x08]
       mov      ecx, dword ptr [rax+0x08]
       test     rcx, rcx
       je       SHORT G_M54492_IG05
       mov      rcx, 0x137EF1554A0      ; 'abc'
       mov      gword ptr [rax+0x10], rcx
       inc      dword ptr [rdx+0x14]
       add      rsp, 40
       ret      
G_M54492_IG04:
       call     [System.ThrowHelper:ThrowArgumentOutOfRange_IndexMustBeLessException()]
       int3     
G_M54492_IG05:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
; Total bytes of code 57

So we can either use a faster write barrier helper, or, like in this case, omit it at all.

Diffs - I was expecting more size regressions since we replace a call with a call + explicit bound checks, but, apparently, some opts kick in.

[EgorBo]

@MihuBot

[EgorBo]

@EgorBot -arm64 -intel

using BenchmarkDotNet.Attributes;
using System.Collections.Generic;

public class Bench
{
    List<string> _list = new() { null };

    [Benchmark]
    public void Test() => _list[0] = "abc";
}

[EgorBot]

Benchmark results on Intel

BenchmarkDotNet v0.14.0, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 16 logical and 8 physical cores
  Job-SCTDRH : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  Job-TVIPIS : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Toolchain	Mean	Error	Ratio
Test	Main	3.1684 ns	0.0069 ns	1.00
Test	PR	0.2909 ns	0.0056 ns	0.09

BDN_Artifacts.zip

[EgorBot]

Benchmark results on Arm64

BenchmarkDotNet v0.14.0, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Unknown processor
  Job-OPFJED : .NET 10.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
  Job-PWDKJF : .NET 10.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD

Method	Toolchain	Mean	Error	Ratio
Test	Main	5.684 ns	0.0038 ns	1.00
Test	PR	1.361 ns	0.0023 ns	0.24

BDN_Artifacts.zip

[EgorBo]

@MihuBot

[EgorBo]

No changes here, it's just moved from importer.cpp and renamed.

[EgorBo]

@MihuBot

[EgorBo]

Ugh, isMoreSpecific is not working as I expected (e.g. it says that IFoo is more specific then FooImpl<_Canon>). While getFieldType requires exact owner (for validation purposes mainly). Looks like I need to modify JIT-EE 🙁

[AndyAyersMS]

Ugh, isMoreSpecific is not working as I expected (e.g. it says that IFoo is more specific then FooImpl<_Canon>). While getFieldType requires exact owner (for validation purposes mainly). Looks like I need to modify JIT-EE 🙁

I think you could update isMoreSpecific to say that a class type is more specific than an interface type, as long as the class is not _Canon or Object.

[EgorBo]

I think you could update isMoreSpecific to say that a class type is more specific than an interface type, as long as the class is not _Canon or Object.

I think it's still not enough sadly, say we have:

class BaseClass
class SubClass<T> : BaseClass

It is still going to return BaseClass as more specific than SubClass<_Canon> (although, it passes your as long as the class is not _Canon or Object. suggestion). isMoreSpecific needs to also check class hierarchy I guess.

I was afraid of touching it, so I ended up just validating that the hint class actually has the field or not.

[EgorBo]

@EgorBot -intel -arm64

using BenchmarkDotNet.Attributes;
using System.Collections.Generic;

public sealed class SealedClass {}

public class Bench
{
    SealedClass _bench = new();
    List<SealedClass> _list = new() { null };

    [Benchmark]
    public void Test() => _list[0] = _bench;
}

[EgorBot]

Benchmark results on Intel

BenchmarkDotNet v0.14.0, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 16 logical and 8 physical cores
  Job-WZVEOL : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  Job-WNDBFK : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI

Method	Toolchain	Mean	Error	Ratio
Test	Main	4.476 ns	0.0071 ns	1.00
Test	PR	2.835 ns	0.0225 ns	0.63

BDN_Artifacts.zip

[EgorBot]

Benchmark results on Arm64

BenchmarkDotNet v0.14.0, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Unknown processor
  Job-WKEUVO : .NET 10.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD
  Job-OAPBMN : .NET 10.0.0 (42.42.42.42424), Arm64 RyuJIT AdvSIMD

Method	Toolchain	Mean	Error	Ratio
Test	Main	5.335 ns	0.0024 ns	1.00
Test	PR	3.627 ns	0.0009 ns	0.68

BDN_Artifacts.zip

[jkotas]

The contract for IsMoreSpecific is vague: it is trying to inform the JIT about which type might lead to more specific answers to other queries.

Right. A better name for this method would be isLikelyMoreSpecificType. It is not always possible to decide which one of two types is more specific. The JIT should produce correct code even if this method returns a random number.

@jkotas just checked that change only - only 10 diffs across all --frameworks libs and it seems that all are size improvements

Thanks for checking. In case there are any cases that end up producing worse code, they must be very rare.

The VM change LGTM.

[EgorBo]

@jkotas does it look better now? It seems that R2R/NAOT don't use "fieldOwner" arg at all and manage to successfully generate expected code for my code example as is 😐

[jkotas]

It seems that R2R/NAOT don't use "fieldOwner" arg at all and manage to successfully generate expected code for my code example as is

Are you able to construct a test case for the situation where we need to find a matching parent of the hint? Does this case work in AOT compilers too?

[EgorBo]

It seems that R2R/NAOT don't use "fieldOwner" arg at all and manage to successfully generate expected code for my code example as is

Are you able to construct a test case for the situation where we need to find a matching parent of the hint? Does this case work in AOT compilers too?

I was not, it seems that the difference is that in case of CoreCLR, we first import System.Collections.Generic.List'1[System.__Canon]:set_Item(int,System.__Canon):this inlinee as fully generic, insert it and then gtGetClassHandle tries to match the actual field type from context (object), while in R2R/NAOT when import IR for the generic set_Item callee we already propagate the actual generic context, so the callee IR looks like this:

Importing BB03 (PC=014) of 'System.Collections.Generic.List`1[System.__Canon]:set_Item(int,System.__Canon):this'
    [ 0]  14 (0x00e) ldarg.0
    [ 1]  15 (0x00f) ldfld 0A001EF4
    [ 1]  20 (0x014) ldarg.1
    [ 2]  21 (0x015) ldarg.2
    [ 3]  22 (0x016) stelem 1B000038
Querying runtime about current class of field System.Collections.Generic.List`1[System.String]:_items (declared as System.String[])
Field's current class not available

stelem to T[] with T exact: skipping covariant store check


STMT00003 ( 0x00E[E-] ... ??? ) <- INLRT @ 0x000[E-]
               [000016] nA-XG------                         *  STOREIND  ref   
               [000015] ---XG------                         +--*  INDEX_ADDR byref ref[]
               [000012] n--XG------                         |  +--*  IND       ref   
               [000011] ---X-------                         |  |  \--*  FIELD_ADDR byref  System.Collections.Generic.List`1[System.String]:_items
               [000010] -----------                         |  |     \--*  LCL_VAR   ref    V01 arg1         
               [000013] -----------                         |  \--*  CNS_INT   int    0
               [000014] -----------                         \--*  CNS_STR   ref   <string constant>  

Not sure why that doesn't happen for CoreCLR:

Importing BB03 (PC=014) of 'System.Collections.Generic.List`1[System.__Canon]:set_Item(int,System.__Canon):this'
    [ 0]  14 (0x00e) ldarg.0
    [ 1]  15 (0x00f) ldfld 0A001EF4
    [ 1]  20 (0x014) ldarg.1
    [ 2]  21 (0x015) ldarg.2
    [ 3]  22 (0x016) stelem 1B000038
Querying runtime about current class of field System.Collections.Generic.List`1[System.__Canon]:_items (declared as System.__Canon[])
Field's current class not available


STMT00003 ( 0x00E[E-] ... ??? ) <- INLRT @ 0x000[E-]
               [000015] --CXG------                         *  CALL help void   CORINFO_HELP_ARRADDR_ST
               [000012] n--XG------ arg0                    +--*  IND       ref   
               [000011] ---X-------                         |  \--*  FIELD_ADDR byref  System.Collections.Generic.List`1[System.__Canon]:_items
               [000010] -----------                         |     \--*  LCL_VAR   ref    V01 arg1         
               [000013] ----------- arg1                    +--*  CNS_INT   long   0
               [000014] ----------- arg2                    \--*  CNS_STR   ref   <string constant>

[jkotas]

LGTM. Thanks!