JIT: Restore EH-region check in if-conversion flow validation

[Copilot]

A Fuzzlyn-generated repro hits assert(!block->HasFlag(BBF_DONT_REMOVE)) during the JIT's If conversion phase when the Then block is the start of a try region. Bisected to dea4756 (#125347), which simplified IfConvertCheckFlow and inadvertently dropped the BasicBlock::sameEHRegion check previously performed by IfConvertCheckInnerBlockFlow. Without it, if-conversion proceeds on a Then/Else block that begins an EH region, then trips the assert when later removing it.

Changes

• src/coreclr/jit/ifconversion.cpp — In OptIfConversionDsc::IfConvertCheckFlow, bail when falseBb (Then) or trueBb (Else, when present) is not in the same EH region as m_startBlock. After merging origin/main, the Else guard uses the new HasElseBlock() helper (which replaced the removed m_doElseConversion member).
• src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs — Regression test using the original repro. The test source is added to the shared merged src/tests/JIT/Regression/Regression_ro_2.csproj (alongside other JitBlue regression tests); no per-test csproj or environment variables are needed.

if (falseBb->GetUniquePred(m_compiler) == nullptr)
{
    return false;
}

// The Then/Else blocks will be removed by if-conversion, so they must be in the same
// EH region as m_startBlock. Otherwise they may be the start of a try/handler region
// (and thus marked BBF_DONT_REMOVE), or removing them could leave dangling EH state.
if (!BasicBlock::sameEHRegion(falseBb, m_startBlock))
{
    return false;
}

m_finalBlock = HasElseBlock() ? trueBb->GetUniqueSucc() : trueBb;

if (HasElseBlock() && !BasicBlock::sameEHRegion(trueBb, m_startBlock))
{
    return false;
}

Verified the assert reproduces on the unfixed JIT and is gone with the fix; the new test passes through the merged test runner (global::Runtime_127446.Runtime_127446.TestEntryPoint() → Passed) after merging the latest origin/main.

[dotnet-policy-service]

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

[BoyBaykiller]

Maybe I should just directly check for BBF_DONT_REMOVE. This handles BasicBlock::sameEHRegion and anything else...

[EgorBo]

@copilot please delete Runtime_127446.csproj, juse use the shared csproj, we don't need explicit env vars for this.

[Copilot]

@copilot please delete Runtime_127446.csproj, juse use the shared csproj, we don't need explicit env vars for this.

Done in 1a10c80 — removed Runtime_127446.csproj and added the test to the shared merged JIT/Regression/Regression_o_3.csproj. Verified it runs and passes through the merged runner (global::Runtime_127446.Runtime_127446.TestEntryPoint() → Passed).

Note

This reply was AI-generated by GitHub Copilot.

[EgorBo]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Merged origin/main in 4e6840c. Conflict was in src/coreclr/jit/ifconversion.cpp where main removed the m_doElseConversion field in favor of a new HasElseBlock() helper; resolved by adapting the EH-region guards to use HasElseBlock(). Verified the build succeeds and the regression test passes.

Note

This reply was AI-generated by GitHub Copilot.

[Copilot]

Pull request overview

This PR fixes an if-conversion flow-validation gap in the CoreCLR JIT: it restores an EH-region guard so the if-conversion transform won’t attempt to remove a Then/Else block that begins an exception-handling region (and may be marked BBF_DONT_REMOVE), avoiding an assertion hit observed via a Fuzzlyn repro.

Changes:

• Reintroduce an EH-region compatibility check in OptIfConversionDsc::IfConvertCheckFlow so if-conversion bails out when the candidate Then/Else blocks aren’t in the same EH region as m_startBlock.
• Add a JitBlue regression test and include it in the merged regression project to exercise the scenario that previously triggered the assertion.

Reviewed changes

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

File	Description
src/coreclr/jit/ifconversion.cpp	Adds BasicBlock::sameEHRegion gating for Then/Else blocks so if-conversion doesn’t remove EH region boundary blocks.
src/tests/JIT/Regression/Regression_ro_2.csproj	Includes the new JitBlue regression source file in the merged regression project.
src/tests/JIT/Regression/JitBlue/Runtime_127446/Runtime_127446.cs	Adds a regression test based on the Fuzzlyn repro that previously triggered the if-conversion assertion.

[github-actions]

🤖 Copilot Code Review — PR #127450

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: The PR fixes a real JIT assertion failure where the if-conversion phase attempted to remove basic blocks that are the start of EH (exception handling) regions (marked BBF_DONT_REMOVE). The linked issue #127446 provides the reproducer.

Approach: The fix adds BasicBlock::sameEHRegion() checks in IfConvertCheckFlow() to bail out when target blocks are in a different EH region from the start block. This is the correct and minimal approach — it prevents if-conversion from creating invalid flow graph state by removing EH region boundaries.

Summary: ✅ LGTM. The fix is correct, well-targeted, and includes a proper regression test. The EH region checks precisely mirror the block removal logic later in the function. Both sub-agent models (Claude Sonnet 4.5, GPT-5.3-Codex) confirmed correctness of the core fix.

---

Detailed Findings

✅ Correctness — EH region check placement is correct

The two sameEHRegion checks exactly mirror the block removal logic at lines 660-677:

• falseBb is always removed → unconditional EH check (line 154) ✓
• trueBb is removed only when HasElseBlock() → conditional EH check (line 161) ✓

When there's no else block, trueBb IS the join/final block and remains in the graph — no EH check needed. The ordering (falseBb check before m_finalBlock computation) avoids unnecessary work on early bail-out.

Flagged by: all 3 models (primary + both sub-agents)

✅ Regression Test — Correctly exercises the bug

The test creates the exact scenario: a conditional branch where the false-target block starts a try region. Without the fix, the JIT would assert in fgRemoveBlock (which asserts !block->HasFlag(BBF_DONT_REMOVE) at fgbasic.cpp:5156). The test is properly included in Regression_ro_2.csproj in alphabetical order, uses the standard [Fact] attribute, and follows JIT regression test naming conventions (Runtime_127446 matching the issue number).

💡 Observation — Test covers only the falseBb path

The test exercises the first EH check (falseBb in a different EH region). The second check (trueBb with HasElseBlock()) doesn't have explicit test coverage in this PR. This is fine as a follow-up concern — the code path is structurally identical and the logic is clearly correct by inspection.

💡 Observation — sameEHRegion vs direct BBF_DONT_REMOVE check

One sub-agent noted that checking HasFlag(BBF_DONT_REMOVE) directly would be more defensive. However, sameEHRegion is the standard JIT idiom for this kind of validation, and blocks with BBF_DONT_REMOVE for non-EH reasons (e.g., merged return blocks) would already fail other flow checks (unique predecessor requirement). The current approach is appropriate.

Generated by Code Review for issue #127450 · ◷

[EgorBo]

/ba-g timeouts