JIT: Fix assert in object allocation phase

[AndyAyersMS]

The previous DFS and remove phase may leave unreachable pred blocks around. Bail out of conditional escape analysis when this happens.

Fixes #117039.

[Copilot]

Pull Request Overview

The PR updates the object allocation phase to guard against unreachable predecessor blocks by adding a DFS containment check and bailing out of conditional escape analysis when such blocks are encountered.

• Adds a check using comp->m_dfsTree->Contains(predBlock) to detect and handle unreachable predecessors.
• Emits a JITDUMP message and returns false when an unreachable block is found.

Comments suppressed due to low confidence (2)

src/coreclr/jit/objectalloc.cpp:4049

• This new bailout path for unreachable predecessor blocks isn't exercised by existing tests. Consider adding a JIT or unit test that triggers this scenario to ensure the code path is covered.

            if (!comp->m_dfsTree->Contains(predBlock))

src/coreclr/jit/objectalloc.cpp:4056

• The removed comment about EH paths suggests uncertainty around exception-handler edges. Confirm and document whether EH paths are accounted for by this DFS check or require separate handling.

            //

[Copilot]

[nitpick] The standalone // line adds noise without additional context. Consider removing it or merging it with adjacent comment lines for clarity.

Suggested change

//

[AndyAyersMS]

@dotnet/jit-contrib PTAL

This assert was firing in xunit code. Because of how xunit randomizes test execution order the PGO data for this method is quite different from one run to the next, so it took a while to repro this.

[amanasifkhalid]

Are we not walking the blocklist in RPO here? If not, can we? That way, we never waste time with unreachable blocks.

[AndyAyersMS]

Are we not walking the blocklist in RPO here? If not, can we? That way, we never waste time with unreachable blocks.

We are generally walking in RPO, but when we find a CEA opportunity we need to walk backwards in flow from the GDV uses, and that is where we may find unreachable preds.

[amanasifkhalid]

We are generally walking in RPO, but when we find a CEA opportunity we need to walk backwards in flow from the GDV uses, and that is where we may find unreachable preds.

I see. Normally, if a block's pred becomes unreachable, I'd expect the previous phase to remove the edge into the block, so we wouldn't encounter unreachable preds during normal pred edge iteration. In this case, is the unreachable pred we're finding an EH pred?

Sorry for asking for these clarifications when the repro is tricky...

[AndyAyersMS]

We are generally walking in RPO, but when we find a CEA opportunity we need to walk backwards in flow from the GDV uses, and that is where we may find unreachable preds.

I see. Normally, if a block's pred becomes unreachable, I'd expect the previous phase to remove the edge into the block, so we wouldn't encounter unreachable preds during normal pred edge iteration. In this case, is the unreachable pred we're finding an EH pred?

Sorry for asking for these clarifications when the repro is tricky...

Yes it's EH flow from some blocks that couldn't be removed by the DFS/remove phase. We have deeply nested EH in this huge example, here's snippet of the flow graph

BB449 [0539]  0  4  5                       0       0 [000..000)                             (throw ) T4 H5 try { }     i IBC rare keep bwd
BB450 [0540]  1  6  4                       0       0 [000..000)                             (throw ) T6 H4 finally { } i IBC rare keep bwd
...
BB52  [0051]  1  7    BB39                  1.00  162 [???..???)-> BB40(1)                   (callf ) T7                i IBC internal
BB53  [0052]  1  7    BB43                  1.00  162 [???..???)-> BB44(1)                   (callfr) T7                i IBC internal
BB40  [0039]  2  7  6 BB52                  1.00  162 [4AB..4AF)-> BB43(0),BB41(1)           ( cond ) T7 H6 finally {   i IBC keep

BB449/BB450 are unreachable but remain; BB40 has BB450 as an EH pred and is part of the region we want to clone for CEA.

[AndyAyersMS]

Along those lines, we might want to revisit places where we decide we can't remove blocks because of EH, as much of it was written before we had upgraded the flow graph representation and EH removal capabilities.

[amanasifkhalid]

Along those lines, we might want to revisit places where we decide we can't remove blocks because of EH, as much of it was written before we had upgraded the flow graph representation and EH removal capabilities.

Agreed, sounds like something I can take a look at.

[amanasifkhalid]

Thanks for walking me through this