Do not block folding of unsigned (and unordered) comparisons in value numbering

[SingleAccretion]

These were blocked and it was causing issues for unrolling as the unroller substitutes iterator variables with constants, but if the comparison is unsigned (in other words, the (uint)index >= Length trick was used), later phases do not clean it up. The code to actually handle the folding is already there:

runtime/src/coreclr/jit/valuenum.cpp

Line 862 in a350bc6

int ValueNumStore::EvalComparison(VNFunc vnf, T v0, T v1)

runtime/src/coreclr/jit/valuenum.cpp

Lines 889 to 904 in a350bc6

	else // must be a VNF_ function
	{
	switch (vnf)
	{
	case VNF_GT_UN:
	return T(UT(v0) > UT(v1));
	case VNF_GE_UN:
	return T(UT(v0) >= UT(v1));
	case VNF_LT_UN:
	return T(UT(v0) < UT(v1));
	case VNF_LE_UN:
	return T(UT(v0) <= UT(v1));
	default:
	// For any other value of 'vnf', we will assert below
	break;
	}

The diffs (with --pmi) are as expected:

Vector methods

PMI CodeSize Diffs for System.Private.CoreLib.dll for default jit Summary of Code Size diffs: (Lower is better) Total bytes of base: 6504771 Total bytes of diff: 6504383 Total bytes of delta: -388 (-0.01% of base) diff is an improvement. Top file improvements (bytes): -388 : System.Private.CoreLib.dasm (-0.01% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -70 (-26.92% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int32][System.Int32]:<Equals>g__SoftwareFallback|12_0(byref,System.Runtime.Intrinsics.Vector128`1[Int32]):bool -70 (-26.32% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector256`1[Int64][System.Int64]:<Equals>g__SoftwareFallback|14_0(byref,System.Runtime.Intrinsics.Vector256`1[Int64]):bool -58 (-22.31% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector256`1[Int64][System.Int64]:GetHashCode():int:this -54 (-25.59% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int32][System.Int32]:GetHashCode():int:this -38 (-16.10% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Double][System.Double]:<Equals>g__SoftwareFallback|12_0(byref,System.Runtime.Intrinsics.Vector128`1[Double]):bool -34 (-12.01% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Double][System.Double]:GetHashCode():int:this -34 (-24.64% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int64][System.Int64]:<Equals>g__SoftwareFallback|12_0(byref,System.Runtime.Intrinsics.Vector128`1[Int64]):bool -30 (-19.61% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int64][System.Int64]:GetHashCode():int:this Top method improvements (percentages): -70 (-26.92% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int32][System.Int32]:<Equals>g__SoftwareFallback|12_0(byref,System.Runtime.Intrinsics.Vector128`1[Int32]):bool -70 (-26.32% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector256`1[Int64][System.Int64]:<Equals>g__SoftwareFallback|14_0(byref,System.Runtime.Intrinsics.Vector256`1[Int64]):bool -54 (-25.59% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int32][System.Int32]:GetHashCode():int:this -34 (-24.64% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int64][System.Int64]:<Equals>g__SoftwareFallback|12_0(byref,System.Runtime.Intrinsics.Vector128`1[Int64]):bool -58 (-22.31% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector256`1[Int64][System.Int64]:GetHashCode():int:this -30 (-19.61% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Int64][System.Int64]:GetHashCode():int:this -38 (-16.10% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Double][System.Double]:<Equals>g__SoftwareFallback|12_0(byref,System.Runtime.Intrinsics.Vector128`1[Double]):bool -34 (-12.01% of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1[Double][System.Double]:GetHashCode():int:this 8 total methods with Code Size differences (8 improved, 0 regressed), 36609 unchanged. 

An example.

Draft for now to test this in CI.

[EgorBo]

Nice! Do you need any help with the asserts (look related)? Also, could you please run a full jit-diff (not just corelib)

Assert failure(PID 5132 [0x0000140c], Thread: 4328 [0x10e8]): Assertion failed '!"Unhandled operation in EvalComparison<double>"' in 'ILGEN_0x372a9ae6:Method_0xdc6ff1a4(byte,byte,int,long,ushort,double,long,long):int' during 'Do value numbering' (IL size 11052) File: D:\workspace\_work\1\s\src\coreclr\jit\valuenum.cpp Line: 793 Image: C:\h\w\B21E094A\p\CoreRun.exe 

[SingleAccretion]

Do you need any help with the asserts (look related)?

Thank you! Hopefully fixing those will be straightforward enough.

Also, could you please run a full jit-diff (not just corelib)

Of course! Was holding off so that the final diffs are for the full and proper fix.

[SingleAccretion]

With basic CI passing, posting diffs collected with SPMI:

The diffs

libraries.pmi.windows.x64.checked Summary of Code Size diffs: (Lower is better) Total bytes of base: 73 Total bytes of diff: 64 Total bytes of delta: -9 (-12.33% of base) diff is an improvement. Top file improvements (bytes): -9 : 166188.dasm (-12.33% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -9 (-12.33% of base) : 166188.dasm - Microsoft.Diagnostics.Tracing.Extensions.ETWKernelControl:IsWin8orNewer():bool Top method improvements (percentages): -9 (-12.33% of base) : 166188.dasm - Microsoft.Diagnostics.Tracing.Extensions.ETWKernelControl:IsWin8orNewer():bool 1 total methods with Code Size differences (1 improved, 0 regressed), 0 unchanged. tests.pmi.windows.x64.checked Summary of Code Size diffs: (Lower is better) Total bytes of base: 28116 Total bytes of diff: 27508 Total bytes of delta: -608 (-2.16% of base) diff is an improvement. Top file regressions (bytes): 8 : 86594.dasm (2.34% of base) 4 : 133174.dasm (1.47% of base) Top file improvements (bytes): -82 : 132677.dasm (-31.54% of base) -82 : 135315.dasm (-31.54% of base) -62 : 222622.dasm (-32.98% of base) -62 : 222623.dasm (-32.98% of base) -58 : 222612.dasm (-34.32% of base) -58 : 222613.dasm (-34.32% of base) -35 : 132769.dasm (-5.65% of base) -35 : 135378.dasm (-5.65% of base) -34 : 222615.dasm (-29.82% of base) -34 : 222614.dasm (-29.82% of base) -31 : 210438.dasm (-83.78% of base) -17 : 85415.dasm (-4.71% of base) -16 : 85395.dasm (-4.46% of base) -14 : 132722.dasm (-15.91% of base) 16 total files with Code Size differences (14 improved, 2 regressed), 17 unchanged. Top method regressions (bytes): 8 ( 2.34% of base) : 86594.dasm - DevDiv_591210:ILGEN_METHOD(long,long,bool,short):long 4 ( 1.47% of base) : 133174.dasm - testout1:Func_0_4_2_5_5():short Top method improvements (bytes): -82 (-31.54% of base) : 132677.dasm - testout1:Func_0_1_5_3_1():double -82 (-31.54% of base) : 135315.dasm - testout1:Func_0_1_5_3_1():double -62 (-32.98% of base) : 222622.dasm - GitHub_22815.Program:test256(long):bool -62 (-32.98% of base) : 222623.dasm - GitHub_22815.Program:test256(long):bool -58 (-34.32% of base) : 222612.dasm - GitHub_22815.Program:test128(int):bool -58 (-34.32% of base) : 222613.dasm - GitHub_22815.Program:test128(int):bool -35 (-5.65% of base) : 132769.dasm - testout1:Func_0_2_2_1_5():float -35 (-5.65% of base) : 135378.dasm - testout1:Func_0_2_2_1_5():float -34 (-29.82% of base) : 222615.dasm - GitHub_22815.Program:test128(long):bool -34 (-29.82% of base) : 222614.dasm - GitHub_22815.Program:test128(long):bool -31 (-83.78% of base) : 210438.dasm - TestStructReturns:TestNativeIntFloatFieldCall1():int -17 (-4.71% of base) : 85415.dasm - Test.AA:Main():int -16 (-4.46% of base) : 85395.dasm - Test.AA:Main():int -14 (-15.91% of base) : 132722.dasm - testout1:Func_0_1_5_6_2():long Top method regressions (percentages): 8 ( 2.34% of base) : 86594.dasm - DevDiv_591210:ILGEN_METHOD(long,long,bool,short):long 4 ( 1.47% of base) : 133174.dasm - testout1:Func_0_4_2_5_5():short Top method improvements (percentages): -31 (-83.78% of base) : 210438.dasm - TestStructReturns:TestNativeIntFloatFieldCall1():int -58 (-34.32% of base) : 222612.dasm - GitHub_22815.Program:test128(int):bool -58 (-34.32% of base) : 222613.dasm - GitHub_22815.Program:test128(int):bool -62 (-32.98% of base) : 222622.dasm - GitHub_22815.Program:test256(long):bool -62 (-32.98% of base) : 222623.dasm - GitHub_22815.Program:test256(long):bool -82 (-31.54% of base) : 132677.dasm - testout1:Func_0_1_5_3_1():double -82 (-31.54% of base) : 135315.dasm - testout1:Func_0_1_5_3_1():double -34 (-29.82% of base) : 222615.dasm - GitHub_22815.Program:test128(long):bool -34 (-29.82% of base) : 222614.dasm - GitHub_22815.Program:test128(long):bool -14 (-15.91% of base) : 132722.dasm - testout1:Func_0_1_5_6_2():long -35 (-5.65% of base) : 132769.dasm - testout1:Func_0_2_2_1_5():float -35 (-5.65% of base) : 135378.dasm - testout1:Func_0_2_2_1_5():float -17 (-4.71% of base) : 85415.dasm - Test.AA:Main():int -16 (-4.46% of base) : 85395.dasm - Test.AA:Main():int 16 total methods with Code Size differences (14 improved, 2 regressed), 17 unchanged. 

Two regressions: magic division expansion and a lonely CSE.

It would be good to run the outerloop.

[EgorBo]

Do we have tests for comparisons with NaN (as a constant, not Xunit arg)?

[SingleAccretion]

ls *.cs -r | sls "(<=|==|>=|<|>|!=) ?(double|float).NaN ?(<=|==|>=|<|>|!=)" in src/tests finds nothing, as does manual looking at ls *.il -r | sls "ldc.r(4|8)" which suggests we do not have great coverage for this. There are tests in libs that cover some NaN folding, but I would be very surprised to see them ever hit this path (unordered compares are generally seen when Jit inverts the condition, and those tests are too simple for that).

It is somewhat surprising that's the case, I will look into improving the coverage. Testing this particular change is tricky because it runs late'ish, hopefully I will be able to figure out something out though.

[SingleAccretion]

With the core changes not expected to be in flux, posting some updated diffs (obtained via superpmi.py asmdiffs, diffs obtained via a --pmi run over the shared framework are exactly the same as in the PR's description).

Some tests have been partially folded

benchmarks.run.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 442 Total bytes of diff: 428 Total bytes of delta: -14 (-3.17% of base) diff is an improvement. Top file improvements (bytes): -14 : 24730.dasm (-3.17% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -14 (-3.17% of base) : 24730.dasm - Benchstone.BenchF.DMath:Bench(int):bool Top method improvements (percentages): -14 (-3.17% of base) : 24730.dasm - Benchstone.BenchF.DMath:Bench(int):bool 1 total methods with Code Size differences (1 improved, 0 regressed), 0 unchanged. -------------------------------------------------------------------------------- libraries.crossgen.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 64 Total bytes of diff: 55 Total bytes of delta: -9 (-14.06% of base) diff is an improvement. Top file improvements (bytes): -9 : 72095.dasm (-14.06% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -9 (-14.06% of base) : 72095.dasm - Microsoft.Diagnostics.Tracing.Extensions.ETWKernelControl:IsWin8orNewer():bool Top method improvements (percentages): -9 (-14.06% of base) : 72095.dasm - Microsoft.Diagnostics.Tracing.Extensions.ETWKernelControl:IsWin8orNewer():bool 1 total methods with Code Size differences (1 improved, 0 regressed), 0 unchanged. -------------------------------------------------------------------------------- libraries.pmi.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 73 Total bytes of diff: 64 Total bytes of delta: -9 (-12.33% of base) diff is an improvement. Top file improvements (bytes): -9 : 104699.dasm (-12.33% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method improvements (bytes): -9 (-12.33% of base) : 104699.dasm - Microsoft.Diagnostics.Tracing.Extensions.ETWKernelControl:IsWin8orNewer():bool Top method improvements (percentages): -9 (-12.33% of base) : 104699.dasm - Microsoft.Diagnostics.Tracing.Extensions.ETWKernelControl:IsWin8orNewer():bool 1 total methods with Code Size differences (1 improved, 0 regressed), 0 unchanged. -------------------------------------------------------------------------------- tests.pmi.windows.x64.checked.mch Summary of Code Size diffs: (Lower is better) Total bytes of base: 669757 Total bytes of diff: 657751 Total bytes of delta: -12006 (-1.79% of base) diff is an improvement. Top file regressions (bytes): 8 : 86598.dasm (2.34% of base) 4 : 133178.dasm (1.47% of base) Top file improvements (bytes): -1302 : 247492.dasm (-16.84% of base) -862 : 243249.dasm (-64.81% of base) -679 : 243832.dasm (-10.52% of base) -679 : 243830.dasm (-10.54% of base) -571 : 215453.dasm (-2.17% of base) -571 : 215459.dasm (-2.17% of base) -537 : 243842.dasm (-8.53% of base) -537 : 243840.dasm (-8.53% of base) -407 : 215348.dasm (-1.54% of base) -407 : 215341.dasm (-1.54% of base) -404 : 215476.dasm (-1.52% of base) -404 : 215470.dasm (-1.52% of base) -398 : 215415.dasm (-1.57% of base) -398 : 215421.dasm (-1.57% of base) -398 : 215440.dasm (-1.54% of base) -398 : 215434.dasm (-1.54% of base) -287 : 215514.dasm (-1.10% of base) -287 : 215508.dasm (-1.10% of base) -209 : 133557.dasm (-22.77% of base) -207 : 135929.dasm (-22.95% of base) 56 total files with Code Size differences (54 improved, 2 regressed), 6 unchanged. Top method regressions (bytes): 8 ( 2.34% of base) : 86598.dasm - DevDiv_591210:ILGEN_METHOD(long,long,bool,short):long 4 ( 1.47% of base) : 133178.dasm - testout1:Func_0_4_2_5_5():short Top method improvements (bytes): -1302 (-16.84% of base) : 247492.dasm - _rem:main(System.String[]):int -862 (-64.81% of base) : 243249.dasm - pow3:Main():int -679 (-10.52% of base) : 243832.dasm - _mul:main(System.String[]):int -679 (-10.54% of base) : 243830.dasm - _mul:main(System.String[]):int -571 (-2.17% of base) : 215453.dasm - r8NaNdiv:Main():int -571 (-2.17% of base) : 215459.dasm - r8NaNdiv:Main():int -537 (-8.53% of base) : 243842.dasm - _sub:main(System.String[]):int -537 (-8.53% of base) : 243840.dasm - _sub:main(System.String[]):int -407 (-1.54% of base) : 215348.dasm - r4NaNadd:Main():int -407 (-1.54% of base) : 215341.dasm - r4NaNadd:Main():int -404 (-1.52% of base) : 215476.dasm - r8NaNmul:Main():int -404 (-1.52% of base) : 215470.dasm - r8NaNmul:Main():int -398 (-1.57% of base) : 215415.dasm - r4NaNsub:Main():int -398 (-1.57% of base) : 215421.dasm - r4NaNsub:Main():int -398 (-1.54% of base) : 215440.dasm - r8NaNadd:Main():int -398 (-1.54% of base) : 215434.dasm - r8NaNadd:Main():int -287 (-1.10% of base) : 215514.dasm - r8NaNsub:Main():int -287 (-1.10% of base) : 215508.dasm - r8NaNsub:Main():int -209 (-22.77% of base) : 133557.dasm - testout1:Func_0_6_2_5_1():int -207 (-22.95% of base) : 135929.dasm - testout1:Func_0_6_2_5_1():int Top method regressions (percentages): 8 ( 2.34% of base) : 86598.dasm - DevDiv_591210:ILGEN_METHOD(long,long,bool,short):long 4 ( 1.47% of base) : 133178.dasm - testout1:Func_0_4_2_5_5():short Top method improvements (percentages): -31 (-83.78% of base) : 210425.dasm - TestStructReturns:TestNativeIntFloatFieldCall1():int -862 (-64.81% of base) : 243249.dasm - pow3:Main():int -81 (-45.25% of base) : 81192.dasm - Test.AA:Main():int -58 (-34.32% of base) : 222611.dasm - GitHub_22815.Program:test128(int):bool -58 (-34.32% of base) : 222612.dasm - GitHub_22815.Program:test128(int):bool -62 (-32.98% of base) : 222622.dasm - GitHub_22815.Program:test256(long):bool -62 (-32.98% of base) : 222621.dasm - GitHub_22815.Program:test256(long):bool -82 (-31.91% of base) : 132681.dasm - testout1:Func_0_1_5_3_1():double -82 (-31.91% of base) : 135319.dasm - testout1:Func_0_1_5_3_1():double -34 (-29.82% of base) : 222613.dasm - GitHub_22815.Program:test128(long):bool -34 (-29.82% of base) : 222614.dasm - GitHub_22815.Program:test128(long):bool -207 (-22.95% of base) : 135929.dasm - testout1:Func_0_6_2_5_1():int -209 (-22.77% of base) : 133557.dasm - testout1:Func_0_6_2_5_1():int -1302 (-16.84% of base) : 247492.dasm - _rem:main(System.String[]):int -14 (-15.91% of base) : 132726.dasm - testout1:Func_0_1_5_6_2():long -26 (-13.98% of base) : 135613.dasm - testout1:Func_0_3_5_6_5():float -26 (-13.98% of base) : 133078.dasm - testout1:Func_0_3_5_6_5():float -77 (-11.72% of base) : 248512.dasm - pow0:Main():int -679 (-10.54% of base) : 243830.dasm - _mul:main(System.String[]):int -679 (-10.52% of base) : 243832.dasm - _mul:main(System.String[]):int 56 total methods with Code Size differences (54 improved, 2 regressed), 6 unchanged. -------------------------------------------------------------------------------- 

As an example of a positive diff, the whole pow3 test for floats and doubles has been folded. This method is actually a bit interesting in that if you remove decimal from it entirely, it does not collapse completely, there is a mov ecx 1; test ecx, ecx; je sequence left.

Note for reviewers: all the hand-written changes are in the first 4 commits, all the *.il tests changes have been autogenerated from the existing test cases.

[AndyAyersMS]

@briansull PTAL

[briansull]

Looks Good

[briansull]

Was this a bug before?

[SingleAccretion]

I believe so, yes.

[briansull]

Merging