[Bounds checks] Merge inter-block assertions in RangeCheck::MergeAssertion #121527
Conversation
|
@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. |
There was a problem hiding this comment.
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
TreeAssertionVisitorclass to walk statements and collect assertions from bounds checks before the target operation - Limits the search to blocks with the
BBF_MAY_HAVE_BOUNDS_CHECKSflag to reduce overhead
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:
|
|
@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. |
|
@EgorBo ping on this (just going through PR backlog) did you still want to move forward here? |
…r-ComputeRangeForBinOp
7241934 to
397f223
Compare
When RangeCheck inspects, say, "i + cns" tree, it tries to get the "assertion-based" range for
ivia its block'sbbAssertionIn. It's too conservative and doesn't take into account assertions created before thatiin 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):
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