Add GT_ARR_LEN to optNonNullAssertionProp_Ind

[EgorBo]

Closes #84248
We were not taking advantage of assert propagation for GT_ARR_LENGTH (and MD_LENGHT).

void Test(int[]? arr)
{
    if (arr != null)
    {
        var _ = arr.Length;
    }
}

; Assembly listing for method Prog:Test(int[]):this (FullOpts)
-      test     rdx, rdx
-      je       SHORT G_M2580_IG04
-      mov      eax, dword ptr [rdx+0x08]
-G_M2580_IG04:
       ret      
-; Total bytes of code 9
+; Total bytes of code 1

[ghost]

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

Issue Details

---

Closes #84248
We were not taking advantage of assert propagation for GT_ARR_LENGTH (and MD_LENGHT).

void Test(int[]? arr)
{
    if (arr != null)
    {
        var _ = arr.Length;
    }
}

; Assembly listing for method Prog:Test(int[]):this (FullOpts)
-      test     rdx, rdx
-      je       SHORT G_M2580_IG04
-      mov      eax, dword ptr [rdx+0x08]
-G_M2580_IG04:
       ret      
-; Total bytes of code 9
+; Total bytes of code 1

Author:	EgorBo
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[SingleAccretion]

I will note a subtle property of this code: it is relying on array metadata only being used for arrays (TYP_REF, aka heap objects). Array metadata indirections contain an implicit offset addition before the dereference - if array metadata nodes were more "general-purpose" (applicable to random pointers), the code would needs to be made aware of it. However, TYP_REF is either null or a valid object, so this is not necessary.

[jakobbotsch]

LGTM. Diffs look a bit mixed, any idea why? Do we end up kicking out useful assertions?
Also seems somewhat costly TP wise.

[EgorBo]

LGTM. Diffs look a bit mixed, any idea why? Do we end up kicking out useful assertions? Also seems somewhat costly TP wise.

Thanks, yeah it sounds like the PR itself doesn't do anything unusual. Some of the diffs are regressions because we enabled more conditions to be folded in optOptimizeBool/ifConvert (and branchless version often bigger). but some look weird, e.g.:

int _i;

internal void Test(long[] items)
{
    items[_i] = 0;
    items[_i] = 0;
}

this regressed to perform two bound checks while previosly it was just one. I am investigating these now..

Also, noticed a problem in optOptimizeBool - it didn't update flags so it could end up with a "Missing flag" assert:

Missing flags on tree [000016]: ----O-----
N007 ( 21, 13) [000016] J-CXG--N---                         *  NE        int
N005 ( 19, 11) [000089] --CXGO-----                         +--*  OR        int
N002 ( 15,  7) [000014] --CXG------                         |  +--*  CALL r2r_ind int    <unknown method> $1c0
N001 (  1,  1) [000013] ----------- arg0 in rcx             |  |  \--*  LCL_VAR   ref    V01 arg1         u:1 $81
N004 (  3,  3) [000021] -----O-----                         |  \--*  ARR_LENGTH int    $102
N003 (  1,  1) [000020] -----------                         |     \--*  LCL_VAR   ref    V01 arg1         u:1 $81
N006 (  1,  1) [000015] -----------                         \--*  CNS_INT   int    0 $40
ISSUE: <ASSERT> #45080 C:\prj\runtime\src\coreclr\jit\fgdiagnostic.cpp (3385) - Assertion failed '!"Missing flags on tree"' in 'System.Xml.Xsl.XsltOld.SortAction:ParseLang(System.String):System.String:this' during 'Optimize bools' (IL size 72; hash 0xb41e1622; FullOpts)

fixed by gtUpdateStmtSideEffects

[EgorBo]

So for

void Test(int i, long[] items)
{
    items[i] = 0;
    items[i] = 0;
}

with this PR we regress by emitting two bound checks instead of one, it seems to be happening because the 2nd bound check now has a differen VN (same VN as the first one but without NullExc set), JITDUMP diff: https://www.diffchecker.com/CJGsabh8/

Quoting @jakobbotsch:

I also have a note about this issue in CSE somewhere, essentially there is an asymmetry in exception sets between uses/defs that CSE does not manage to make good use of. Essentially we should allow CSE'ing uses with defs if the defs promise a superset of exceptions

So as the actual change seems to be harmless and useful, it unintentionally hits problems we want to fix first before landing it, will re-open it if we fix them.