Handle overlapped groups of bounds checks

[EgorBo]

A follow up to #112595

[dotnet-policy-service]

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

[EgorBo]

PTAL @jakobbotsch @dotnet/jit-contrib a small addition, mainly to handle

dst[i + 0] = src[i + 0];
dst[i + 1] = src[i + 1];
dst[i + 2] = src[i + 2];
dst[i + 3] = src[i + 3];

here we have 2 groups and if we cut the statements too precisely for e.g. dst array, the next pass won't be able to find 4 checks in the new block for src because one of them will be left out in a different block.

I do want eventually to handle this via 1 joint condition (and 1 joint fallback) - but that will require a bit of work to handle side effects properly

Diffs - no PerfScore/TP regressions

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

[jakobbotsch]

This isn't really correct. The only thing we know here is that the conservative normal VN is the same as the first bounds check's conservative normal VN, but the array length here could still be something like 0 * (1 / 0) + arr.Length (obviously a constructed example) where this code would get rid of the DivisionByZeroException.

Maybe something like

if (bndsChk->GetArrayLength()->OperIs(GT_ARR_LENGTH) &&
    firstBndsChk->GetArrayLength()->OperIs(GT_ARR_LENGTH) &&
    (bndsChk->GetArrayLength()->GetIndirOrArrMetaDataAddr()->gtVNPair.GetConservative() == firstBndsChk->GetArrayLength()->GetIndirOrArrMetaDataAddr()->gtVNPair.GetConservative())
{
  bndsChk->GetArrayLength()->gtFlags |= GTF_IND_NONFAULTING;
  gtUpdateStmtSideEffects(stmt);
}

would get the cases?

[EgorBo]

Actually, even previous code didn't catch anything, so let me just remove this part

[jakobbotsch]

Is block here guaranteed to be nonnull in the case where we cloned and it returned fastPathBb->Prev()?

Might be a bit clearer to have optRangeCheckCloning_DoClone return the next block to visit

[EgorBo]

Addressed

[jakobbotsch]

Another way you could do this without requiring the asserts would be:

BasicBlock* next;
for (BasicBlock* block = fgFirstBB; block != nullptr; block = next)
{
  next = block->Next();
  ...
  next = optRangeCheckCloning_DoClone(...);
}

[EgorBo]

Ah, makes sense, I was thinking about goto 😄 I'll change in a follow up to avoid spinning CI