Fix for 15671

[briansull]

If we are fetching an Array Length for an array ref that came from global memory
then for CSE safety we must use the conservative value number for both

[briansull]

@mikedn PTAL

[briansull]

@AndyAyersMS PTAL

[AndyAyersMS]

Looks plausible enough at first glance. Let me think about it a while.

Any data on the CQ/CS impact?

[mikedn]

Make sense I guess, I haven't though about using GTF_GLOB_REF since I'm not very familiar with it. My attempt was something like:

if (tree->OperIs(GT_ARR_LENGTH) && !tree->AsArrLen()->ArrRef()->gtVNPair.BothEqual())
{
    vnlib = tree->GetVN(VNK_Conservative);
}

It will be interesting to see jit-diff results, mine resulted in some 2kbytes improvements (!!!) due to CSE no longer being done in some cases. But of course, some range checks were no longer eliminated unfortunately.

[briansull]

Without the check for GTF_GLOB_REF, I saw a lots of methods with large regressions, with the check, there are a mix of gains and losses, with only small method size changes. This is using the Desktop build for my diffs..

[AndyAyersMS]

@dotnet/dnceng @jashook ARM64 testing seems to be in a bad way

[mikedn]

This is using the Desktop build for my diffs..

The .NET Core build produces this diff:

Total bytes of diff: 555 (0.00% of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         156 : Microsoft.Diagnostics.Tracing.TraceEvent.dasm (0.01% of base)
         125 : Microsoft.CSharp.dasm (0.04% of base)
         112 : System.Private.CoreLib.dasm (0.00% of base)
         101 : System.Data.Common.dasm (0.01% of base)
          32 : System.Collections.NonGeneric.dasm (0.12% of base)
Top file improvements by size (bytes):
         -54 : System.Text.RegularExpressions.dasm (-0.04% of base)
         -41 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
         -25 : System.Linq.Parallel.dasm (0.00% of base)
         -25 : System.Runtime.Serialization.Formatters.dasm (-0.03% of base)
         -22 : System.Linq.Expressions.dasm (0.00% of base)
48 total files with size differences (16 improved, 32 regressed), 82 unchanged.
Top method regessions by size (bytes):
          78 : System.Private.CoreLib.dasm - List`1:EnsureCapacity(int):this (26 methods)
          61 : System.Private.CoreLib.dasm - MultiElementAsyncLocalValueMap:Set(ref,ref):ref:this
          50 : System.Data.Common.dasm - Select:CreateIndex():this
          30 : System.Private.CoreLib.dasm - Dictionary`2:TryInsert(ref,int,ubyte):bool:this (3 methods)
          27 : System.Private.Xml.dasm - XPathNodeInfoAtom:Init(ref,ref,ref,ref,ref,ref,ref,ref,int,int):this
Top method improvements by size (bytes):
         -80 : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,ref,ref,byref):ref:this
         -39 : System.Text.RegularExpressions.dasm - RegexParser:ScanDollar():ref:this
         -33 : System.Private.CoreLib.dasm - Dictionary`2:FindEntry(ref):int:this (11 methods)
         -33 : System.Private.CoreLib.dasm - Dictionary`2:Remove(ref):bool:this (11 methods)
         -29 : System.Data.Common.dasm - Select:BuildLinearExpression():this
500 total methods with size differences (252 improved, 248 regressed), 140316 unchanged.

Example diff that shows a range check no longer being eliminated: https://gist.github.com/mikedn/d1f519b534806c4c29bd35e2bcbdcefb

It's unfortunate but it doesn't look like there are easy solutions to make the fix less conservative.

[briansull]

@AndyAyersMS should I check this in?

[AndyAyersMS]

Yes, looks good.

CQ impact was less than I expected, so that's a pleasant surprise.