Optimization for full range checks (#70145)

[SkiFoD]

fixes #70145
I also added code for comparison with int.MinValue/long.MinValue in case of int/long.

[ghost]

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

Issue Details

---

I also added code for comparison with int.MinValue/long.MinValue in case of int/long.

Author:	SkiFoD
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[SkiFoD]

@jakobbotsch Hey, could you please become my reviewer?

[danmoseley]

If this fixes #70145 you can put "fixes #70145" in the top comment to ensure it gets closed

[SkiFoD]

Trying to presave the side effects gave me many regressions, so I decided to cut it out for now.

[jakobbotsch]

It doesn't seem right that this can only fold things into false. What about when the comparison is always true?

[SkiFoD]

I can't think of a case when the comparison is always true. There are 7 typical trees which I'm testing on (value types may vary but the trees are always generalized to the 7 cases):

1. When const is on the right and MinValue
   example v >= int.MinValue:

    *  RETURN    int
    \--*  EQ        int
       +--*  LT        int
       |  +--*  LCL_VAR   int    V00 arg0
       |  \--*  CNS_INT   int    -0x80000000
       \--*  CNS_INT   int    0

2. When const is on the left and MinValue
   example int.MinValue <= v

   *  RETURN    int
   \--*  EQ        int
      +--*  GT        int
      |  +--*  CNS_INT   int    -0x80000000
      |  \--*  LCL_VAR   int    V00 arg0

      \--*  CNS_INT   int    0

3. When const is on the right and MaxValue
   example v <= int.MaxValue

 *  RETURN    int
 \--*  EQ        int
    +--*  GT        int
    |  +--*  LCL_VAR   int    V00 arg0
    |  \--*  CNS_INT   int    0x7FFFFFFF
    \--*  CNS_INT   int    0

4. When const is on the left and MaxValue
   example int.MaxValue >= v

 *  RETURN    int
 \--*  EQ        int
    +--*  LT        int
    |  +--*  CNS_INT   int    0x7FFFFFFF
    |  \--*  LCL_VAR   int    V00 arg0
    \--*  CNS_INT   int    0

5. When const is ulong/uint and value is MinValue
   example v >= ulong.MinValue
   example ulong.MinValue <= v

*  RETURN    int
\--*  CNS_INT   int    1

6. When const is on the right and value type is ulong/uint and value is MaxValue
   example v <= ulong.MaxValue;

 \--*  EQ        int
    +--*  GT        int
    |  +--*  LCL_VAR   long   V00 arg0
    |  \--*  CNS_INT   long   -1
    \--*  CNS_INT   int    0

7. When const is on the left and value type is ulong/uint and value is MaxValue
   example ulong.MaxValue >= v

*  RETURN    int
\--*  EQ        int
   +--*  LT        int
   |  +--*  CNS_INT   long   -1
   |  \--*  LCL_VAR   long   V00 arg0
   \--*  CNS_INT   int    0

[SkiFoD]

So for example:
v >= int.MinValue generates a tree that is equal to return (v < int.MinValue) == false
int.MinValue <= v generates a tree that is equal to return (int.MinValue > v) == false
However in case of ulong/uint:
v <= ulong.MaxValue generates a tree that is equal to return (v > -1) == false ulong.MaxValue >= vgenerates a tree that is equal toreturn (-1 < v) == false
These trees look odd to me because (v > -1) is going to be always true.
Correct me please if I'm wrong.

[jakobbotsch]

One example would be:
bool Foo(sbyte i) => i > -129;

[jakobbotsch]

We generally do not see x >= y much here because there is no such IL instruction, so that's why the more "common" pattern i >= sbyte.MinValue is reversed by Roslyn. But we can still see such IR if we introduce it ourselves, so I think it makes sense to handle it (and also the above case).

[SkiFoD]

Do you know how to generate the always true condition but with GT_LE? I'm considering should we be bothered with such a case at all.

[jakobbotsch]

Given that you already have the intervals I think the actual check itself is so simple that there is no reason not to add it, it should not be more than a couple of lines.
[x0, x1] <= [y0, y1] is always true if x1 <= y0.

[SkiFoD]

I tried to apply this condition, which considering only LT for simplicity and it gave me 1700+ improvements during the spmi asmdiff run, which is suspicious :)

