JIT: consistently handle no return calls in qmarks

[AndyAyersMS]

When we expand QMARKS, ensure that any block with a no-return call gets changed to BBJ_THROW.

This fixes a case I am seeing with cross-block local assertion prop, as the upper QMARK gets optimized away and so we don't check if the expansing has any noreturn calls.

It also happens in places with just within-block local assertion prop.

Contributes to #94363.

[ghost]

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

Issue Details

---

When we expand QMARKS, ensure that any block with a no-return call gets changed to BBJ_THROW.

This fixes a case I am seeing with cross-block local assertion prop, as the upper QMARK gets optimized away and so we don't check if the expansing has any noreturn calls.

It also happens in places with just within-block local assertion prop.

Contributes to #94363.

Author:	AndyAyersMS
Assignees:	AndyAyersMS
Labels:	area-CodeGen-coreclr
Milestone:	-

[AndyAyersMS]

@dotnet/jit-contrib fyi
@jakobbotsch PTAL

A handful of diffs expected (a few more with cross-block enabled, but still not a lot).

Ideally we'd tail merge these, but the noreturn calls often end up with different arguments, and tail merge / throw helper merge aren't able to handle these cases (yet).

[jakobbotsch]

LGTM. Is the fgExpandQmarkForCastInstOf special case still necessary?

[AndyAyersMS]

LGTM. Is the fgExpandQmarkForCastInstOf special case still necessary?

You mean the part that is described by

        // This is a rare case that arises when we are doing minopts and encounter isinst of null
        // gtFoldExpr was still is able to optimize away part of the tree (but not all).
        // That means it does not match our pattern.

I don't see how it can be reached, as the outer qmark is the one that would have the GTF_QMARK_CAST_INSTOF bit, and that's the one that can get optimized away.

Let me see if we get there with SPMI.

[jakobbotsch]

I mean the entirety of fgExpandQmarkForCastInstOf... it was never clear to me why the normal qmark expansion wouldn't be sufficient, apart from this difference in handling of no-return calls.

[AndyAyersMS]

Ah... I can look at this. Maybe as part of revisiting #86778.

[AndyAyersMS]

Will revisit some of the above when I look at moving qmark expansion earlier.