Extra bound checks are not eliminated for pinning

[EgorBo]

The following "array pinning" syntax

public static void Test(int[] array)
{
    fixed (int* p = array)
        DoWork(p);
}

is a С# 7(?) syntax sugar for:

public static void Test(int[] array)
{
    if (array != null && array.Length > 0)
    {
        fixed (int* p = &array[0]) // array is always not empty at this point
            DoWork(p);
    }
}

As you can see, there is no way this code can throw an out of bounds exception.

Codegen (master):

; Method Test(System.Int32[])
G_M31592_IG01:
       4883EC28             sub      rsp, 40
       33C0                 xor      rax, rax
       4889442420           mov      qword ptr [rsp+20H], rax
G_M31592_IG02:
       48894C2420           mov      gword ptr [rsp+20H], rcx
       4885C9               test     rcx, rcx
       740B                 je       SHORT G_M31592_IG04
G_M31592_IG03:
       488B4C2420           mov      rcx, gword ptr [rsp+20H]
       83790800             cmp      dword ptr [rcx+8], 0    ;; we already check Length here
       7504                 jne      SHORT G_M31592_IG05
G_M31592_IG04:
       33C9                 xor      rcx, rcx
       EB14                 jmp      SHORT G_M31592_IG06
G_M31592_IG05:
       488B4C2420           mov      rcx, gword ptr [rsp+20H]
       83790800             cmp      dword ptr [rcx+8], 0     ;; redundant
       761A                 jbe      SHORT G_M31592_IG08      ;; redundant
       488B4C2420           mov      rcx, gword ptr [rsp+20H]
       4883C110             add      rcx, 16
G_M31592_IG06:
       E8EB60FFFF           call     EgorClass:DoWork(long)
       33C0                 xor      rax, rax
       4889442420           mov      gword ptr [rsp+20H], rax
G_M31592_IG07:
       4883C428             add      rsp, 40
       C3                   ret      
G_M31592_IG08:   ;; not needed
       E872C0485F           call     CORINFO_HELP_RNGCHKFAIL   ;; redundant
       CC                   int3                               ;; redundant
; Total bytes of code: 79

Unfortunately, Assertion prop doesn't optimize the bound check away.

category:cq
theme:pinning
skill-level:expert
cost:extra-large
impact:medium

[EgorBo]

I suspect this could be fixed in Roslyn: to use (uint)-cast hack for the = array syntax when it checks for array length and don't introduce a new variable for that.
https://sharplab.io/#v2:C4LghgzgtgPgsAKAAIGYAESCMA2NBXAOwjADMBTDAJjQGFEBvRNZjdLXJAFjQBUyJgmABQBLAsADaAXTRgATnLABPAJRMWjBC21oSIgB5kAJmlHiAVGgAOaALyyFylWnU60mt8wC+rtD60svgD0QWgAFmRyFCIQaADuYWDAaACSaGT6VmQAxskkcgD2UGgASgUQADZKBGjABelQInkFcmgABnwCmG26LWhQZL6oGDgY3J3AlGaSMvKKqr4eniIkpnPKaACE9gR4FRVoAGSHaAAMaAA8pnhiwCrrSgB0ADJkBADmwGFqAW5Lniw9IYTNNLDZ7IcHhJTlIfgDtP94f4AciWP4vEA==

[jkotas]

The JIT should be able to figure this out, even without the uint hack. I do not think we would want to teach Roslyn how to do this.

[EgorBo]

Updated the description. Even if I help JIT to convert array.Length != 0 to 0 < (uint)array.Length in morph it doesn't optimize the range check.

[EgorBo]

Example:

    public static void Test1(int[] array)
    {
        fixed (int* p = array)
        {
        }
    }

IR for OptimizeRangeChecks phase:

*************** Starting PHASE Optimize index checks
*************** In OptimizeRangeChecks()
Blocks/trees before phase

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1        [000..005)-> BB03 ( cond )                     i label target 
BB02 [0001]  1       BB01                  0.50     [005..00A)-> BB04 ( cond )                     i idxlen 
BB03 [0002]  2       BB01,BB02             0.50     [00A..00F)-> BB05 (always)                     i label target 
BB04 [0003]  1       BB02                  0.50     [00F..018)                                     i label target idxlen 
BB05 [0004]  2       BB03,BB04             1        [018..01B)        (return)                     i label target 
-----------------------------------------------------------------------------------------------------------------------------------------

------------ BB01 [000..005) -> BB03 (cond), preds={} succs={BB02,BB03}

