Enable eliding some bounds checks in the jit

[nathan-moore]

• For loop conditionals like i < bnd +- k, fix up assertion prop to generate Bound Oper Bnd assertions instead of Const Loop Bnd assertions.

• When checking the range of a tree, handle comma nodes.

• Add logging for when the jit bails due to encountering a negative constant.

This removes bound checks from the first two cases in #31638.
I also see it enabling removing a couple of other bounds check types, like the ones here with the index casted to a byte:

runtime/src/libraries/Common/src/System/Net/CaseInsensitiveAscii.cs

Lines 90 to 97 in 6a48efa

	private int FastGetHashCode(string myString)
	{
	int myHashCode = myString.Length;
	if (myHashCode != 0)
	{
	myHashCode ^= AsciiToLower[(byte)myString[0]] << 24 ^ AsciiToLower[(byte)myString[myHashCode - 1]] << 16;
	}
	return myHashCode;

The assembly diff is available here.

The two method regressions seem to be caused by register allocation. One of them is here if you wish to peruse through it.

[dnfclas]

All CLA requirements met.

[sandreenko]

PTAL @dotnet/jit-contrib

[sandreenko]

Overall it looks good, thank you @nathan-moore.
I like the diffs:

x64, arm64: Crossgen CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for default jit Summary of Code Size diffs: (Lower is better) Total bytes of diff: -2860 (-0.01% of base) Crossgen CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for linuxnonjit.dll Summary of Code Size diffs: (Lower is better) Total bytes of diff: -9376 (-0.01% of base) Crossgen CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for protononjit.dll Summary of Code Size diffs: (Lower is better) Total bytes of diff: -8744 (-0.01% of base) 

but there are a few regressions, that happens when we create a new function instead of a better old variant.
For example:

N004 ( 5, 5) [000127] ------------ * JTRUE void N003 ( 3, 3) [000124] J------N---- \--* LT int $1c3 N001 ( 1, 1) [000126] ------------ +--* LCL_VAR int V07 loc1 u:2 $1c1 N002 ( 1, 1) [000153] ------------ \--* CNS_INT int 0 $40 

where LCL_VAR is a result of ADD:

N006 ( 5, 5) [000012] -A-XG---R--- * ASG int $1c2 N005 ( 1, 1) [000011] D------N---- +--* LCL_VAR int V07 loc1 d:2 $1c1 N004 ( 5, 5) [000010] ---XG------- \--* ADD int $1c2 N002 ( 3, 3) [000077] ---XG------- +--* ARR_LENGTH int $1c0 N001 ( 1, 1) [000076] ------------ | \--* LCL_VAR ref V13 tmp1 u:1 $83 N003 ( 1, 1) [000009] ------------ \--* CNS_INT int -1 $46 

your new condition catches it when without your changes we were creating Const_Loop_Bnd and
optimized it better.
How did you choose a place for else if (vnStore->IsVNCompareCheckedBoundArith(relopVN)), would it be better to move it down after

 // Cases where op1 holds the condition bound check and op2 is 0. // Loop condition like: "i < 100 == 0" // Assertion: "i < 100 == false" else if (hasTestAgainstZero && vnStore->IsVNConstantBound(op1VN)) 

?

I attached the regressed method logs.
base.txt
diff.txt

Full diffs:
branchRemoval.All.txt

[sandreenko]

How did you choose a place for else if (vnStore->IsVNCompareCheckedBoundArith(relopVN)), would it be better to move it down after

@briansull how would you choose where to put that check?

[nathan-moore]

@sandreenko

your new condition catches it when without your changes we were creating Const_Loop_Bnd and
optimized it better.

As far as I can tell by grepping around the code, Const_Loop_Bnd and Bound_Oper_Bnd are only consumed by rangecheck. In the example you linked, (and in the regressions from the framework), we now remove the range check in those cases. Something just goes awry after that. I compared the lir after lowering in the case you linked, and the only difference I saw was the removal of the range check. That leads me to think it's a register allocation oddity caused by removing the check, which would explain the new spilling, but I could be missing something as that's a bit odd. If you have ideas, I can look into them more.

How did you choose a place for else if (vnStore->IsVNCompareCheckedBoundArith(relopVN)), would it be better to move it down after

I tried moving it to where you suggested and didn't see a difference (I also don't expect it to make a difference). I can move it if you think that location is better; I only put it where it is currently due to symmetry.

[sandreenko]

That leads me to think it's a register allocation oddity caused by removing the check, which would explain the new spilling, but I could be missing something as that's a bit odd. If you have ideas, I can look into them more.

You are right, I have another example when with your changes we don't remove null check on a call anymore, but on the second look we were reaching assertion limit there and that is why we did not create a constant assertion there (Microsoft.CodeAnalysis.VisualBasic.Binder:ReportOverloadResolutionFailureForASingleCandidate, Windows x64), so just a side-effect that should not be fixed.

[sandreenko]

LGTM

[sandreenko]

Thanks @nathan-moore!