[Bounds checks] Merge inter-block assertions in RangeCheck::MergeAssertion

[EgorBo]

When RangeCheck inspects, say, "i + cns" tree, it tries to get the "assertion-based" range for i via its block's bbAssertionIn. It's too conservative and doesn't take into account assertions created before that i in the block (inter-block assertions). Let's see if we can cheaply just accumulate those by hands (@AndyAyersMS's idea).

A simplified version of the repro is the following (thanks @BoyBaykiller for the repro):

void Test(int[] arr, int i)
{
    arr[i] = 0;  // creates 'i >= 0 && i < arr.Length' assertion
    i++;         // same block as ^

    if (i < arr.Length)
        arr[i] = 0;
}

Codegen diff:

; Method Benchmarks:Test(int[],int):this (FullOpts)
G_M59621_IG01:
       sub      rsp, 40
G_M59621_IG02:
       mov      eax, dword ptr [rdx+0x08]
       cmp      r8d, eax
       jae      SHORT G_M59621_IG05
       mov      ecx, r8d
       xor      r10d, r10d
       mov      dword ptr [rdx+4*rcx+0x10], r10d
       inc      r8d
       cmp      eax, r8d
       jle      SHORT G_M59621_IG04
G_M59621_IG03:
-      cmp      r8d, eax
-      jae      SHORT G_M59621_IG05
       mov      eax, r8d
       xor      ecx, ecx
       mov      dword ptr [rdx+4*rax+0x10], ecx
G_M59621_IG04:
       add      rsp, 40
       ret      
G_M59621_IG05:
       call     CORINFO_HELP_RNGCHKFAIL
       int3     
-; Total bytes of code: 56
+; Total bytes of code: 51

[EgorBo]

@AndyAyersMS cc @dotnet/jit-contrib any opinion on this? unfortunately, there was a bug that made me think the diffs were huge, they turned out to be quite small: diffs

And unavoidable 0.10-0.25 TP impact.

[Copilot]

Pull Request Overview

This PR optimizes bounds checks by improving the RangeCheck::MergeAssertion method to collect intra-block assertions (assertions created within the same block before the current operation). Previously, the method only considered assertions from bbAssertionIn, which is conservative and doesn't include assertions from bounds checks earlier in the same block.

• Adds early exit when no assertions exist (lines 1137-1140)
• Introduces a TreeAssertionVisitor class to walk statements and collect assertions from bounds checks before the target operation
• Limits the search to blocks with the BBF_MAY_HAVE_BOUNDS_CHECKS flag to reduce overhead

[AndyAyersMS]

And unavoidable 0.10-0.25 TP impact.

I wonder if there's some way to mitigate the TP cost -- one guess is that it comes from walking very large blocks hoping we'll find something interesting and (much of the time) failing.

Some ideas along those lines:

• Give up on the forward walk after walking N nodes, so we bail out in very large basic blocks, with the idea that the interesting info is more likely "close" to the current node?
• If you can destructure the bounds check VN, see if some VN input comes from a local def'd in the current block. If not, then the chances that some earlier op in the block asserts something interesting seem low.
• Stop the forward walk if we generate an assertion that is "interesting..." eg tells us something about the VNs in the check that we are trying to optimize.

[EgorBo]

@AndyAyersMS I added the budget check and kept the diffs while reducing the TP regression down to 0.06%-0.12% (diffs)

Unfortunately it's hard to say whether we already found what we looked for and that there is no more useful assertions down the tree.

Also, I simplified the change to use the generic helper fgWalkTreePost which provides execution-order/post just like I needed.

[adamperlin]

@EgorBo ping on this (just going through PR backlog) did you still want to move forward here?

[EgorBo]

@AndyAyersMS could you please review again? I've addressed your feedback. New diffs