if (((op == GT_LT) && (lhsMin >= rhsMax)) || (((op == GT_LE) && (lhsMin > rhsMax))))
    {
        ret = gtNewZeroConNode(TYP_INT);
    }
    else if ((op == GT_LT) && rhsMax > lhsMax)
    {
        ret = gtNewOneConNode(TYP_INT);
    }

Then I tried to check which Op is const to make the condition more strict:

    //When lhs is constant
    *  RETURN    int
    \--*  LT       int
         +--*  CNS_INT   int    -129
          \--*  LCL_VAR   byte   V00 arg0
    else if ((op == GT_LT) && rhsMin > lhsMin && lhsMin == lhsMax)
    {
        ret = gtNewOneConNode(TYP_INT);
    }
    // When rhs is constant
      For cases like this:
      *  RETURN    int
      \--*  LT        int
          +--*  LCL_VAR   byte   V00 arg0
           \--*  CNS_INT   int    129

    else if ((op == GT_LT) && rhsMax > lhsMax && rhsMin == rhsMax)
    {
        ret = gtNewOneConNode(TYP_INT);
    }

It works fine and there are not so many improvements. What do you think of this?

[jakobbotsch]

[x0, x1] < [y0, y1] is true if x1 < y0. So I think
else if ((op == GT_LT) && rhsMax > lhsMax)
should be
else if ((op == GT_LT) && lhsMax < rhsMin).

[jakobbotsch]

What's an example this catches?

[SkiFoD]

I don't like the part of code, but I haven't come up with an idea how to get ride of the magic number (-1) yet. The idea here is that ulong and uint use this const.
For example: bool Test_M27(ulong v) => v <= ulong.MaxValue generates this tree:

     *  RETURN    int
 \--*  EQ        int
    +--*  GT        int
    |  +--*  LCL_VAR   long   V00 arg0
    |  \--*  CNS_INT   long   -1
    \--*  CNS_INT   int    0

[jakobbotsch]

For unsigned comparisons a signed interval that spans 0 represents two distinct intervals, i.e. for [x, y]:

1. if y < 0: represents [(unsigned)x, (unsigned)y]
2. if x >= 0: represents [x, y]
3. else: represents [0, y] U [(unsigned)x, MaxValue]

So the "generalized" code here would need to check these conditions. I agree this is a bit tricky, so it is fine with me to special case this, although it would be nice to find a way to shape the code such that we avoid the "normal" check below in the special case.

Also, I think the lefOpValue and rightOpValue are a bit confusing, I would just try to stick with lhsMin/Max and rhsMin/Max.

[SkiFoD]

The tricky part here is that when rhs is ulong then
rhsMin = IntegralRange::SymbolicToRealValue(rhsRange.GetLowerBound()); returns -9223372036854775808 instead of 0.
So what if we would use something like this:

 else if (cmp->IsUnsigned() && (op == GT_LT) && rhsMin < 0 && !isLhsConst)
    {
        ret = gtNewZeroConNode(TYP_INT);
    }
    else if (cmp->IsUnsigned() && (op == GT_LT) && lhsMin < 0 && isLhsConst)
    {
        ret = gtNewZeroConNode(TYP_INT);
    }

[SkiFoD]

For unsigned comparisons a signed interval that spans 0 represents two distinct intervals, i.e. for [x, y]:

1. if y < 0: represents [(unsigned)x, (unsigned)y]
2. if x >= 0: represents [x, y]
3. else: represents [0, y] U [(unsigned)x, MaxValue]

So the "generalized" code here would need to check these conditions. I agree this is a bit tricky, so it is fine with me to special case this, although it would be nice to find a way to shape the code such that we avoid the "normal" check below in the special case.

Could you please explain this idea in more details, I'm not sure I can understand how to apply this.

[jakobbotsch]

I guess you would do something like:

if (cmp->IsUnsigned())
{
  if ((lhsMin < 0) && (lhsMax >= 0))
  {
    // [0, (uint64_t)lhsMax] U [(uint64_t)lhsMin, MaxValue]
    lhsMin = 0;
    lhsMax = -1;
  }

  if ((rhsMin < 0) && (rhsMax >= 0))
  {
    // [0, (uint64_t)rhsMax] U [(uint64_t)rhsMin, MaxValue]
    rhsMin = 0;
    rhsMax = -1;
  }
}

