JIT: Optimize simple range checks with uint hack

[EgorBo]

JIT-level optimization for checks in #27462.

I decided to run the optimization twice before and after CSE.
Before CSE run helps to remove bound checks but can't really optimize the constant-range case due to possible CSE candidates (if I check both conditional nodes for IsCSECandidate and don't run it after CSE phase I'll lose ~1000 bytes in the jit-diff).

If I remove the weight == 0 check, the jit-diff becomes -3858 but in theory can decrease performance in some cases, e.g.:

if (i < 10 || i > 100)
    Foo();

if i < 10 is mostly true - it's better to leave this if as is. (Perhaps I should check the first condition for "is likely to be true" instead of zero weight for the target block)
UPD: It seems, C++ compilers don't really care: https://godbolt.org/z/D9YRVQ (add has 1 latency on most CPUs)

Current jit-diff:

With `ends with exception block` condition:
Total bytes of diff: -3215 (-0.01% of base)

Without the condition:
Total bytes of diff: -3858 (-0.01% of base)

https://gist.github.com/EgorBo/f4a21c16e350e9ae3cd9338ef683526a
(NOTE: the diff can be a lot bigger if we remove hundreds of uint hacks already exist in the BCL)

Let me know if you think this opt is a good idea and I'll add tests (or feel free to close if you are ok with the managed-side fix in #27462).

/cc: @jkotas @mikedn @benaadams

UPD: Can be implemented:

1. return i < 10 || i > 100; won't be optimized because it needs something like "expand BBJ_RETURN blocks to BBJ_COND" kind of de-optimization, see Expand BBJ_RETURN blocks with bool conditions #27167
2. In theory can improve more kinds of range checks, e.g. this one with bit-shifts (probably already optimized by mikedn).
3. Since the opt relies on weights, it's a +1 for Unsafe.Assume kind of API 🙂
4. if(index == 0 || index > 1) -> if(index >= 1)
5. if (('A' <= c) && (c <= 'Z'))

[benaadams]

GT_ARR_LENGTH-based:

Does this include string.Length, Span<T>.Length and ReadOnlySpan<T>.Length?

[EgorBo]

@benaadams good question, unfortunately not 🙁 GT_ARR_LENGTH won't be involved in case of spans (string.Length works):

    int Case1(int i, Span<int> array)
    {
        if (i < 0 || i >= array.Length) 
            throw new ArgumentException();

        return array[i];
    }

***** BB02
N004 (  5,  5) [000003] ------------              *  JTRUE     void  
N003 (  3,  3) [000002] J------N----              \--*  LT        int    $2c0
N001 (  1,  1) [000000] ------------                 +--*  LCL_VAR   int    V01 arg1         u:1 $80
N002 (  1,  1) [000001] ------------                 \--*  CNS_INT   int    0 $45

***** BB03
N004 (  5,  5) [000018] ------------              *  JTRUE     void  
N003 (  3,  3) [000017] J------N----              \--*  GE        int    <l:$2c2, c:$2c1>
N001 (  1,  1) [000012] ------------                 +--*  LCL_VAR   int    V01 arg1         u:1 $80
N002 (  1,  1) [000037] ------------                 \--*  LCL_VAR   int    V06 tmp3         u:1 <l:$240, c:$280>

But probably I somehow can figure out that that LCL_VAR int V06 tmp3 is always positive? (Perhaps, RangeCheck::ComputeRange or vnStore->IsVNCompareCheckedBound(b2Cond->GetVN(VNK_Conservative) ?)

[EgorBo]

NOTE: test failures are related, the toSub value obviously is wrong for the constant range test case.

[maryamariyan]

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your coreclr repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>

[GrabYourPitchforks]

Just a heads up that I don't think the code gen optimization listed at the very beginning of this issue is valid. That is, I believe it produces observable side effects.

In the "before" code, if i is negative and array is null, control will flow to the throw new ArgumentException(); line and an ArgumentException will be thrown. In the "after" code, the r8 (array) register is eagerly dereferenced, meaning that if i is negative and array is null a NullReferenceException will instead be thrown.

[mikedn]

That is, I believe it produces observable side effects.

Yes, it does. And that's not the only issue in this PR.

[maryamariyan]

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.