[RyuJIT] Unnecessary nullcheck in Exchange_long benchmark

[EgorBo]

dotnet/performance/Perf.Interlocked.cs#L39:

private long _long = 0;

[MethodImpl(MethodImplOptions.NoInlining)]
public long Exchange_long() => Interlocked.Exchange(ref _long, 1);

Codegen for Exchange_long:

       cmp      dword ptr [rcx], ecx ;; <--- nullcheck
       lea      rax, bword ptr [rcx+8]
       mov      edx, 1
       xchg     qword ptr [rax], rdx
       mov      rax, rdx
       ret

this code never throws NRE.
Is it #define CONSERVATIVE_NULL_CHECK_BYREF_CREATION 1's fault?

/cc @dotnet/jit-contrib @sandreenko

UPD: Same in MONO:

   0:   50                      push   %rax
   1:   48 85 ff                test   %rdi,%rdi  ;; <----
   4:   74 0b                   je     11 <xchange_long___+0x11>
   6:   b8 01 00 00 00          mov    $0x1,%eax
   b:   48 87 47 10             xchg   %rax,0x10(%rdi)
   f:   59                      pop    %rcx
  10:   c3                      retq
  11:   48 b8 60 e9 74 54 13    movabs $0x56135474e960,%rax
  18:   56 00 00
  1b:   bf 32 01 00 00          mov    $0x132,%edi
  20:   ff 10                   callq  *(%rax)

category:cq
theme:optimization
skill-level:beginner
cost:small
impact:small

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[EgorBo]

Ah, it's an instance method

[erozenfeld]

@EgorBo I think we can teach optFoldNullCheck to eliminate this null check. What's the IR before early prop?

[EgorBo]

@EgorBo I think we can teach optFoldNullCheck to eliminate this null check. What's the IR before early prop?

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1        [000..00E)        (return)                     i label target nullcheck 
-----------------------------------------------------------------------------------------------------------------------------------------

------------ BB01 [000..00E) (return), preds={} succs={}

***** BB01
STMT00000 (IL 0x000...0x00D)
N009 (  8,  8) [000006] -A-XG-------              *  RETURN    long  
N008 (  7,  7) [000005] -A-XG-------              \--*  XCHG      long  
N006 (  5,  5) [000012] ---XG--N----                 +--*  COMMA     byref 
N002 (  2,  2) [000008] ---X---N----                 |  +--*  NULLCHECK byte  
N001 (  1,  1) [000007] ------------                 |  |  \--*  LCL_VAR   ref    V00 this         u:1
N005 (  3,  3) [000011] ------------                 |  \--*  ADD       byref 
N003 (  1,  1) [000009] ------------                 |     +--*  LCL_VAR   ref    V00 this         u:1 (last use)
N004 (  1,  1) [000010] ------------                 |     \--*  CNS_INT   long   8 field offset Fseq[_long]
N007 (  1,  1) [000004] ------------                 \--*  CNS_INT   long   1

[erozenfeld]

Yes, this should be easy to handle. optEarlyPropRewriteTree should call optFoldNullCheck when it sees GT_XCHG (in addition to indirections and GT_ARR_LENGTH) and optFindNullCheckToFold needs to be modified to handle GT_EXCHG.

[sandreenko]

Another implicit indir is INDEX_ADDR that we do as:

                                                              /--*  t9     ref    
                                                              +--*  t6     int    
Generating: N040 (  5,  4) [000025] ---XG-------        t25 = *  INDEX_ADDR byref  REG rax
							GC regs: 00000001 {rax} => 00000000 {}
							GC regs: 00000000 {} => 00000001 {rax}
IN0009:        cmp      edx, dword ptr [rax+8]

and if t9 == nullptr we correctly generate a nullptr exception but currently we don't use it to delete nullchecks.

[JulieLeeMSFT]

Moving to .NET 8.