int foldValue;
if (cmp->IsUnsigned())
{
  if ((op == GT_LT && ((uint64_t)lhsMax < (uint64_t)rhsMin) ||
      (op == GT_LE && ((uint64_t)lhsMax <= (uint64_t)rhsMin))
  {
    foldValue = 1;
  }
  else if ((op == GT_LT && ((uint64_t)lhsMin >= (uint64_t)rhsMax) ||
           (op == GT_LE && ((uint64_t)lhsMin > (uint64_t)rhsMax))
  {
    foldValue = 0;
  }
  else
  {
    return;
  }
}
else
{
  if ((op == GT_LT && (lhsMax < rhsMin) ||
      (op == GT_LE && (lhsMax <= rhsMin))
  {
    foldValue = 1;
  }
  else if ((op == GT_LT && (lhsMin >= rhsMax) ||
           (op == GT_LE && (lhsMin > rhsMax))
  {
    foldValue = 0;
  }
  else
  {
    return;
  }
}

// fold to foldValue here

But, it's hard to get all the cases right :-) I would probably double check with something like https://github.com/jakobbotsch/Fuzzlyn. I will definitely run that on this PR before merging.

[jakobbotsch]

For example, running Fuzzlyn on your PR in the current shape quickly finds examples. I did:

> .\Fuzzlyn.exe --host C:\dev\dotnet\runtime2\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\corerun.exe --num-programs 10000000 --parallelism 8
Found example with seed 10360326530109389226

> .\Fuzzlyn.exe --host C:\dev\dotnet\runtime2\artifacts\tests\coreclr\windows.x64.Checked\Tests\Core_Root\corerun.exe --reduce --seed 10360326530109389226
Simplifying Coarsely. Total elapsed: 00:00:10. Method 51/51.
Simplifying Statements. Total elapsed: 00:00:15. Iter: 107/107
Simplifying Expressions. Total elapsed: 00:00:16. Iter: 478/478
Simplifying Members. Total elapsed: 00:00:19. Iter: 9/9
Simplifying Statements. Total elapsed: 00:00:19. Iter: 12/12
Simplifying Expressions. Total elapsed: 00:00:19. Iter: 45/45
Simplifying Members. Total elapsed: 00:00:20. Iter: 7/7
Simplifying Statements. Total elapsed: 00:00:20. Iter: 8/8
Simplifying Expressions. Total elapsed: 00:00:20. Iter: 35/35
Simplifying Members. Total elapsed: 00:00:20. Iter: 6/6
Simplifying Statements. Total elapsed: 00:00:20. Iter: 8/8
Simplifying Expressions. Total elapsed: 00:00:20. Iter: 34/34
Simplifying Members. Total elapsed: 00:00:20. Iter: 6/6

which outputs:

// Generated by Fuzzlyn v1.5 on 2022-06-14 19:20:16
// Run on X64 Windows
// Seed: 10360326530109389226
// Reduced from 64.1 KiB to 0.5 KiB in 00:00:23
// Debug:
// Release: Outputs 0
public class Program
{
    public static IRuntime s_rt;
    public static uint s_4;
    public static void Main()
    {
        s_rt = new Runtime();
        bool vr1 = M1(0);
    }

    public static bool M1(short arg0)
    {
        if ((12729537629719743250UL < (uint)arg0))
        {
            s_rt.WriteLine(s_4);
        }

        return true;
    }
}

public interface IRuntime
{
    void WriteLine<T>(T value);
}

public class Runtime : IRuntime
{
    public void WriteLine<T>(T value) => System.Console.WriteLine(value);
}

This program is incorrect with the current PR.

[SkiFoD]

Looks like an amazing tool. I played with it a little bit and got something like this:

// Generated by Fuzzlyn v1.5 on 2022-06-15 09:15:55
// Run on X64 Windows
// Seed: 6538447736931409473
// Reduced from 109.8 KiB to 0.2 KiB in 00:00:53
// Debug: Outputs True
// Release: Outputs False
public class Program
{
    public static long s_33 = 1;
    public static bool s_50;
    public static void Main()
    {
        uint vr0 = (uint)(-s_33);
        s_50 = 4038847739U < vr0;
        System.Console.WriteLine(s_50);
    }
}

Does it mean that if I cut out the code (from main) and build it in Debug then it returns True and if I build it in Release then it returns False?

[jakobbotsch]

Yes, that's what it means (and also tiered compilation has to be disabled).

[jakobbotsch]

FWIW, your treatment of unsigned comparisons is still wrong, you cannot use the signed comparisons for the interval checks in that case. It is probably the reason for this problem. I would suggest you shape the code somewhat like the example I posted earlier.

[jakobbotsch]

Shouldn't the first check be generalized to lhsMin >= rhsMax?
Also, I think this still needs special handling for unsigned comparisons. The easiest is probably to bail for negative intervals except for your special case above. For positive intervals you can use the signed comparisons.

[SkiFoD]

Shouldn't the first check be generalized to lhsMin >= rhsMax?

It may work with GT_LT, but what about GT_LE?
If it is GT_LE then lhsMin>rhsMax and lhsMin>=rhsMax have different results. Right?

[jakobbotsch]

Yes, by "first check" I meant the GT_LT part.

[SkiFoD]

Then it would look like if ((op == GT_LE && lefOpValue > rightOpValue) || lhsMin>=rhsMax)

[jakobbotsch]

Ah, I just meant
((op == GT_LT) && (lhsMin >= rhsMax)) || ((op == GT_LE) && (lhsMin > rhsMax)))

[jakobbotsch]

This looks like it needs to be updated.

[jakobbotsch]

Ditto for the "Always false" in the summary above.

[jakobbotsch]

Suggested change

	ret->SetVNsFromNode(cmp);
	fgUpdateConstTreeValueNumber(ret);

(Given that we folded, this is more precise)

[jakobbotsch]

/azp run Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[SkiFoD]

@jakobbotsch I was testing the code via Fuzzlyn and found 1 seed out of 10000, then I tried to debug the reduced piece of code, but It threw an exception (assertion was false) even though it never went through the new lines of code written by me, which was suspicious. I repeated the same experiment but on the main branch, without any line of my code, and the result was the same. Can Fuzzlyn generate programs that work with errors on current runtime-HEAD? If yes, then is there a rule of thumb how to make sure Fuzzlyn says my code doesn't break anything?

[jakobbotsch]

Looks great to me now, thanks! The Fuzzlyn run has only known failures.

We currently have some issues with the superpmi-diffs pipeline that means we cannot evaluate throughput impact, but I am hoping this will be resolved soon.

[jakobbotsch]

@jakobbotsch I was testing the code via Fuzzlyn and found 1 seed out of 10000, then I tried to debug the reduced piece of code, but It threw an exception (assertion was false) even though it never went through the new lines of code written by me, which was suspicious. I repeated the same experiment but on the main branch, without any line of my code, and the result was the same. Can Fuzzlyn generate programs that work with errors on current runtime-HEAD? If yes, then is there a rule of thumb how to make sure Fuzzlyn says my code doesn't break anything?

What was the assertion error? There are some known failures currently. We don't have a good way to signal what is a "known failure", but you can try searching the repo issues to see if an issue is already open for it.

[SkiFoD]

@jakobbotsch I was testing the code via Fuzzlyn and found 1 seed out of 10000, then I tried to debug the reduced piece of code, but It threw an exception (assertion was false) even though it never went through the new lines of code written by me, which was suspicious. I repeated the same experiment but on the main branch, without any line of my code, and the result was the same. Can Fuzzlyn generate programs that work with errors on current runtime-HEAD? If yes, then is there a rule of thumb how to make sure Fuzzlyn says my code doesn't break anything?

What was the assertion error? There are some known failures currently. We don't have a good way to signal what is a "known failure", but you can try searching the repo issues to see if an issue is already open for it.

// Hits JIT assert in Release:
// Assertion failed 'cookie != nullptr' in 'Program:M48(S0):byref' during 'Emit GC+EH tables' (IL size 221; hash 0x2d403f38; 
FullOpts)

I assume this is a known issue then :)

[SkiFoD]

Looks great to me now, thanks! The Fuzzlyn run has only known failures.

We currently have some issues with the superpmi-diffs pipeline that means we cannot evaluate throughput impact, but I am hoping this will be resolved soon.

I does look great now. Thank you for your guidence. I would never be able to accomplish this without your help, although the issue was marked as easy 👍

[jakobbotsch]

I assume this is a known issue then :)

Yep, that one was part of #69659, fixed in #69897.

[SkiFoD]

Do you want me to merge the main-HEAD branch changes to this one?

[jakobbotsch]

Do you want me to merge the main-HEAD branch changes to this one?

No, that's not necessary -- the CI jobs already do such a merge before running.

[jakobbotsch]

Many nice diffs that look like the following:

 ; Assembly listing for method Microsoft.CodeAnalysis.EnumBounds:IsValid(ubyte):bool
 ; Emitting BLENDED_CODE for X64 CPU with AVX - Windows
 ; optimized code
 ; rsp based frame
 ; partially interruptible
 ; No matching PGO data
 ; Final local variable assignments
 ;
 ;  V00 arg0         [V00,T00] (  3,  3   )   ubyte  ->  rcx         single-def
 ;# V01 OutArgs      [V01    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
-;  V02 cse0         [V02,T01] (  3,  2.50)     int  ->  rax         "CSE - aggressive"
 ;
 ; Lcl frame size = 0

-G_M51999_IG01:        ; gcVars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, gcvars, byref, nogc <-- Prolog IG
+G_M51999_IG01:        ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, nogc <-- Prolog IG
                                                ;; size=0 bbWeight=1    PerfScore 0.00
-G_M51999_IG02:        ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, isz
-       movzx    rax, cl
-       test     eax, eax
-       jl       SHORT G_M51999_IG05
-                                               ;; size=7 bbWeight=1    PerfScore 1.50
-G_M51999_IG03:        ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
-       cmp      eax, 1
-       setle    al
-       movzx    rax, al
-                                               ;; size=9 bbWeight=0.50 PerfScore 0.75
-G_M51999_IG04:        ; , epilog, nogc, extend
-       ret
-                                               ;; size=1 bbWeight=0.50 PerfScore 0.50
-G_M51999_IG05:        ; gcVars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, gcvars, byref
+G_M51999_IG02:        ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
        xor      eax, eax
-                                               ;; size=2 bbWeight=0.50 PerfScore 0.12
-G_M51999_IG06:        ; , epilog, nogc, extend
+       cmp      cl, 1
+       setbe    al
+                                               ;; size=8 bbWeight=1    PerfScore 1.50
+G_M51999_IG03:        ; , epilog, nogc, extend
        ret
-                                               ;; size=1 bbWeight=0.50 PerfScore 0.50
+                                               ;; size=1 bbWeight=1    PerfScore 1.00

-; Total bytes of code 20, prolog size 0, PerfScore 5.38, instruction count 9, allocated bytes for code 20 (MethodHash=704934e0) for method Microsoft.CodeAnalysis.EnumBounds:IsValid(ubyte):bool
+; Total bytes of code 9, prolog size 0, PerfScore 3.40, instruction count 4, allocated bytes for code 9 (MethodHash=704934e0) for method Microsoft.CodeAnalysis.EnumBounds:IsValid(ubyte):bool

In many cases we also remove entire basic blocks because they are now unreachable.

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

As one would expect the throughput impact on x86 is a bit higher than 64-bit platforms, but I think it is at an acceptable level where keeping the code path uniform is preferable.

windows x64

Collection	PDIFF
aspnet.run.windows.x64.checked.mch	-0.01%
benchmarks.run.windows.x64.checked.mch	+0.02%
coreclr_tests.pmi.windows.x64.checked.mch	+0.01%
libraries.crossgen2.windows.x64.checked.mch	+0.01%
libraries.pmi.windows.x64.checked.mch	+0.01%
libraries_tests.pmi.windows.x64.checked.mch	+0.01%

windows x86

Collection	PDIFF
benchmarks.run.windows.x86.checked.mch	+0.05%
coreclr_tests.pmi.windows.x86.checked.mch	+0.03%
libraries.crossgen2.windows.x86.checked.mch	+0.04%
libraries.pmi.windows.x86.checked.mch	+0.05%
libraries_tests.pmi.windows.x86.checked.mch	+0.04%