JIT: infer ranges from small int type operations

[AndyAyersMS]

Have range check use the ranges for small int types if it has no better
information.

Fixes #21841

[AndyAyersMS]

@erozenfeld PTAL
cc @dotnet/jit-contrib

This fixes the case in #21841 and gets a few other hits in the framework code. It might be worth looking at related cases (say casts) to see if there's more to be had here.

There may be some symbolic way to express the value range for the small types, but I couldn't find suitable defines.

Jit-diffs over fx:

Total bytes of diff: -202 (0.00% of base)
    diff is an improvement.
Top file improvements by size (bytes):
        -102 : System.Private.DataContractSerialization.dasm (-0.01% of base)
         -26 : Newtonsoft.Json.dasm (0.00% of base)
         -19 : System.Net.HttpListener.dasm (-0.01% of base)
         -19 : System.Net.WebSockets.Client.dasm (-0.11% of base)
         -19 : System.Net.WebSockets.dasm (-0.05% of base)
6 total files with size differences (6 improved, 0 regressed), 123 unchanged.
Top method improvements by size (bytes):
         -81 (-15.14% of base) : System.Private.DataContractSerialization.dasm - XmlUTF8NodeWriter:WriteEscapedText(ref,int,int):this (2 methods)
         -21 (-10.19% of base) : System.Private.DataContractSerialization.dasm - XmlUTF8NodeWriter:UnsafeWriteEscapedText(long,int):this
         -19 (-3.28% of base) : System.Net.HttpListener.dasm - WebSocketValidate:ValidateSubprotocol(ref)
         -19 (-3.28% of base) : System.Net.WebSockets.Client.dasm - WebSocketValidate:ValidateSubprotocol(ref)
         -19 (-3.28% of base) : System.Net.WebSockets.dasm - WebSocketValidate:ValidateSubprotocol(ref)
Top method improvements by size (percentage):
         -81 (-15.14% of base) : System.Private.DataContractSerialization.dasm - XmlUTF8NodeWriter:WriteEscapedText(ref,int,int):this (2 methods)
         -21 (-10.19% of base) : System.Private.DataContractSerialization.dasm - XmlUTF8NodeWriter:UnsafeWriteEscapedText(long,int):this
         -11 (-9.09% of base) : Newtonsoft.Json.dasm - JavaScriptUtils:ShouldEscapeJavaScriptString(ref,ref):bool
         -17 (-6.88% of base) : System.Net.Requests.dasm - CommandStream:PostSendCommandProcessing(byref):bool:this
         -19 (-3.28% of base) : System.Net.HttpListener.dasm - WebSocketValidate:ValidateSubprotocol(ref)
8 total methods with size differences (8 improved, 0 regressed), 193432 unchanged.

Search loop in #21841 is now:

G_M32190_IG03:
       4863D0               movsxd   rdx, eax
       48B9E82985840B020000 mov      rcx, 0x20B848529E8
       0FB6140A             movzx    rdx, byte  ptr [rdx+rcx]
       4863D2               movsxd   rdx, edx
       48B9E82885840B020000 mov      rcx, 0x20B848528E8
       803C0A00             cmp      byte  ptr [rdx+rcx], 0
       740C                 je       SHORT G_M32190_IG05
       FFC0                 inc      eax
       83F804               cmp      eax, 4
       7CD5                 jl       SHORT G_M32190_IG03

[mikedn]

I suppose you could use INT8_MIN, INT8_MAX and so on but it's probably a bit of an abuse as those are relevant to C/C++ types and not JIT's IR types.

Or you could "steal" the code from GenIntCastDesc that computes min/max from the type size and signedness:

coreclr/src/jit/codegenlinear.cpp

Lines 1989 to 1991 in ca65764

	const int castNumBits = (castSize * 8) - (castUnsigned ? 0 : 1);
	m_checkSmallIntMax = (1 << castNumBits) - 1;
	m_checkSmallIntMin = (castUnsigned | srcUnsigned) ? 0 : (-m_checkSmallIntMax - 1);

