Fold nullchecks in Interlocked.*

[EgorBo]

This PR removes redundant nullchecks emitted with Interlocked.* apis since Interlocked load/store itself is an implicit nullcheck (like any other indir). Also, Interlocked implies a potentially contended memory so we should avoid probing it when it's not needed.
Example:

int fld;

void Test() => Interlocked.Increment(ref fld);

diff:

; Method Program:Test():this (FullOpts)
-      cmp      byte  ptr [rcx], cl
       add      rcx, 8
       lock     
       inc      dword ptr [rcx]
       ret      
-; Total bytes of code: 10
+; Total bytes of code: 8

[dotnet-policy-service]

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

[EgorBo]

PTAL @AndyAyersMS @jakobbotsch @dotnet/jit-contrib
We basically remove redundant bound checks like these:

N008 (  7,  7) [000014] -A-XGO-----                         +--*  XADD      int   
N006 (  5,  5) [000023] ---X-O-N---                         |  +--*  COMMA     byref 
N002 (  2,  2) [000019] ---X-O-----                         |  |  +--*  NULLCHECK byte  
N001 (  1,  1) [000018] -----------                         |  |  |  \--*  LCL_VAR   ref    V00 this         u:1
N005 (  3,  3) [000022] -----O-----                         |  |  \--*  ADD       byref 
N003 (  1,  1) [000020] -----------                         |  |     +--*  LCL_VAR   ref    V00 this         u:1 (last use)
N004 (  1,  1) [000021] -----------                         |  |     \--*  CNS_INT   long   8 Fseq[fld]
N007 (  1,  1) [000013] -----------                         |  \--*  CNS_INT   int    1

It's interesting that optFindNullCheckToFold already has in its description a promise to handle COMMA(nullcheck(addr), addr) or COMMA(nullcheck(addr), addr + smallCns)

[jakobbotsch]

Doesn't this transformation effectively undo what fgMorphExpandInstanceField is creating? I'm not sure it's legal because of the possibility of "almost null" byrefs.

[jakobbotsch]

Perhaps it's sufficient to verify that the base is TYP_REF and then allow it if the offset is <= MAX_UNCHECKED_OFFSET_FOR_NULL_OBJECT. That might still get most of diffs if I'm reading them right.

[AndyAyersMS]

Perhaps it's sufficient to verify that the base is TYP_REF and then allow it if the offset is <= MAX_UNCHECKED_OFFSET_FOR_NULL_OBJECT. That might still get most of diffs if I'm reading them right.

There's a call to fgIsBigOffset down at the end of the method, is that not sufficient?

[jakobbotsch]

Ah yes, it should be. I wonder then if it wouldn't be sufficient to just add a gtEffectiveVal() on top of tree->GetIndirOrArrMetaDataAddr()? Isn't the existing code essentially a more general version of the logic added here?

[EgorBo]

Thanks for the suggestion!