Consider changing BBJ_CALLFINALLY/BBJ_ALWAYS representation of call to finally

[BruceForstall]

A non-exceptional call to an EH finally clause is represented in RyuJIT by two basic blocks:

1. a BBJ_CALLFINALLY, whose block target is the finally clause being called
2. an immediately following BBJ_ALWAYS which represents the block to which the finally clause should return (also called the "continuation" or "finally continuation"). This block must be empty (of code). It is marked with the BBF_KEEP_BBJ_ALWAYS flag. If the JIT can prove that the finally doesn't return (e.g., it always executes a throw), this block won't exist, and the BBJ_CALLFINALLY will have the BBF_RETLESS_CALL flag.

The flow graph is constructed as follows. The single successor of BBJ_CALLFINALLY is its finally clause target. The predecessors of a finally clause entry are all the BBJ_CALLFINALLY which call it. The finally clause ends with a BBJ_EHFINALLYRET block. Its successors are all the BBJ_ALWAYS blocks paired with BBJ_CALLFINALLY blocks which call that finally. The BBJ_ALWAYS block predecessor is the single BBJ_EHFINALLYRET of the finally clause called by the paired BBJ_CALLFINALLY.

Having these two blocks, paired, leads to a lot of special case code in the JIT, including checking for isBBCallAlwaysPair, isBBCallAlwaysPairTail, etc.

Proposal

Consider removing the paired BBJ_ALWAYS block and adding the continuation target as an additional field of the BBJ_CALLFINALLY block. Call this bbFinallyContinuation.

In the flow graph, the successor of the BBJ_EHFINALLYRET would no longer by the BBJ_ALWAYS; it would be all the finally continuation blocks. The finally continuation block predecessors would include both BBJ_EHFINALLYRET and non-finally predecessors.

"Retless" BBJ_CALLFINALLY might no longer need the BBF_RETLESS_CALL flag: a null bbFinallyContinuation would be sufficient.

This adds an additional BasicBlock* to the BasicBlock type that will not be used by most blocks. It's possible that the elimination of bbNext as "fall through" flow will lead to introducing an additional BasicBlock* for BBJ_COND "false" (branch not taken) flow, in which case the BBJ_CALLFINALLY can reuse this additional field.

@dotnet/jit-contrib

[ghost]

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

Issue Details

---

A non-exceptional call to an EH finally clause is represented in RyuJIT by two basic blocks:

1. a BBJ_CALLFINALLY, whose block target is the finally clause being called
2. an immediately following BBJ_ALWAYS which represents the block to which the finally clause should return (also called the "continuation" or "finally continuation"). This block must be empty (of code). It is marked with the BBF_KEEP_BBJ_ALWAYS flag. If the JIT can prove that the finally doesn't return (e.g., it always executes a throw), this block won't exist, and the BBJ_CALLFINALLY will have the BBF_RETLESS_CALL flag.

The flow graph is constructed as follows. The single successor of BBJ_CALLFINALLY is its finally clause target. The predecessors of a finally clause entry are all the BBJ_CALLFINALLY which call it. The finally clause ends with a BBJ_EHFINALLYRET block. Its successors are all the BBJ_ALWAYS blocks paired with BBJ_CALLFINALLY blocks which call that finally. The BBJ_ALWAYS block predecessor is the single BBJ_EHFINALLYRET of the finally clause called by the paired BBJ_CALLFINALLY.

Having these two blocks, paired, leads to a lot of special case code in the JIT, including checking for isBBCallAlwaysPair, isBBCallAlwaysPairTail, etc.

Proposal

Consider removing the paired BBJ_ALWAYS block and adding the continuation target as an additional field of the BBJ_CALLFINALLY block. Call this bbFinallyContinuation.

In the flow graph, the successor of the BBJ_EHFINALLYRET would no longer by the BBJ_ALWAYS; it would be all the finally continuation blocks. The finally continuation block predecessors would include both BBJ_EHFINALLYRET and non-finally predecessors.

"Retless" BBJ_CALLFINALLY might no longer need the BBF_RETLESS_CALL flag: a null bbFinallyContinuation would be sufficient.

This adds an additional BasicBlock* to the BasicBlock type that will not be used by most blocks. It's possible that the elimination of bbNext as "fall through" flow will lead to introducing an additional BasicBlock* for BBJ_COND "false" (branch not taken) flow, in which case the BBJ_CALLFINALLY can reuse this additional field.

@dotnet/jit-contrib

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

[BruceForstall]

While this proposal will eliminate the BBJ_CALLFINALLY/BBJ_ALWAYS pairing, it introduces new problems for flow graph maintenance, as follows:

1. If a CALLFINALLY is removed, the finally block loses one predecessor. The continuation block might also be unreachable, in which case the BBJ_EHFINALLYRET should lose the continuation as a successor, and the continuation should lose the BBJ_EHFINALLYRET as a predecessor. However, we can't easily do this because multiple BBJ_CALLFINALLY might share a continuation. Each BBJ_EHFINALLYRET successor is unique (we don't reference count them). We could look for all the CALLFINALLY that are left and reconstruct the BBJ_EHFINALLYRET successor list, eliminating now-unreferenced continuations, but that seems costly. If we don't remove the continuation, then a dead continuation could continue to be referenced so we wouldn't be able to remove it. In the current "pair" representation, each BBJ_EHFINALLYRET successor returns to a unique CALLFINALLY/ALWAYS pair, and multiple such ALWAYS blocks target a shared continuation. In this way, it's easier to remove unreferenced continuations.
2. Any time a continuation is changed, say by being merged into a fall-through predecessor, not only do all the BBJ_EHFINALLYRET successors need to be updated (as today), but all the CALLFINALLY that call the related finally block need to be searched and each matching continuation needs to be updated. Before, the continuation predecessors included the ALWAYS pair tail, which could be updated directly, without searching for the CALLFINALLY.

[BruceForstall]

In light of the above problems, I'm planning to abandon this proposal.

Instead, I propose a much more incremental change that should help reduce the confusion around the ALWAYS block of the CALLFINALLY/ALWAYS pair:

Proposal 2

Currently, we have BBJ_CALLFINALLY/BBJ_ALWAYS pairs. Create a new block kind, call it BBJ_CALLFINALLYRET, and use it instead of BBJ_ALWAYS in these pairs. Thus, we would have BBJ_CALLFINALLY/BBJ_CALLFINALLYRET pairs. The RET stands for "return", as in, "this block targets where the finally block should return, aka, the continuation".

With this change, BBJ_ALWAYS semantics would not be muddied by its use in BBJ_CALLFINALLY/BBJ_ALWAYS pairs. Asserts and checks should be easier. E.g., we would have:

bool BasicBlock::isBBCallFinallyPairTail() const
{
    return KindIs(BBJ_CALLFINALLYRET);
}

We could get rid of BBF_KEEP_BBJ_ALWAYS.

The block table dump would be slightly more explicit/clearer.

This proposal doesn't introduce any new capability that couldn't be done with the current flowgraph, but it makes these special BBJ_ALWAYS blocks more explicit and separates them from "normal" uses.

[BruceForstall]

I'm going to consider this "done" with #95945

[BruceForstall]

Actually, I want to keep this open until Compiler::fgUpdateCallFinally() is removed.

[BruceForstall]

This is complete with #96159