but I don't think it's worth the trouble.

[AndyAyersMS]

Test failures seem unrelated, will rerun a few...

@dotnet-bot retest OSX10.12 x64 Checked Innerloop Build and Test
@dotnet-bot retest Windows_NT arm Cross Checked Innerloop Build and Test

[AndyAyersMS]

OSX now showing "typical" failures in PAL file tests:

Test Result (5 failures / +5)
file_io.GetFileSize.test1
file_io.GetFileSizeEx.test1
file_io.SetFilePointer.test5
file_io.SetFilePointer.test6
file_io.SetFilePointer.test7

will give it one more try....

@dotnet-bot retest OSX10.12 x64 Checked Innerloop Build and Test

Ubuntu arm failure in "dynamo" GC test (linking to #17129)

14:22:16         /ssd/j/workspace/dotnet_coreclr/master/arm_cross_checked_ubuntu_innerloop_tst_prtest/bin/tests/Linux.arm.Checked/Tests/Core_Root/corerun dynamo.exe 1000 40 191919
14:22:16          
14:22:16         Total amount of RandomNode Memory: 
14:22:16         12487500
14:22:16          
14:22:16         Running Finalize Test...
14:22:16          
14:22:16         Deleting Node:
14:22:16         0
14:22:16         Deleting Node:
14:22:16         19
14:22:16         The registered number: 50
14:22:16         The analyzed number: 42
14:22:16         The registered number and analyzed number are not equal
14:22:16         
14:22:16         Test Failed with seed: 191919

@dotnet-bot retest Ubuntu arm Cross Checked Innerloop Build and Test

[erozenfeld]

We have MAX_SHORT_AS_INT and MIN_SHORT_AS_INT consts in jit.h (currently unused). Perhaps we can add constants for ushort, byte, and ubyte there.

[erozenfeld]

LGTM

[GrabYourPitchforks]

@AndyAyersMS, in your code gen you have the following.

       0FB6140A             movzx    rdx, byte  ptr [rdx+rcx]
       4863D2               movsxd   rdx, edx

Is it possible to get the JIT to drop the movsxd instruction? It seems redundant.

[AndyAyersMS]

The jit disassembler is not quite right here -- actual disassembly is

       0FB6140A             movzx    edx, byte  ptr [rdx+rcx]    // dest is edx, not rdx
       4863D2               movsxd   rdx, edx

(encoding would need to be 48 0fb6140a to be movzx rdx ...)

And while it seems like it ought to be simple to fix this, there are various issues...

Early on there are two uses of the load result -- one for the bounds check and one for the array element address, of type int and long respectively. The stack type is int so that's how we initially model the result of the load. One could argue int is a good choice here as the bounds check will happen at least as often as the address computation.

Later during optimization we eliminate the bounds check (which this PR enables) and only the long use in the element address computation remains, so we could in principle try and change the type to match the remaining use (that is, push the "cast to long" onto the load), but there's nothing in the jit to do that. The def and use trees are split and there is some cruft in between. And we don't try to restitch broken trees.

We don't catch this later on after lowering even though def and use trees end up adjacent because the jit does not model the implicit upper half zeroing on x64 for 32 bit register destinations (that is, we don't realize the movzx edx we emit does appropriate extension and the cast is not needed) and the machine-level IR we producing is not all that amenable to peephole opts that might let us catch this sort of thing.

[pentp]

If the JIT did model/track the implicit upper half zeroing on x64, it would allow most sign-extensions to be eliminated from array/span accesses also. I tried to get something working in #21553, but at the moment it looks a bit too complicated for my limited JIT development skill. While replacing movsx with mov works, it doesn't gain much (the temp registers are still allocated).

[jakobbotsch]

Should this be -128 to 127 or am I missing something?

[AndyAyersMS]

Yeah, it should be... thanks for spotting it.

[erozenfeld]

We have GetLowerBoundForIntegralType and GetUpperBoundForIntegralType that could've been used here.