***** BB01
STMT00000 (IL 0x000...0x002)
N003 (  1,  3) [000003] -A------R---              *  ASG       ref    $80
N002 (  1,  1) [000002] D------N----              +--*  LCL_VAR   ref    V02 loc1         
N001 (  1,  1) [000001] ------------              \--*  LCL_VAR   ref    V00 arg0         u:1 $80

***** BB01
STMT00001 (IL   ???...0x003)
N004 (  5,  5) [000006] ------------              *  JTRUE     void  
N003 (  3,  3) [000005] J------N----              \--*  EQ        int    $100
N001 (  1,  1) [000000] ------------                 +--*  LCL_VAR   ref    V00 arg0         u:1 (last use) $80
N002 (  1,  1) [000004] ------------                 \--*  CNS_INT   ref    null $VN.Null

------------ BB02 [005..00A) -> BB04 (cond), preds={BB01} succs={BB03,BB04}

***** BB02
STMT00005 (IL 0x005...0x008)
N005 (  7,  7) [000019] ---X--------              *  JTRUE     void  
N004 (  5,  5) [000018] J--X---N----              \--*  NE        int    $103
N002 (  3,  3) [000016] ---X--------                 +--*  ARR_LENGTH int    $101
N001 (  1,  1) [000015] ------------                 |  \--*  LCL_VAR   ref    V02 loc1          $140
N003 (  1,  1) [000017] ------------                 \--*  CNS_INT   int    0 $40

------------ BB03 [00A..00F) -> BB05 (always), preds={BB01,BB02} succs={BB05}

------------ BB04 [00F..018), preds={BB02} succs={BB05}

***** BB04
STMT00006 (IL 0x00F...0x017)
N004 (  8, 11) [000035] ---X--------              *  ARR_BOUNDS_CHECK_Rng void   $1c6
N001 (  1,  1) [000021] ------------              +--*  CNS_INT   int    0 $40
N003 (  3,  3) [000034] ---X--------              \--*  ARR_LENGTH int    $104
N002 (  1,  1) [000020] ------------                 \--*  LCL_VAR   ref    V02 loc1          $141

------------ BB05 [018..01B) (return), preds={BB03,BB04} succs={}

***** BB05
STMT00003 (IL 0x018...0x019)
N003 (  1,  3) [000013] -A------R---              *  ASG       ref    $VN.Null
N002 (  1,  1) [000012] D------N----              +--*  LCL_VAR   ref    V02 loc1         
N001 (  1,  1) [000011] ------------              \--*  CNS_INT   ref    null $VN.Null

***** BB05
STMT00004 (IL 0x01A...0x01A)
N001 (  0,  0) [000014] ------------              *  RETURN    void   $200

@AndyAyersMS any idea why ARR_LENGTH (and underlying local) has different VN in BB02 and BB04 ? it should be the same as far as I understand. I guess Roslyn could avoid generating an additional variable for the array.

[AndyAyersMS]

Suspect this happens because pinned locals are untracked, untracked locals can't be in SSA, and VN relies on SSA for locals. Thus VN doesn't know that the two appearances of V02 refer to the same value.

Unwinding this a bit:

• Untracked locals can't be in SSA because some of them might be aliased.
• Pinned locals are untracked because the CLR GC reporting for pinned locals requires that (for gc purposes) they be untracked gc refs (see lvaSortByRefCount).

We could probably unblock optimization by allowing pinned locals to be tracked for SSA purposes but then reported as untracked for GC purposes. Not sure how hard this would be. We'd also need to be careful not to dead store any nulling writes to pinned locals.

[VSadov]

Pinned locals are untracked because the CLR GC reporting for pinned locals requires that (for gc purposes) they be untracked gc refs (see lvaSortByRefCount).

@AndyAyersMS - what part of GC reporting is missing for pinned registers?

• I think GCInfo is expressive enough to record pinned reg slots. At least the non-x86 kind.
• The interface with GC does not even know how the variable is stored. In fact we report some registers on Unix as pinned in rare cases due to pal limitations (see ReportRegisterToGC) - to tolerate double-reporting (double-marking is ok, but double-relocating is not, so pinning makes potential double-relocating nonissue).

There could be some other parts that I could be missing.

lvaSortByRefCount says that on non-x86 it may be ok to enregister a pinning ref. Are there other reasons maybe?

[jkotas]

There should not be any restrictions in non-x86 GC info encoding that get in the way of optimizing pinned locals.

The problem with pinned locals is that their lifetime has to be extended till end of method or till they are explicitly cleared (or assigned some other value that is pinned implicitly). The JIT takes a shortcut to achieve this lifetime extension by marking them as untracked.